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

Ban amp-state in Mustache template output #25323

Merged
merged 1 commit into from Oct 30, 2019

Conversation

zhangsu
Copy link
Member

@zhangsu zhangsu commented Oct 30, 2019

This is for consistency with amp-list and amp-form: we don't allow
XHR-firing components to be nested under another XHR-firing template.

/to @choumx

src/sanitation.js Outdated Show resolved Hide resolved
@honeybadgerdontcare
Copy link
Contributor

@rcebulko clicking the owners details link just gives me a bunch of links and not why this failed. Would it be possible for it to say x, y or z did not approve this yet?

This is for consistency with amp-list and amp-form: we don't allow
XHR-firing components to be nested under another XHR-firing template.
@rcebulko
Copy link
Contributor

@rcebulko clicking the owners details link just gives me a bunch of links and not why this failed. Would it be possible for it to say x, y or z did not approve this yet?

I'm not sure exactly what you're asking. Does the details link not show you this?
image

It lists the files in the PR needing approval and says who could approve it. What do you suggest adding?
http://ampproject-owners-bot.appspot.com/tree

@honeybadgerdontcare
Copy link
Contributor

@rcebulko clicking the owners details link just gives me a bunch of links and not why this failed. Would it be possible for it to say x, y or z did not approve this yet?

I'm not sure exactly what you're asking. Does the details link not show you this?
image

It lists the files in the PR needing approval and says who could approve it. What do you suggest adding?
http://ampproject-owners-bot.appspot.com/tree

It does not. Clicking the details link goes to https://ampproject-owners-bot.appspot.com

@rcebulko
Copy link
Contributor

It does not. Clicking the details link goes to https://ampproject-owners-bot.appspot.com

😮
That's weird, I can't reproduce. Can you check what the actual href is on the details link? Are you viewing on desktop or mobile? You're viewing from the PR page correct?

image

@honeybadgerdontcare
Copy link
Contributor

Desktop. Even emulating mobile shows it as the link I get it. But since we're both getting consistently different results I looked at my extensions. I'm using Refined GitHub 19.10.28 and turning it off gets me the link you get. Mystery solved. Sorry for the confusion.

Screenshot 2019-10-30 at 8 24 28 AM

@honeybadgerdontcare
Copy link
Contributor

@choumx for owners approval

@rcebulko
Copy link
Contributor

image

@dreamofabear dreamofabear merged commit aae668b into ampproject:master Oct 30, 2019
@zhangsu zhangsu deleted the amp-state branch October 31, 2019 01:00
micajuine-ho pushed a commit to micajuine-ho/amphtml that referenced this pull request Dec 27, 2019
This is for consistency with amp-list and amp-form: we don't allow
XHR-firing components to be nested under another XHR-firing template.
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.

None yet

5 participants