Skip to content

🐛 [amp story shopping] Do not render shopping CTA if no shopping tags are on the page #37503

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

Merged
merged 10 commits into from
Feb 2, 2022

Conversation

processprocess
Copy link
Contributor

Building the attachment was refactored as part of #37278 to be a separate component and introduced a regression which was not caught since Percy diffs were broken at the time of submitting the PR.

This PR removes dead code that previously prevented the CTA from being built if no shopping-tags are on the page (originally done in #37004) and puts the functionality in amp-story-shopping-attachment.
It also renames renderOpenAttachmentUI_ to installPageAttachmentExtension_ to better describe functionality.

@amp-owners-bot
Copy link

amp-owners-bot bot commented Jan 27, 2022

Hey @gmajoulet! These files were changed:

extensions/amp-story-shopping/0.1/amp-story-shopping-attachment.js
extensions/amp-story/1.0/amp-story-page.js

Hey @newmuis! These files were changed:

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

Copy link
Contributor

@mszylkowski mszylkowski left a comment

Choose a reason for hiding this comment

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

Nit:

If there are no shopping tags, the attachment extension will still be fetched right? How often would this happen?

We could prevent that by removing the tag from the querySelector on amp-story-page and on shopping-attachment install the extension.

Copy link
Contributor

@mszylkowski mszylkowski left a comment

Choose a reason for hiding this comment

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

Could be useful to add tests to check whether the attachment is built or not, code is 👍

@processprocess processprocess merged commit ca1c7d4 into ampproject:main Feb 2, 2022
@processprocess processprocess deleted the install branch February 2, 2022 14:41
samouri pushed a commit to samouri/amphtml that referenced this pull request Feb 2, 2022
… are on the page (ampproject#37503)

* Rename. Do not render CTA.

* Update comment

* Update test.

* Remove unused

* revert

* revert

* install in shopping attachment

* check in layoutCallback

* Move selectors to buildCallback.

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

Successfully merging this pull request may close these issues.

4 participants