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

Ship remote page attachments #27709

Merged
merged 2 commits into from
Apr 13, 2020

Conversation

cramforce
Copy link
Member

  • Adds docs
  • Adds validator rules
  • Adds example
  • Fixes robustness of drawer stylesheet (Ideally the UI would move into shadow DOM)

- Adds docs
- Adds validator rules
- Adds example
- Fixes robustness of drawer stylesheet (Ideally the UI would move into shadow DOM)
@amp-owners-bot
Copy link

Hey @ampproject/wg-caching, these files were changed:

extensions/amp-story/1.0/test/validator-amp-story-page-attachment-error.html
extensions/amp-story/1.0/test/validator-amp-story-page-attachment-error.out
extensions/amp-story/1.0/test/validator-amp-story-page-attachment.html
extensions/amp-story/1.0/test/validator-amp-story-page-attachment.out
extensions/amp-story/validator-amp-story.protoascii

Comment on lines 774 to 780
attrs: {
name: "href"
value_url: {
protocol: "http"
protocol: "https"
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Having any descendant today breaks the layout. I think we should have two separate validation specs for this <amp-story-page-attachment> tag:

  1. The current one that allows descendants
  2. A new one with a mandatory href attribute, and 0 allowed descendants

The alternative would be to remove all descendants at build time, but the validation spec is more strict.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you push the changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry. Done

Comment on lines +80 to +82
<amp-story-page-attachment layout="nodisplay" href="https://www.example.com"
>...</amp-story-page-attachment
>
Copy link
Contributor

Choose a reason for hiding this comment

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

Making it more clear that there is no children allowed:

Suggested change
<amp-story-page-attachment layout="nodisplay" href="https://www.example.com"
>...</amp-story-page-attachment
>
<amp-story-page-attachment layout="nodisplay" href="https://www.example.com">
</amp-story-page-attachment>

## Allowed content and components
## Linked content

When providing a `href` attribute as page attachment, the respective URL is opened when the user activates the page attachment.
Copy link
Contributor

Choose a reason for hiding this comment

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

Wdyt about adding one line about the fact that this is a page navigation on the web, but can be handled different in native contexts (in-app browser, customized app experience...)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we should specify what exactly happens.

@cramforce cramforce merged commit b213e1a into ampproject:master Apr 13, 2020
@cramforce cramforce deleted the remote-drawer-styling branch April 13, 2020 21:33
twifkak added a commit to twifkak/amphtml that referenced this pull request Apr 15, 2020
@twifkak twifkak mentioned this pull request Apr 15, 2020
twifkak added a commit that referenced this pull request Apr 16, 2020
* cl/305561778 Revision bump for #27624

* cl/305733898 CSS validation changes.

* cl/306335254 Revision bump for #27709

* cl/306343181 Fix javascript validator symbol export.

* cl/306685743 Revision bump for #27768

Co-authored-by: Greg Grothaus <greggrothaus@google.com>
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