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

♻️[amp story page advancement] handle clicks on drawer #36715

Merged
merged 1 commit into from
Nov 2, 2021

Conversation

processprocess
Copy link
Contributor

This simplifies the handle event check when a click is inside a draggable drawer.

This checks if the element is a draggable drawer rather than checking the tag name of elements that extend the draggable drawer.

This will be helpful in the future - when other elements extend draggable drawer (such as amp-story-shopping-attachment), we will not need to add another tag to this list.

@processprocess processprocess self-assigned this Nov 2, 2021
@processprocess processprocess added this to In progress in wg-stories Sprint via automation Nov 2, 2021
@amp-owners-bot
Copy link

amp-owners-bot bot commented Nov 2, 2021

Hey @gmajoulet, @newmuis! These files were changed:

extensions/amp-story/1.0/page-advancement.js

@processprocess processprocess merged commit e5a9aec into ampproject:main Nov 2, 2021
wg-stories Sprint automation moved this from In progress to Done Nov 2, 2021
@processprocess processprocess deleted the drawer-clicks branch November 2, 2021 20:08
tagName === 'amp-story-page-attachment' ||
tagName === 'amp-story-page-outlink'
) {
if (el.classList.contains('amp-story-draggable-drawer-root')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to revert, I'm just trying to offer an alternative argument to consider next times we want to do such optimizations.
The previous implementation had the advantage of being much easier to understand for people with 0 context, making the codebase a bit more accessible. It used to read as "content within page-attachment or page-outlink don't trigger page advancement". Now with this class it's a bit less obvious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree that this needs a comment. Opened #36764

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants