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

🐛 [Story Preview] Wait until the story is visible before pausing for consent resolution #38313

Closed
wants to merge 1 commit into from

Conversation

coreymasanto
Copy link
Contributor

@coreymasanto coreymasanto commented Jun 27, 2022


Background: Stories that include an amp-consent element are paused until the consent has been resolved.

Issue: The consent never resolves in preview mode because amp-consent doesn’t get built until the story is visible. This means that stories that contain amp-consent elements remain paused in preview mode until the story transitions to the visible state.

Fix: Wait until the story is visible before calling pauseStoryUntilConsentIsResolved_().

@coreymasanto coreymasanto self-assigned this Jun 27, 2022
@amp-owners-bot
Copy link

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

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

@coreymasanto
Copy link
Contributor Author

@gmajoulet this PR results in a failure for the 'should pause the story if there is a consent' test, due to setStateStub being called 3 times instead of once. Based on this comment, we probably don't want that to occur. Is this PR acceptable, or should I find a different approach?

@yizhaomtv
Copy link

The consent prevents story preview from autoplaying, shall we merge this change to unblock story preview? @coreymasanto @gmajoulet

Copy link
Contributor

@newmuis newmuis left a comment

Choose a reason for hiding this comment

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

This only applies to stories that utilize consent as a feature, right?

I have some additional questions about this, let's follow up before merging this.

@yizhaomtv
Copy link

Thanks for the update! Do we have a plan for resolving this issue? Otherwise we'll have to manually filter stories that have a consent dialog because they are not previewable.

@coreymasanto
Copy link
Contributor Author

Thanks for the update! Do we have a plan for resolving this issue? Otherwise we'll have to manually filter stories that have a consent dialog because they are not previewable.

There are ongoing talks around how amp-consent should be handled within story previews. The final decision has not yet been made, although it seems increasingly likely that we will need to prevent every story with a consent dialog from being previewed at all. I'll resolve the talks ASAP so that the next steps are clear

@coreymasanto
Copy link
Contributor Author

Closing because we decided last October on two milestones:

  1. Prevent amp-consent stories from being previewed because allowing a preview would be circumventing the publisher-defined condition of needing to hit the Accept button
  2. IF previewing amp-consent stories becomes a high-priority, we can add a preview mode opt-in attribute for amp-consent stories

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

Successfully merging this pull request may close these issues.

4 participants