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.

Sign up for GitHub

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

Jump to bottom

Disallows bookend-config-src attr in amp-story tag #15846

Merged
merged 3 commits into from Jun 8, 2018
Merged

Disallows bookend-config-src attr in amp-story tag #15846

merged 3 commits into from Jun 8, 2018

Conversation

Enriqe
Copy link
Contributor

@Enriqe Enriqe commented Jun 5, 2018

Closes #15796

With the introduction of the bookend config API, we want to deprecate and eventually remove the ability to add a bookend-config-src attribute in the amp-story tag.

@Enriqe Enriqe requested a review from newmuis June 5, 2018 20:48
@newmuis
Copy link
Contributor

newmuis commented Jun 5, 2018

I don't think I get how this does what it's saying. Do we need to somewhere specify in the protoascii that bookend-config-src is deprecated?

Adding @Gregable because I clearly don't know what's going on here 😝

@newmuis newmuis requested a review from Gregable June 5, 2018 20:51
newmuis
newmuis reviewed Jun 5, 2018
View reviewed changes
extensions/amp-story/validator-amp-story.protoascii Outdated
@@ -48,6 +48,8 @@ tags: { # <amp-story>
allowed_protocol: "http"
allowed_protocol: "https"
}
deprecation: "AMP-STORY-BOOKEND"
deprecation_url: "https://www.ampproject.org/docs/reference/components/amp-story#announcements"
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Heads up: Your #15465 changes the name of this section, which will change the URL fragment as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@Enriqe
Copy link
Contributor Author

Enriqe commented Jun 7, 2018

@Gregable friendly ping

@Enriqe
Copy link
Contributor Author

Enriqe commented Jun 8, 2018

@newmuis read to merge

@newmuis newmuis merged commit 2214c20 into ampproject:master Jun 8, 2018
@newmuis
Copy link
Contributor

newmuis commented Jun 8, 2018

Actually just to double-check:

This still allows bookend-config-src to be valid, but throws a deprecation warning, is that correct @Enriqe @Gregable?

@Enriqe
Copy link
Contributor Author

Enriqe commented Jun 8, 2018

Yes it will still be valid but throw a warning like so:

image

twifkak added a commit to twifkak/amphtml that referenced this pull request Jun 15, 2018
@twifkak
cl/200465168 Revision bump for ampproject#15846
8bcdf1a
@twifkak twifkak mentioned this pull request Jun 15, 2018
Merged
Gregable pushed a commit that referenced this pull request Jun 15, 2018
@twifkak @Gregable
Validator rollup (#16087)
6c75d48 
 - cl/200465168 Revision bump for #15846
 - cl/199817818 Revision bump for #15824
 - cl/199808139 Revision bump for #15779
 - cl/199704020 Validator: Comment on prefetch disabl
 - cl/199688832 Validator: Capture HTTP request leak
 - cl/199519251 Remove AMP4Email css whitelist and de
@Enriqe Enriqe deleted the disallow-bookend-config-src branch June 21, 2018 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@newmuis newmuis newmuis left review comments

@Gregable Gregable Gregable approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Enriqe @newmuis @Gregable @googlebot