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
Allow for functional links in amp-accordion headers #9335
Conversation
631db4b
to
69970be
Compare
69970be
to
cc85994
Compare
// for buttons and links, which should not have their default behavior | ||
// overidden. | ||
const target = dev().assertElement(event.target); | ||
if (target.tagName != 'A') { |
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.
Should we support anything else here besides anchor tags?
// for on links, which should not have their default behavior | ||
// overidden. | ||
const target = dev().assertElement(event.target); | ||
if (target.tagName != 'A') { |
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.
this may not work for image links (<a href><amp-img>...
). I think we need to check all the ancestors of target up to the header element. ancestorElements(child, predicate)
should help find it.
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.
done.
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.
lgtm
Thanks! |
Nice—thanks for jumping in on this, @kmh287 ! /cc @camelburrito as FYI |
We should not be doing this in accordion, ideally what you need in #7712 could be achieved by using amp-selector. This goes against what an accordion is supposed to do. Could you hold on this till Tuesday when i am back. |
This PR allows links in accordion headers that can be pressed without expanding or collapsing the corresponding accordion section.
Fixes #7712
/to @aghassemi