[Suggest] refactor to support new QueryList state #2748
Merged
Conversation
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
added 4 commits
July 31, 2018 18:26
also remove isTyping state, which fixes double render. when open, selectedItem appears in placeholder instead of input value. this is a nicer experience and works great with resetOnSelect.
these keys used to clear the selection, now they just cancel a selection in progress and keep the previous selection state.
fix esc/tab testsPreview: documentation | landing | table |
themadcreator
approved these changes
Aug 1, 2018
| > | ||
| <InputGroup | ||
| placeholder="Search..." | ||
| value={inputValue} | ||
| placeholder={isOpen && selectedItemText ? selectedItemText : "Search..."} |
There was a problem hiding this comment.
"", which you set as the default on line 110 is falsy. is that what you intend?
There was a problem hiding this comment.
yes it should say "Search..." instead of ""
default placeholderPreview: documentation | landing | table |
giladgray
added a commit
that referenced
this pull request
Aug 1, 2018
* move resetOnSelect to IListItemsProps so all components share it * ⭐ allow activeItem and query to be un/controlled move required props from IQueryListProps to optional in IListItemsProps! add handleQueryChange to renderer props. move activeItem/query state into IQueryListState * QueryList initialize state and update renderer usage * refactor QueryList to control activeItem and query and always pick an enabled first item * refactor four friends to push state into QueryList * little fixes * common suite of tests for select components * update docs: no value/onChange in inputProps * tzp tests * only invoke onQueryChange if it changed * revert Suggest (followup PR with more extensive changes) * [Suggest] refactor to support new QueryList state (#2748) * refactor Suggest to use QueryList's activeItem and query also remove isTyping state, which fixes double render. when open, selectedItem appears in placeholder instead of input value. this is a nicer experience and works great with resetOnSelect. * add resetOnSelect switch to Suggest example * revert disabled tests * fix esc/tab tests these keys used to clear the selection, now they just cancel a selection in progress and keep the previous selection state. * default placeholder * remove omnibar query prop (semantic conflict) * activeItem: T | null - explicit null for "no active item" - undefined optional prop puts it in uncontrolled mode * maybeRenderClearButton
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Targets #2747
Changes proposed in this pull request:
Suggestto support newQueryListstate and un/controlled features.Suggestnow shows the selected item as the placeholder text when editing.isTyping, which was responsible for inconsistent behavior in the common test suites.resetOnSelectswitch to example