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): implement selectOnNavigation prop #2009

Merged
merged 8 commits into from
Sep 23, 2017
Merged

feat(Dropdown): implement selectOnNavigation prop #2009

merged 8 commits into from
Sep 23, 2017

Conversation

rijk
Copy link
Member

@rijk rijk commented Aug 27, 2017

No description provided.

@levithomason
Copy link
Member

Great, next steps here will be adding an example to the docs in /docs/app/Examples/modules/Dropdown/Usage. Be sure to add the example to the index.js in that directory as well.

Then, we also need a test for this in Dropdown-test.js. We should describe('selectOnKeydown') and add assertions for what it() does when true and false.

@codecov-io
Copy link

codecov-io commented Aug 28, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2009      +/-   ##
=========================================
+ Coverage    99.8%   99.8%   +<.01%     
=========================================
  Files         148     148              
  Lines        2561    2590      +29     
=========================================
+ Hits         2556    2585      +29     
  Misses          5       5
Impacted Files Coverage Δ
src/modules/Dropdown/Dropdown.js 100% <100%> (ø) ⬆️
src/modules/Modal/Modal.js 100% <0%> (ø) ⬆️
src/addons/Portal/Portal.js 100% <0%> (ø) ⬆️
src/modules/Sticky/Sticky.js 100% <0%> (ø) ⬆️
src/modules/Search/Search.js 100% <0%> (ø) ⬆️
src/elements/Step/Step.js 100% <0%> (ø) ⬆️
src/modules/Modal/ModalActions.js 100% <0%> (ø) ⬆️
src/behaviors/Visibility/Visibility.js 100% <0%> (ø) ⬆️

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 b60e2aa...0bf102b. Read the comment docs.

@levithomason
Copy link
Member

Cool, thanks much! We'll get a review on this as soon as we can. I just made a release so we'll target next weekend for releasing this one.

@rijk
Copy link
Member Author

rijk commented Aug 28, 2017

Just to let you know: I was not able to test the Doc example.

@levithomason
Copy link
Member

If you mean automated tests, that's OK. Doc example tests are a little looser than they should be right now. We just render them and assert no errors occurred. This is done automatically for you.

@rijk
Copy link
Member Author

rijk commented Aug 28, 2017

No, I meant I was unable to open the docs locally after building, to see if/how the example looked and worked. The tests should be passing.

@layershifter layershifter changed the title feat(Dropdown): implement selectOnKeydown prop (#1993) feat(Dropdown): implement selectOnKeydown prop Sep 12, 2017
@levithomason
Copy link
Member

levithomason commented Sep 23, 2017

Fixed lint issues, testing manually. Hope to ship this one today as well!

@levithomason
Copy link
Member

Ok, finished updating docs. I renamed this prop to selectOnNavigation since key down still applies for the Enter case.

I added test assertions and doc descriptions to clarify the prop controls the change in value. The fact that onChange is called is a side-effect of changing the value only.

@levithomason levithomason changed the title feat(Dropdown): implement selectOnKeydown prop feat(Dropdown): implement selectOnNavigation prop Sep 23, 2017
@levithomason levithomason merged commit 38aede2 into Semantic-Org:master Sep 23, 2017
@levithomason
Copy link
Member

Released in [email protected]

@rijk rijk deleted the feature/select-on-keydown branch November 29, 2017 13:44
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