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-shopping does not display CTA if all tags are invalid and Fixes remote shopping data loading on refresh with page attachment open #38035

Merged
merged 28 commits into from
Apr 19, 2022

Conversation

jshamble
Copy link
Contributor

@jshamble jshamble commented Apr 7, 2022

A follow up PR to #37503 after config validation was added.
Closes #38034 and #38040

@amp-owners-bot
Copy link

amp-owners-bot bot commented Apr 7, 2022

Hey @gmajoulet! These files were changed:

extensions/amp-story-shopping/0.1/amp-story-shopping-attachment.js
extensions/amp-story-shopping/0.1/amp-story-shopping-tag.js
extensions/amp-story-shopping/0.1/test/test-amp-story-shopping-attachment.js
extensions/amp-story-shopping/0.1/test/test-amp-story-shopping-tag.js
extensions/amp-story/1.0/amp-story.js
extensions/amp-story/1.0/history.js

Hey @newmuis! These files were changed:

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

@jshamble jshamble changed the title 🐛 amp-story-shopping does not display CTA on if all tags are invalid 🐛 amp-story-shopping does not display CTA if all tags are invalid Apr 11, 2022
@processprocess
Copy link
Contributor

processprocess commented Apr 11, 2022

This is an async problem that can be fixed by refactoring the promise in layoutCallback to be a Promise.all.

getShoppingConfig and this.localizationService_.getLocalizedStringAsync need to resolve before the listeners are attached.

Copy link
Contributor

@processprocess processprocess left a comment

Choose a reason for hiding this comment

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

This approach prevents the error from throwing but it doesn't fix the async problem.
On refresh sometimes the attachment will not display.
Please see the Promise.all approach.

@processprocess
Copy link
Contributor

Check out the approach in #38090 and how it uses Promise.reject to prevent layoutCallback from running.
This way can remove conditionals and log why the component isn't rendering.

@jshamble jshamble changed the title 🐛 amp-story-shopping does not display CTA if all tags are invalid 🐛 amp-story-shopping does not display CTA if all tags are invalid and Fixes remote shopping data loading on refresh with page attachment open Apr 15, 2022
@jshamble jshamble requested a review from gmajoulet April 19, 2022 00:11
@jshamble jshamble requested a review from gmajoulet April 19, 2022 18:26
extensions/amp-story/1.0/amp-story.js Outdated Show resolved Hide resolved
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.

amp-story-shopping do not render cta when all there are no valid shopping tags
4 participants