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(menu): Allow anchor links as menu list items #3680

Merged
merged 4 commits into from
Oct 19, 2018

Conversation

jamesmfriedman
Copy link
Contributor

@jamesmfriedman jamesmfriedman commented Oct 1, 2018

The current logic prevents anchor links from firing when clicked on in the menu, a fairly common case.
fixes: #3486

The current logic prevents anchor links from firing when clicked on in the menu, a fairly common case.
jamesmfriedman pushed a commit to rmwc/rmwc that referenced this pull request Oct 1, 2018
@codecov-io
Copy link

codecov-io commented Oct 1, 2018

Codecov Report

Merging #3680 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3680   +/-   ##
=======================================
  Coverage   98.48%   98.48%           
=======================================
  Files         120      120           
  Lines        5227     5227           
  Branches      657      657           
=======================================
  Hits         5148     5148           
  Misses         79       79
Impacted Files Coverage Δ
packages/mdc-menu/foundation.js 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 61128be...880a663. Read the comment docs.

@jamesmfriedman jamesmfriedman changed the title Allow anchor links as list items Allow anchor links as menu list items Oct 1, 2018
@williamernest
Copy link
Contributor

williamernest commented Oct 8, 2018

This doesn't work when the user clicks on the text or graphic of the link. We can set pointer-events: none; on the mdc-list-item__text and mdc-list-item__grapic classes when the mdc-list-item is a link.

Adding the following to the end of the mdc-menu will work.

.mdc-menu {
...
  //stylelint-disable selector-max-type, selector-no-qualifying-type
  a.mdc-list-item .mdc-list-item__text,
  a.mdc-list-item .mdc-list-item__graphic {
    pointer-events: none;
  }
  // stylelint-enable selector-max-type, selector-no-qualifying-type
}

@jamesmfriedman
Copy link
Contributor Author

Ah, yeah that makes sense.

@williamernest
Copy link
Contributor

@jamesmfriedman Can you give me access to your repo to add this snippet?

@jamesmfriedman
Copy link
Contributor Author

Sorry for the delay. Added you as a collaborator.

Copy link
Contributor

@williamernest williamernest left a comment

Choose a reason for hiding this comment

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

LGTM

@williamernest williamernest changed the title Allow anchor links as menu list items fix(menu): Allow anchor links as menu list items Oct 18, 2018
@williamernest williamernest merged commit d312271 into material-components:master Oct 19, 2018
@jamesmfriedman
Copy link
Contributor Author

Thanks @williamernest!

@yguedidi
Copy link

@jamesmfriedman @williamernest thanks for the fix! But it looks like it doesn't solve the following comment: #3486 (comment)

@jamesmfriedman jamesmfriedman mentioned this pull request Oct 30, 2018
27 tasks
@williamernest
Copy link
Contributor

@yguedidi we set pointer events none on the elements within the list to ensure the anchor tag gets clicked and we don't prevent default on the event. Any custom markup you add inside the list item should also set pointer events to none.

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.

Menu with Surface prevents links with href from firing
5 participants