-
Notifications
You must be signed in to change notification settings - Fork 2.1k
fix(menu): Updated menu to use list's custom event #4151
Conversation
Wait until #4144 is merged, then merge this to master separately, so that both appear in the changelog. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found an issue with this. Activating a menu item with space now causes the page to scroll, because we're no longer calling preventDefault
under any circumstance here (nor in List, aside from the single-selection case).
See also https://github.com/material-components/material-components-web/pull/4144/files#r245053405
FYI, I think this branch will need to be synced with master and you'll need to test your updated fixture.js against other existing menu screenshot test pages. I think it might break on some of them (e.g. the group-selection test page). See #4172 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this seems reasonable; once #4144 is finished and merged, switch this to base on master so CI will run.
Codecov Report
@@ Coverage Diff @@
## master #4151 +/- ##
========================================
Coverage ? 98.4%
========================================
Files ? 127
Lines ? 5688
Branches ? 757
========================================
Hits ? 5597
Misses ? 91
Partials ? 0
Continue to review full report at Codecov.
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
CLAs look good, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one small nit, otherwise LGTM!
…enu_update_issue3481
…sue3481' into list_custom_event_menu_update_issue3481
…enu_update_issue3481
…sue3481' into list_custom_event_menu_update_issue3481
🤖 Beep boop! Screenshot test report 🚦6 screenshots changed from Details3 Changed:3 Added: |
🤖 Beep boop! Screenshot test report 🚦6 screenshots changed from Details3 Changed:3 Added: |
BREAKING CHANGE: Replaced menu's foundation methods
handleClick
andhandleSelection
withhandleItemAction
to handle list item action (i.e., list's custom eventMDCList:action
)Other Changes:
Fixes #3481