-
Notifications
You must be signed in to change notification settings - Fork 8
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 - no longer update value while navigating dropdown #2029
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
atmgrifter00
changed the title
Select no value update refactor
Select - no longer update value while navigating dropdown
Apr 22, 2024
@mollykreis would you mind buddying this? |
atmgrifter00
commented
Apr 22, 2024
mollykreis
reviewed
Apr 22, 2024
change/@ni-nimble-components-9da2496e-e544-4019-a666-d7fca3046771.json
Outdated
Show resolved
Hide resolved
…ide override for typeaheadBufferChanged.
…ni/nimble into select-no-value-update-refactor
mollykreis
reviewed
Apr 24, 2024
packages/nimble-components/src/select/testing/select.pageobject.ts
Outdated
Show resolved
Hide resolved
mollykreis
approved these changes
Apr 30, 2024
packages/nimble-components/src/select/testing/select.pageobject.ts
Outdated
Show resolved
Hide resolved
…ni/nimble into select-no-value-update-refactor
rajsite
reviewed
May 3, 2024
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.
Didn't finish reviewing today, expect to Monday
rajsite
approved these changes
May 6, 2024
rajsite
reviewed
May 6, 2024
rajsite
reviewed
May 6, 2024
Co-authored-by: Milan Raj <[email protected]>
…ni/nimble into select-no-value-update-refactor
jattasNI
approved these changes
May 6, 2024
rajsite
reviewed
May 7, 2024
rajsite
pushed a commit
that referenced
this pull request
May 7, 2024
# Pull Request ## 🤨 Rationale In #2029 I broke the hover styling for non-selected options. This fixes it. ## 👩💻 Implementation Tiny CSS change. ## 🧪 Testing Manual. As this is hover-based, a matrix test didn't seem possible. ## ✅ Checklist <!--- Review the list and put an x in the boxes that apply or ~~strike through~~ around items that don't (along with an explanation). --> - [ ] I have updated the project documentation to reflect my changes or determined no changes are needed.
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.
Pull Request
🤨 Rationale
While in the midst of implementing the clear select feature, I recognized that the fact that we were updating the value while navigating the dropdown was both confusing (since we don't update the display value) and resulting in an awkward implementation for the feature.
👩💻 Implementation
While we no longer want to update
selectedIndex
(and as a corollary to this,value
) while a user navigates the dropdown, we still want the appropriate option in the dropdown to appear selected/highlighted. The styling was keying off of the state of thearia-selected
attribute, which was being updated when theselected
state of an option changed. Now, as theselected
state is no longer changing, I've created a new attribute for theListOption
,activeOption
, that the CSS for theSelect
leverages to manage the styling of the options.To set the new attribute at the appropriate times required providing overrides to the various
select<>Option
(Next
,Previous
,First
,Last
) methods. In order to accomplish this we have to keep some additional internal state for what the current active index is (the index for which option should currently be highlighted).It was also discovered during the implementation of this fix that there was an issue stemming from the typeahead feature that allows a user to select an option (when there is no filter, or when the dropdown is closed) by typing characters while the
Select
has keyboard focus. This required forking some FASTListbox
implementations to prohibit updates to theselectedIndex
while the dropdown was open.Additionally, since I was already updating related implementation, I was able to tackle this issue as well (where tabbing away from the
Select
could end up with unexpected display).🧪 Testing
Largely this just required updates to tests that used to expect the
selectedIndex
/value
to change while the dropdown was open and a user performed a navigation within the dropdown. Beyond that I provided new tests for the<Tab>
behaviors, new tests to verify the typeahead behaviors, and a slight update to the pageObject for reporting what the selected option is (based on whether or not it's open). This last point could be subject for debate.✅ Checklist