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(Dropdown): update text when item selected with keyboard #2162

Merged
merged 2 commits into from
Oct 16, 2017

Conversation

ayastreb
Copy link
Contributor

@ayastreb ayastreb commented Oct 4, 2017

Fixes #2161
the text on dropdown wasn't updated when item is selected with keyboard after search because searchQuery wasn't cleared if there are multiple search results (it is cleared if there's only one search result). And if searchQuery is not cleared, we do not update text in renderText

@codecov-io
Copy link

codecov-io commented Oct 4, 2017

Codecov Report

Merging #2162 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2162      +/-   ##
==========================================
+ Coverage   99.73%   99.76%   +0.03%     
==========================================
  Files         151      151              
  Lines        2611     2600      -11     
==========================================
- Hits         2604     2594      -10     
+ Misses          7        6       -1
Impacted Files Coverage Δ
src/modules/Dropdown/Dropdown.js 100% <100%> (ø) ⬆️
src/elements/Step/StepGroup.js 100% <0%> (ø) ⬆️
src/views/Statistic/StatisticValue.js 100% <0%> (ø) ⬆️
src/modules/Modal/Modal.js 100% <0%> (ø) ⬆️
src/elements/Step/Step.js 100% <0%> (ø) ⬆️
src/views/Statistic/StatisticGroup.js 100% <0%> (ø) ⬆️
src/elements/Step/StepTitle.js 100% <0%> (ø) ⬆️
src/views/Statistic/Statistic.js 100% <0%> (ø) ⬆️
src/modules/Popup/Popup.js 100% <0%> (ø) ⬆️
src/elements/Step/StepDescription.js 100% <0%> (ø) ⬆️
... and 4 more

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 bc417d9...e3529a9. Read the comment docs.

@layershifter layershifter changed the title fix(Dropdown): update text when item selected with keyboard. (#2161) fix(Dropdown): update text when item selected with keyboard Oct 4, 2017
@layershifter
Copy link
Member

@ayastreb Thanks for the PR, this needs more investigation as it's opposite change to #2112

@layershifter
Copy link
Member

@layershifter
Copy link
Member

In fact, problem comes from condition with optionSize, which was added in #1956. As I think, we should clear searchQuery always, all other cases can be manually handled by user with searchQuery prop.

As I see the following change will not break any test:

-if (isAdditionItem || optionSize === 1) this.clearSearchQuery()
+this.clearSearchQuery()

@levithomason can you clarify?

@levithomason
Copy link
Member

Always clearing seems like the correct solution to me. @ayastreb are there any issues with this feature if we always clear the search query?

@ayastreb
Copy link
Contributor Author

@levithomason seems to be no issues, all tests are green and it works as expected.

@levithomason
Copy link
Member

Thanks guys!

@levithomason
Copy link
Member

Released in semantic-ui-react@0.75.0

@scottrblock
Copy link

Hi @ayastreb and @levithomason, I'm curious as to why this is only the expected behavior if the item is selected with a keyboard. It currently seems that if there are multiple options and one is clicked with the mouse, the searchQuery is not cleared.

This can be handled by locally managing the searchQuery state and clearing it onClose, but I would expect the same default behavior for selecting an item from the keyboard or mouse.

@levithomason
Copy link
Member

Agreed. It should be consistent. This was an oversight. A new issue and PR would be much appreciated.

@Semantic-Org Semantic-Org locked as resolved and limited conversation to collaborators Feb 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants