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

feat(Dropdown): close on blur #467

Merged
merged 1 commit into from
Sep 5, 2016
Merged

feat(Dropdown): close on blur #467

merged 1 commit into from
Sep 5, 2016

Conversation

dylankiss
Copy link
Contributor

Fixed the TODO that the Dropdown didn't close on blur.
I did not yet make it open on focus (other TODO), since I wanted to ask for your comments on this.
In my opinion that behavior is not always wanted, but it is how vanilla SUI works right now.

@codecov-io
Copy link

codecov-io commented Sep 5, 2016

Current coverage is 98.31% (diff: 100%)

Merging #467 into master will increase coverage by <.01%

@@             master       #467   diff @@
==========================================
  Files            98         98          
  Lines          1421       1426     +5   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1397       1402     +5   
  Misses           24         24          
  Partials          0          0          

Powered by Codecov. Last update 50ee76c...f3afccd

@@ -994,6 +1011,7 @@ describe('Dropdown Component', () => {
.simulate('focus')

domEvent.keyDown(document, { key: 'ArrowDown' })
domEvent.keyDown(document, { key: 'ArrowDown' })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a duplicate down arrow here.

Copy link
Member

@levithomason levithomason Sep 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, never mind, I see the close on blur requires another arrow to open.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a comment on 1008 that we should update or remove:

// blur, focus, move item selection down

Since we are now opening after the focus.

@levithomason
Copy link
Member

Thanks for the solid work. I've left a few requests for adding/updating comments only. Then we're good to merge.

@dylankiss
Copy link
Contributor Author

@levithomason Changed the comments to your requests.

@levithomason levithomason merged commit 3580fff into Semantic-Org:master Sep 5, 2016
@levithomason levithomason changed the title fix(Dropdown): close on blur feat(Dropdown): close on blur Sep 5, 2016
@levithomason
Copy link
Member

Released in stardust@0.41.2, thanks much for the PR.

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

Successfully merging this pull request may close these issues.

3 participants