fix(Dropdown): UX behavior of selectedItems and scroll for filtered options. #2375
Conversation
|
So I see that tests failed. I wanted to get your opinion first before starting to fix them.
|
|
Hi there! Thanks for contributing and sorry for the delay. I had a very long and crazy holiday season but I'm getting back to the grind now :) The code and explanations look correct. I'll try to review this one soon.
Yes, thanks!
This should be ensuring that changing the search query sets the selected index to 0. We want to retain that behavior. |
|
Hi guys, I created PR #2523 to address the state of the selected item index when doing searches. It does not address the scrolling items into view though. |
|
I looked at the proposed fixes here and in other PRs. I've updated this PR to fix tests. I also changed the logic a bit as to when/why we set the selected index and scroll items into view. I removed all the scattered calls to set selected index. We were trying to identify all cases where we needed to set it, and call it there. However, even a value prop change would cause a state change and therefore require setting the selected index again. Rather than trace all these down, I've removed all the calls and just set selected index in response to state changes in value. I've done similar with the scrolling into view. There is one additional case for each, resetting the selected index to 0 on query change and scrolling items into view on open. Both of those are also implemented. This works with all current tests. I've also ported and simplified the test from #1510 that asserts the index is properly set when dealing with clicks after a search query. Finally, I've added a memoization optimization to 😅 Hope I don't have to go down that hole again in the next year! Thanks for all the help everyone. |
Codecov Report
@@ Coverage Diff @@
## master #2375 +/- ##
=========================================
- Coverage 99.92% 99.7% -0.23%
=========================================
Files 163 152 -11
Lines 2739 2669 -70
=========================================
- Hits 2737 2661 -76
- Misses 2 8 +6
Continue to review full report at Codecov.
|
|
Alright, I've added a failing test for the bug in #2080. The changes made thus far do not solve this one. The issue is, when a search yields no results the selected index should revert to the currently active item if there is one. Instead, it is currently disregarding the active item's index and keeping 0 as the selected index. The test is |
src/modules/Dropdown/Dropdown.js
Outdated
| this.getMenuOptions.lastSearch = search | ||
|
|
||
| filteredOptions = search(filteredOptions, searchQuery) | ||
| } |
There was a problem hiding this comment.
This seems unwise to add more state management, especially outside of this.state. I thought my PR addressed this issue quite well by ensuring this.state stayed in sync with prop changes. Did you have a chance to review my PR?
| search.simulate('change', { target: { value: '__nonExistingSearchQuery__' } }) | ||
|
|
||
| // click outside, close the dropdown | ||
| domEvent.click(document) |
There was a problem hiding this comment.
This line isn't working really. I noted that once you added describe.only, the call domEvent.click(document) isn't really working, and would also cause a failure in the test closes on click outside because it uses the same call. If you use document.body.click() (like the test blurs the Dropdown node on close by clicking outside component) or domEvent.click(document.body) then things start working. What this causes though is a call to closeOnDocumentClick, which causes the dropdown menu to close, but the focus is still on the input and the input search text is never cleared. In this case, this is correct behavior, since the menu has been closed, but focus is still on the input, so the invalid search text should not be removed. What the test should be doing is calling the handleBlur function which is what you're trying to test with this test case. In summary,
- Your checks below with the wrapper.find() calls aren't working because the dropdown still has the search text in the input and is showing the No results message.
- Need a different way to trigger a blur event instead of document.body.click(). This applies to this test case and to the
blurs the Dropdown node on close by clicking outside componentcase. May need to include another button element to throw the focus to cause a blur to happen. - Actually, I believe the dropdown is working well with your changes, its just the test case isn't correctly testing it
|
Hey guys, Thanks :) |
|
There has been no activity in this thread for 90 days. While we care about every issue and we’d love to see this fixed, the core team’s time is limited so we have to focus our attention on the issues that are most pressing. Therefore, we will likely not be able to get to this one. However, PRs for this issue will of course be accepted and welcome! If there is no more activity in the next 90 days, this issue will be closed automatically for housekeeping. To prevent this, simply leave a reply here. Thanks! |
|
There has been no activity in this thread for 180 days. While we care about every issue and we’d love to see this fixed, the core team’s time is limited so we have to focus our attention on the issues that are most pressing. Therefore, we will likely not be able to get to this one. However, PRs for this issue will of course be accepted and welcome! If there is no more activity in the next 180 days, this issue will be closed automatically for housekeeping. To prevent this, simply leave a reply here. Thanks! |
|
Hello, any updates on this one? Can we expect fix anytime soon? |
|
@MarianSulgan this is open source project: pull, apply changes, fix, test and create PR 🚀 |
|
There has been no activity in this thread for 180 days. While we care about every issue and we’d love to see this fixed, the core team’s time is limited so we have to focus our attention on the issues that are most pressing. Therefore, we will likely not be able to get to this one. However, PRs for this issue will of course be accepted and welcome! If there is no more activity in the next 180 days, this issue will be closed automatically for housekeeping. To prevent this, simply leave a reply here. Thanks! |
…React into fix/dropdown-search-selected-item � Conflicts: � examples/webpack3/config/env.js � examples/webpack3/config/paths.js � examples/webpack3/config/webpack.config.common.js � examples/webpack3/config/webpack.config.prod.js � examples/webpack3/config/webpackDevServer.config.js � examples/webpack3/scripts/start.js � examples/webpack3/src/components/Navbar/Navbar.js � examples/webpack3/src/components/Navbar/NavbarChildren.js � examples/webpack3/src/components/Navbar/NavbarDesktop.js � examples/webpack3/src/index.js � src/modules/Dropdown/Dropdown.js
layershifter
added
needs engineering
PR: Bug Fix 🐛
and removed
needs author feedback
labels
Codecov Report
@@ Coverage Diff @@
## master #2375 +/- ##
==========================================
- Coverage 99.84% 99.70% -0.15%
==========================================
Files 183 152 -31
Lines 3275 2669 -606
==========================================
- Hits 3270 2661 -609
- Misses 5 8 +3
Continue to review full report at Codecov.
|
|
I merged It's great that we have all issues linked to this PR, I will check what actually exists still in |
Issues verification#1485 ✅Is no longer present in the latest #2336 ✅Was also fixed, #2336 (comment) #2080 ⚔️The issue #2 which was qualified as a bug is still present, #2080 (comment) |
Here is an attempt to fix #1485, fix #2336 and fix #2080. Also, this would make the PR #1510 irrelevant now.
selectedIndexof filtered options listsSo, as mentioned in the other bugs, the problem comes from the fact that Dropdown is setting is
selectedIndexusing an index from a small filtered list of options when using search. I don't think it is a problem because it represents the actual state of the list at that time. Instead, I think the fix should be to set theselectedIndexagain when the full list is shown (on open). This is what I did on line 1103This PR also brings two small related fixes.
removed
scrollSelectedItemIntoView()call in open handler: this does not respect the react component lifecycles and could generate bugs. Also, this is already taken care of by a check on thestate.openincomponentDidMount()scrollSelectedItemIntoView when searchQuery has changed: This has been added to
componentDidUpdateand fixes a bug where the scroll position would not get update when typing into search input.This is my first PR on this project and I plan to contribute in the next few weeks. Comments and change request are appreciated. Thanks! 🚀