Skip to content
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(Search): pressing up/down arrow in the search input causes error #1600

Merged
merged 3 commits into from
Apr 21, 2017

Conversation

josie11
Copy link
Contributor

@josie11 josie11 commented Apr 19, 2017

Attempts to fix the issue raised in #1592 by adjusting how event listeners are added/removed based on search results.

@codecov-io
Copy link

codecov-io commented Apr 19, 2017

Codecov Report

Merging #1600 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1600      +/-   ##
==========================================
+ Coverage   99.74%   99.74%   +<.01%     
==========================================
  Files         141      141              
  Lines        2396     2397       +1     
==========================================
+ Hits         2390     2391       +1     
  Misses          6        6
Impacted Files Coverage Δ
src/modules/Search/Search.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37ff255...bb8b83f. Read the comment docs.

@GregoryPotdevin
Copy link

Wouldn't it be safer to let the event listeners as usual (after all, the "no result" is still an open popup) but fix the listener functions to not throw an error if the results are empty? Is there a risk of keyboard events bubbling up if they aren't caught when there are no results?

@levithomason
Copy link
Member

The Dropdown is almost a copy of this component. Ideally, it would share base logic and features, however, that work was begun but not completed.

It may also be worth looking at how the Dropdown solved this issue. It has the same methods but over time has had a lot more attention and therefore fixes.

@josie11
Copy link
Contributor Author

josie11 commented Apr 20, 2017

Oops, I should have examined scrollSelectedItemIntoView closer instead of making all those other changes, sorry!
I adjusted this to match to how I perceive Dropdown handles this, which is seemingly a one line fix?

@layershifter layershifter changed the title Fix(Search): Pressing up/down arrow in the search input causes error fix(Search): pressing up/down arrow in the search input causes error Apr 20, 2017
@levithomason
Copy link
Member

On the surface, this sounds right. There may is likely also a test we can copy over as well that ensures pressing arrow keys when there are no results actually works.

@levithomason
Copy link
Member

Going to merge this for now since it is such a minor fix resolves a known bug.

@levithomason
Copy link
Member

Released in semantic-ui-react@0.68.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants