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(chips): Add keyboard navigation #4844

Merged
merged 45 commits into from
Jul 1, 2019
Merged

Conversation

patrickrodee
Copy link
Contributor

@patrickrodee patrickrodee commented Jun 18, 2019

Add support for keyboard navigation for chips. Update ripple focus-within mixin behavior.

Demos: action chips, choice chips, filter chips, and input chips.

Fixes #2259

BREAKING CHANGE: Chips markup, adapters, foundations, and events have changed.

@codecov-io
Copy link

codecov-io commented Jun 20, 2019

Codecov Report

Merging #4844 into develop will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4844      +/-   ##
===========================================
+ Coverage    98.86%   98.91%   +0.05%     
===========================================
  Files          119      119              
  Lines         5650     5822     +172     
  Branches       749      778      +29     
===========================================
+ Hits          5586     5759     +173     
+ Misses          63       62       -1     
  Partials         1        1
Impacted Files Coverage Δ
packages/mdc-chips/chip/component.ts 99.07% <100%> (+1.63%) ⬆️
packages/mdc-chips/chip-set/component.ts 98.7% <100%> (+0.28%) ⬆️
packages/mdc-chips/chip/foundation.ts 100% <100%> (ø) ⬆️
packages/mdc-chips/chip-set/foundation.ts 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 c5738ed...f6e87c5. Read the comment docs.

packages/mdc-chips/README.md Show resolved Hide resolved
packages/mdc-chips/README.md Show resolved Hide resolved
packages/mdc-chips/README.md Show resolved Hide resolved
packages/mdc-chips/chip-set/adapter.ts Show resolved Hide resolved
packages/mdc-chips/chip-set/adapter.ts Outdated Show resolved Hide resolved
packages/mdc-chips/chip/adapter.ts Outdated Show resolved Hide resolved
packages/mdc-chips/chip/foundation.ts Outdated Show resolved Hide resolved
@mdc-web-bot
Copy link
Collaborator

🤖 Beep boop!

Screenshot test report 🚦

2 screenshots changed from develop on commit a1ad20c:

Details

@mdc-web-bot
Copy link
Collaborator

🤖 Beep boop!

Screenshot test report 🚦

2 screenshots changed from develop on commit 1be41ea:

Details

@mdc-web-bot
Copy link
Collaborator

🤖 Beep boop!

Screenshot test report 🚦

2 screenshots changed from develop on commit f7cf26d:

Details

@mdc-web-bot
Copy link
Collaborator

🤖 Beep boop!

Screenshot test report 🚦

2 screenshots changed from develop on commit f3879ba:

Details

@mdc-web-bot
Copy link
Collaborator

🤖 Beep boop!

Screenshot test report 🚦

2 screenshots changed from develop on commit ed30175:

Details

@mdc-web-bot
Copy link
Collaborator

🤖 Beep boop!

Screenshot test report 🚦

2 screenshots changed from develop on commit d7ed512:

Details

Copy link
Collaborator

@abhiomkar abhiomkar left a comment

Choose a reason for hiding this comment

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

Looks good overall, left few minor comments.

packages/mdc-chips/chip/constants.ts Outdated Show resolved Hide resolved
packages/mdc-chips/chip-set/component.ts Outdated Show resolved Hide resolved
packages/mdc-chips/chip/foundation.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@abhiomkar abhiomkar left a comment

Choose a reason for hiding this comment

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

Found a minor bug in the screenshot demos where pressing Right Arrow when on the last chip would remove the focus from that chip which makes all chip to lose focus. But expected at least one chip to have focus.

@dorivaught dorivaught added this to the Sprint 6: June 25-July 8 milestone Jun 25, 2019
@patrickrodee
Copy link
Contributor Author

Found a minor bug in the screenshot demos where pressing Right Arrow when on the last chip would remove the focus from that chip which makes all chip to lose focus. But expected at least one chip to have focus.

Ah nice, thanks. I've fixed that and added some tests for it.

@mdc-web-bot
Copy link
Collaborator

🤖 Beep boop!

Screenshot test report 🚦

2 screenshots changed from develop on commit 6da39c4:

Details

packages/mdc-chips/README.md Outdated Show resolved Hide resolved
packages/mdc-chips/chip/foundation.ts Outdated Show resolved Hide resolved
@mdc-web-bot
Copy link
Collaborator

🤖 Beep boop!

Screenshot test report 🚦

2 screenshots changed from develop on commit 6a19fc6:

Details

@mdc-web-bot
Copy link
Collaborator

🤖 Beep boop!

Screenshot test report 🚦

2 screenshots changed from develop on commit 1c011d2:

Details

Copy link
Collaborator

@abhiomkar abhiomkar left a comment

Choose a reason for hiding this comment

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

LGTM, few more comments.

packages/mdc-chips/README.md Outdated Show resolved Hide resolved
packages/mdc-chips/README.md Outdated Show resolved Hide resolved
packages/mdc-chips/_mixins.scss Outdated Show resolved Hide resolved
packages/mdc-chips/chip-set/foundation.ts Outdated Show resolved Hide resolved
test/screenshot/spec/mdc-chips/classes/action.html Outdated Show resolved Hide resolved
test/screenshot/spec/mdc-chips/classes/choice.html Outdated Show resolved Hide resolved
@mdc-web-bot
Copy link
Collaborator

🤖 Beep boop!

Screenshot test report 🚦

2 screenshots changed from develop on commit f6e87c5:

Details

@patrickrodee patrickrodee merged commit 42065fe into develop Jul 1, 2019
@delete-merged-branch delete-merged-branch bot deleted the keyboardNavigationChips2 branch July 1, 2019 17:03
@jamesmfriedman jamesmfriedman mentioned this pull request Nov 4, 2019
93 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants