Skip to content

🐛 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
jshamble merged 28 commits intoampproject:mainfrom
jshamble:shoppingJSONPatch
Apr 19, 2022
Merged

Conversation

@jshamble
Copy link
Copy Markdown
Contributor

@jshamble jshamble commented Apr 7, 2022 •
edited
Loading

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

@jshamble jshamble requested a review from processprocess April 7, 2022 18:47
@amp-owners-bot
Copy link
Copy Markdown

amp-owners-bot bot commented Apr 7, 2022 •
edited
Loading

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 requested a review from coreymasanto April 7, 2022 18:48
processprocess
processprocess requested changes Apr 7, 2022
View reviewed changes
@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
@jshamble jshamble requested a review from processprocess April 11, 2022 19:03
@processprocess
Copy link
Copy Markdown
Contributor

processprocess commented Apr 11, 2022 •
edited
Loading

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.

processprocess
processprocess requested changes Apr 11, 2022
View reviewed changes
Copy link
Copy Markdown
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 processprocess mentioned this pull request Apr 11, 2022
Closed
@jshamble jshamble requested a review from processprocess April 11, 2022 21:09
@processprocess
Copy link
Copy Markdown
Contributor

processprocess commented Apr 12, 2022

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 requested a review from processprocess April 15, 2022 18:07
@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 processprocess April 15, 2022 19:10
gmajoulet
gmajoulet reviewed Apr 18, 2022
View reviewed changes
@jshamble jshamble requested a review from gmajoulet April 19, 2022 00:11
gmajoulet
gmajoulet reviewed Apr 19, 2022
View reviewed changes
@jshamble jshamble requested a review from gmajoulet April 19, 2022 18:26
gmajoulet
gmajoulet approved these changes Apr 19, 2022
View reviewed changes
@jshamble jshamble merged commit 2ef2d96 into ampproject:main Apr 19, 2022
@jshamble jshamble mentioned this pull request May 20, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gmajoulet gmajoulet gmajoulet approved these changes

@coreymasanto coreymasanto Awaiting requested review from coreymasanto

@processprocess processprocess Awaiting requested review from processprocess

Assignees

No one assigned

Labels

PR use: In Beta / Experimental PR use: In Stable

Projects

None yet

Milestone

No milestone

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

@jshamble @processprocess @gmajoulet @ampprojectbot