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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
馃摉 Document media query attributes #32789
Conversation
@@ -0,0 +1,40 @@ | |||
<!--- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. But to confirm: this doc file we'll end up on amp.dev
, right? Or do we need to do something to make sure it does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is automatic, I would expect it to go to https://amp.dev/documentation/guides-and-tutorials/learn/amp-html-responsive-attributes based on where AMP's Layout system and AMP Action and events are located (also in this spec/
directory). @CrystalOnScript @sebastianbenz can one of you confirm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've opened a PR that should add it to the imported documents for amp.dev here:
ampproject/amp.dev#5422
this case, if the page has a screen width of 1000px or more, `valueOne` is used. | ||
If the width is between 999px and 600px, `valueTwo` is used. When it is 599px or | ||
smaller, `defaultValue` is used. | ||
options based on a [media query](./../../spec/amp-html-responsive-attributes.md). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do all attributes allow media queries in this component and all others? Or should we come up with some sort of flag per-attribute. E.g. "visible-count (media allowed)"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the ones that are eligible currently are that way for all of their attributes. Hence there is this one section which says so at the top of the ## Attributes
section. It may not be this way going forward for new components that adopt this, but that pattern you recommended is a good one in that case.
|
||
The media queries are evaluated from left to right, with the first matching | ||
media query being used. A default value (without a media query) is required. In | ||
this case, if the page has a screen width of 1000px or more, `valueOne` is used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unclear to me where or how valueOne is defined? Is this something I declare right after the media query or somewhere else on the page? What kind of things does valueOne include?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends on the attribute. For base-carousel for example, the attr-name
could be orientation
and the possible values could be vertical
and horizontal
, so either of those options can sub in for any of valueOne
, valueTwo
, defaultValue
, etc. That is, your set of value options are defined in the component attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to merge this PR for now but let's come back and revise if we hammer out better phrasing here.
Added new doc per [PR #32789](ampproject/amphtml#32789)
Added new doc per [PR #32789](ampproject/amphtml#32789)
* Create document for responsive attributes * Link to responsive attribute documentation * Fix links * Add example * Use Crystal's suggested copy * Add note
Current working location for the new file:
spec/amp-html-responsive-attributes.md
- open to all suggestions for file name/location/content!Fixes #31909