-
Notifications
You must be signed in to change notification settings - Fork 832
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
Fix Combobox filtering with custom itemToString #495
Conversation
5692c0c
to
6319b07
Compare
6319b07
to
0f06c74
Compare
package.json
Outdated
@@ -24,7 +24,7 @@ | |||
"sideEffects": false, | |||
"scripts": { | |||
"test": "xo && ava", | |||
"prepublishOnly": "rm -rf esm commonjs && yarn run build", | |||
"prepare": "rm -rf esm commonjs && yarn run build", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you change this? prepublishOnly
is an npm lifecycle hook that makes sure evergreen is rebuilt automatically when people publish.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, so is prepare
:) https://docs.npmjs.com/misc/scripts#description
I wish npm
had a separate object for lifecycle hooks (lifecycle: { prepare: ... }
), so that this is less so black magic 😬
But from the docs (prepare
is a superset of prepublishOnly
):
prepare: Run both BEFORE the package is packed and published, on local npm install without any arguments, and when installing git dependencies (See below). This is run AFTER prepublish, but BEFORE prepublishOnly.
prepublishOnly: Run BEFORE the package is prepared and packed, ONLY on npm publish. (See below.)
Admittedly, I forgot this was still here -- I was mainly using it for testing in another repo. But in general, this hook builds evergreen
when you add it as a dependency, which allows you to reference a git branch (f.e. "evergreen-ui": "segmentio/evergreen#colin/item-to-string-autocomplete",
). It's pretty useful for testing a branch like this out in a production setting.
However, not really sure if we want to include this change since it may slow down install times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverting this change and dismissing the review so I can ship this :)
Merged master + double-checked that all storybook entries for |
Fixes: #215
If you supply an
itemToString
to anAutocomplete
/Combobox
, we will now correctly fuzzy filter on the string values.