-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[Select] Create-able Select components (a different approach) #3381
Merged
Merged
Changes from 29 commits
Commits
Show all changes
32 commits
Select commit
Hold shift + click to select a range
6668d07
Add option to create new item
3fcd343
Properly clear input on item click
b42418f
Move props to queryList
2548a01
Lint and test
bccb5e2
Lint and docs
684919c
Properly handle keyboard events with create item
shuyangli ff20ae6
Merge branch 'develop' into sl/1710-create-able-multiselect
shuyangli 6964bbb
Minor cleanup
shuyangli 4596f78
Factor out getActiveItem
shuyangli 01a1538
createItem => createNewItem
d99d570
Fix existing tests
6b676d5
Use real item list in filtering test
2aa4b54
More type-safe active item getter
321cd03
Add tests for interacting with create item
4c47f03
Add docs for creating items
59a86aa
Merge branch 'develop' into sl/1710-create-able-multiselect
f54513f
Try a non-breaking API that adds a new generic type C
cmslewis cde4086
Actually, just create a reserved type with a __brand, and incorporate…
cmslewis 4ea4706
Update examples
cmslewis f86db48
Update tests
cmslewis dfbcedd
Update function comments
cmslewis 16b9fe5
Avoid breaking the API, fix tests
cmslewis d1fd4fe
Update docs
cmslewis 2925fec
Make the brand even more specific, and add a deep-comparison check
cmslewis 3e6d62f
Fix unintentional code changes
cmslewis 015e06f
Fix lint in unrelated files?
cmslewis 6ecdfa0
Respond to @adidahiya CR
cmslewis 631b86e
Fix examples: Add created item to list of items (to enable deselection)
cmslewis 3e020ed
Hide 'Create item' option if query already appears in items
cmslewis f126a9d
Write tests
cmslewis 1f7b3bb
Share created-item logic in Select and Suggest components too
cmslewis b9e363f
Small optimization: compare created item to filteredItems, not items
cmslewis File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This reconciliation logic now falls on the users—is it possible to keep this inside the multiselect component? i.e. wrap the passed-in
onItemSelect
and move the logic for maintaining the union of {passed-in items, newly-created items}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.
@shuyangli It's a good thought, but it seems like the component should remain as data-agnostic as possible. Tracking created items would violate that. @adidahiya what do you think?
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.
In particular, a major blocker for doing that is that if a user passes in a totally different set of
items
, wiping away the ones from before, then our components won't have any way to know if they should clear their internally tracked list ofcreatedItems
(which would be stored inthis.state
, presumably). The mutually dependentprops.items
andstate.createdItems
could easily get out of sync and cause more problems than they solve.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.
yeah I agree with @cmslewis here, I would prefer to do less data handling in the component and keep it more presentation-focused. I know it's pretty verbose for consumers this way, but we can always add additional conveniences on top of this approach later on after we see concrete use cases of this feature