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

Ignore amp-access on first story page, but trigger an error on dev. #17813

Merged
merged 2 commits into from Oct 24, 2018

Conversation

Projects
None yet
4 participants
@gmajoulet
Copy link
Collaborator

gmajoulet commented Aug 31, 2018

Ignore amp-access and amp-access-hide attributes on the first amp-story-page, but trigger an error on dev.

Related to #12180

@gmajoulet gmajoulet requested review from newmuis and Enriqe Aug 31, 2018

@googlebot googlebot added the cla: yes label Aug 31, 2018

@Enriqe

Enriqe approved these changes Aug 31, 2018

// First amp-story-page can't be paywall protected.
// Throws an error during development, but just ignore the amp-access rule
// in production.
dev().assert(

This comment has been minimized.

Copy link
@newmuis

newmuis Aug 31, 2018

Collaborator

Honestly, I think this should just be user(); if it wasn't so complicated to enforce this with the validator, I think we would do that too.

@@ -827,6 +827,16 @@ export class AmpStory extends AMP.BaseElement {
accessService.areFirstAuthorizationsCompleted();
accessService.onApplyAuthorizations(
() => this.onAccessApplyAuthorizations_());

const firstPage = this.element.querySelector('amp-story-page');

This comment has been minimized.

Copy link
@newmuis

newmuis Aug 31, 2018

Collaborator

It should be faster to get this.pages_[0].element, since we can avoid querying the DOM.

@gmajoulet gmajoulet force-pushed the gmajoulet:access_not_on_first_page branch from 293bc85 to 6b7a262 Oct 4, 2018

@gmajoulet

This comment has been minimized.

Copy link
Collaborator Author

gmajoulet commented Oct 4, 2018

PTAL


const firstPage = this.pages_[0].element;
// First amp-story-page can't be paywall protected.
// Throws an error during development, but just ignore the amp-access rule

This comment has been minimized.

Copy link
@Enriqe

Enriqe Oct 4, 2018

Collaborator

Is this still true for user()?

This comment has been minimized.

Copy link
@gmajoulet

gmajoulet Oct 22, 2018

Author Collaborator

Yup, this is intended for the publishers, whereas dev is meant for AMP core developers.

@gmajoulet gmajoulet force-pushed the gmajoulet:access_not_on_first_page branch from 6b7a262 to 7083f60 Oct 22, 2018

@gmajoulet

This comment has been minimized.

Copy link
Collaborator Author

gmajoulet commented Oct 22, 2018

Friendly ping

@newmuis
Copy link
Collaborator

newmuis left a comment

Sorry, didn't realize this was waiting on my review!

// First amp-story-page can't be paywall protected.
// Throws an error during development, but just ignore the amp-access rule
// in production.
user().assert(

This comment has been minimized.

Copy link
@newmuis

newmuis Oct 22, 2018

Collaborator

Maybe we should also remove the attribute from the DOM? It would be nice to provide the consistency that you can tell what's going on by looking at the DOM.

This comment has been minimized.

Copy link
@gmajoulet

gmajoulet Oct 22, 2018

Author Collaborator

I don't have a strong opinion on that, but since we display an error message, if a developer tries to use the devtools to look at what they should remove, and see nothing, it might be confusing.
At the same time, if we do what you suggest, we could get rid of the bypassPaywall statements in the switchTo method.
Do you have any preference?

This comment has been minimized.

Copy link
@newmuis

newmuis Oct 23, 2018

Collaborator

Wrong button! My preference would be for removing the DOM attribute and the bypassPaywall special case.

Keeping the DOM consistent also makes it easier for us to debug, since the DOM is served unobfuscated but the JS is minified.

@gmajoulet gmajoulet force-pushed the gmajoulet:access_not_on_first_page branch from 7083f60 to 3d59326 Oct 23, 2018

@gmajoulet

This comment has been minimized.

Copy link
Collaborator Author

gmajoulet commented Oct 23, 2018

PTAL

@gmajoulet gmajoulet force-pushed the gmajoulet:access_not_on_first_page branch from 3d59326 to 78d5581 Oct 23, 2018

@gmajoulet gmajoulet merged commit ce11e5c into ampproject:master Oct 24, 2018

4 checks passed

LGTM analysis: JavaScript No alert changes
Details
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
percy/amphtml Visual review automatically approved, no visual changes found.
Details

Enriqe added a commit to Enriqe/amphtml that referenced this pull request Nov 28, 2018

Ignore amp-access on first story page, but trigger an error on dev. (a…
…mpproject#17813)

* Ignore amp-access on first story page, but trigger an error on dev.

* Remove the amp-access attributes from the first amp-story-page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.