Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#688 New Autosuggest component for SelectorSelectorField #1059

Merged
merged 22 commits into from
Aug 13, 2021

Conversation

BLoe
Copy link
Collaborator

@BLoe BLoe commented Aug 13, 2021

This fixes #688

This change includes several things:

  • Replaces react-select with react-autosuggest in the SelectorSelectorField component, wrapping react-autosuggest with a new component - CreatableAutosuggest - and passing in a small list item component - SelectorListItem - from SelectorSelectorField

  • Sets up storybook examples for SelectorListItem and CreatableAutosuggest:

image

image

  • Sets up an initial example of a standard way we can organize React components going forward:
/componentName
    ComponentName.tsx
    ComponentName.stories.tsx
    ComponentName.test.tsx
    ComponentName.module.css
  • Introduces CSS modules with a typescript plugin. This allows us to define styles using css (or sass) like this:
.container {
  cursor: default;
  padding-right: 10px;
  align-items: center;
}

and then we can import and use the module css styles like this:

import styles from './SelectorListItem.module.css';

...

  <div className={styles.container}>

@BLoe BLoe requested a review from twschiller August 13, 2021 13:37
@twschiller
Copy link
Contributor

We're getting warnings/deprecations during the initial Webpack build coming from bootstrap. @fregante do you have any experience with TypeScript + CSS Modules (this PR is also starting our evaluation of CSS modules)

WARNING: You probably don't mean to use the color value black in interpolation here.
It may end up represented as black, which will likely produce invalid CSS.
Always quote color names when using them as strings or map keys (for example, "black").
If you really want to use the color value here, use '"" + $color'.

  ╷
4 │     --#{$color}: #{$value};
  │         ^^^^^^
  ╵
    node_modules/bootstrap/scss/_root.scss 4:9       @import
    node_modules/bootstrap/scss/bootstrap.scss 11:9  @import
    src/vendors/theme/app/app.scss 22:9              @import
    src/options.scss 17:9                            root stylesheet

src/Globals.d.ts Outdated
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

declare module "*.module.css";
Copy link
Contributor

@twschiller twschiller Aug 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BLoe Are these declarations missing something?

The documentation has more: https://github.com/mrmckeb/typescript-plugin-css-modules#custom-definitions

I think this should probably also help with the IDE autocomplete issue you were seeing?

declare module "*.module.css" {
  const classes: Record<string, string>;
  export default classes;
}

declare module "*.module.scss" {
  const classes: Record<string, string>;
  export default classes;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure what those extra parts were doing, and those lines only seemed to appear in some of the guides I looked at. I am not really seeing any difference when I add those lines and re-build though. They aren't related to the Sass warnings or anything.

@fregante
Copy link
Collaborator

fregante commented Aug 13, 2021

We're getting warnings/deprecations during the initial Webpack build coming from bootstrap. @fregante do you have any experience with TypeScript + CSS Modules (this PR is also starting our evaluation of CSS modules)

That's a sass error: https://stackoverflow.com/q/57455245/288906

Also: sass/sass#1687

And more specifically this seems to be a bootstrap issue and their "solution" seems to be to use node-sass instead of sass: twbs/bootstrap#29219

@twschiller
Copy link
Contributor

That's a sass error:

Yep - I was curious as to why we weren't seeing it before. It seems related to using typescript-plugin-css-modules as this PR doesn't introduce any new @import statements in CSS.

@BLoe Are these files now getting compiled when they hadn't been before? I would they they were getting compiled before too

@fregante

This comment has been minimized.

@fregante
Copy link
Collaborator

fregante commented Aug 13, 2021

Oh I see what's happening. I assumed that this issue was already solved:

It wasn't happening because we are using node-sass, so it's never been a problem. However typescript-plugin-css-modules is using dart-sass internally, reparsing the SASS files with it, surfacing the warning:

https://github.com/mrmckeb/typescript-plugin-css-modules/blob/82ba03548c3d2193508e1a4dbc47c1aa9b22943c/package.json#L81

package.json Show resolved Hide resolved
@BLoe
Copy link
Collaborator Author

BLoe commented Aug 13, 2021

@fregante @twschiller The typescript plugin has an open issue about removing the sass dependency.

It looks like there are other plugins available to accomplish what we're trying to do here, so I'm going to investigate using a different one.

@fregante
Copy link
Collaborator

IMHO it's a "bug" we should resolve or learn to live with since we'll end up switching to sass eventually as per:

Also it's just a warning and doesn't affect the actual build, I think.

@BLoe
Copy link
Collaborator Author

BLoe commented Aug 13, 2021

Yeah, I mean, for that it sounds like we need to consider updating to the latest bootstrap version, right?

@BLoe BLoe marked this pull request as ready for review August 13, 2021 20:08
);

export const disableSelector = liftBackground(
Copy link
Contributor

@twschiller twschiller Aug 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does the same thing as disableOverlay above? What's the point of having both?

  • Remove duplicate method

Copy link
Contributor

@twschiller twschiller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

@BLoe BLoe merged commit d7186b5 into main Aug 13, 2021
@BLoe BLoe deleted the feature/688-selector-selector branch August 13, 2021 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support customization of selector in SelectorSelectorControl component
3 participants