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

Add orientation and device attributes on the amp-story element. #21301

Merged
merged 4 commits into from Apr 1, 2019

Conversation

gmajoulet
Copy link
Contributor

@gmajoulet gmajoulet commented Mar 6, 2019

Adds a [orientation=landscape|portrait] attributes on the <amp-story> element, meant to make CSS styling across experiences and orientations easier for publishers.
This orientation attribute describes how the story is displayed, and not the viewport, like would a CSS media query. Eg: for the desktop three panels UI, the orientation will be portrait, when the viewport is actually landscape.

Part of #21661

@newmuis
Copy link
Contributor

newmuis commented Mar 7, 2019

I think the orientation=foo format is better, since it's more future-proof. Maybe instead of surfacing mobile, we should actually surface the UiType? I know it will be redundant with the desktop attribute, but that's just an unfortunate effect of the legacy code (that maybe we could eventually remove, but until then, I guess is not the worst thing)

@gmajoulet
Copy link
Contributor Author

I've been playing a bit with these and tried to think of how our product will evolve. I'll use an example to make it easier to follow:

I'm publisher X, and I write 30 stories. I design a nice template that's using [ui-type=mobile] and [ui-type=desktop] attributes for the content to adapt to different surfaces.
Tomorrow, AMP Stories decides to launch a new experience for the Google Home Hub (with screen), and introduces [ui-type=google-home-hub].
My [ui-type=mobile] styling would work perfectly, but my story is all broken because these styles won't apply anymore.

My point is that I can see cases where we'll want multiple attributes, like for example: [mobile][google-home-hub]. Whereas orientation will always be only one attribute (either portrait or landscape).
Adding more UiType attributes without removing the old ones sound a lot more future proof to me actually, since we're ensuring that the existing styles will always apply, while allowing new publishers to further customize the experience.

Option A
<amp-story orientation="landscape" mobile google-home-hub>
The one I'm trying to advocate for

Option B
<amp-story orientation="landscape" ui-type="mobile, google-home-hub">
I don't like this one because it becomes harder for publishers to write CSS. They could do [ui-type*=google-home-hub] but I think this is pretty advanced and we'd be making it hard for no good reason.

Option C
<amp-story orientation="landscape" ui-type="desktop">
Can't introduce specific mobile modes like google-home-hub without breaking most publishers.

What do you think?

@gmajoulet
Copy link
Contributor Author

Sorry about the double notification, there's also the option D where we remove the orientation attribute and add landscape or portrait right away:
<amp-story landscape mobile google-home-hub>

Copy link
Contributor

@Enriqe Enriqe left a comment

Choose a reason for hiding this comment

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

LGTM :) Only one question I'm curious about

extensions/amp-story/1.0/test/test-amp-story.js Outdated Show resolved Hide resolved
@newmuis
Copy link
Contributor

newmuis commented Mar 8, 2019

@gmajoulet Thanks for thinking this through! I think your comment brings an important point to the surface: the distinction between the device (or its capabilities) and the "UI type." Currently, we're kind of conflating the two.

In your example, a publisher might want to style a story differently on Google Home Hub because of its capabilities (e.g. smaller screen real estate). A UI type, on the other hand, should generally describe the presentation of the content. An example is our current "3 panels" vs. "fullbleed" UI types; they are both reasonable choices given the same device and set of capabilities; it's just the publisher's decision to choose between those two.

I actually think that it is working as intended that we "Can't introduce specific mobile modes like google-home-hub without breaking most publishers;" new UI presentations should always be opt-in, for a publisher.

@gmajoulet
Copy link
Contributor Author

So that'd give
<amp-story orientation="landscape" screen|device="mobile">
Where in the future, we could add a UI Type if we want to support specific things like ui-type="google-home-hub"?

Can you please confirm these next steps?

  1. We keep the [desktop] attribute but replace it in the documentation with screen="desktop"
  2. We introduce screen="desktop|mobile"

@newmuis
Copy link
Contributor

newmuis commented Mar 9, 2019

Yes, I think you've interpreted what I'm saying correctly.

For action item (2), I think we should decide whether it's actually useful to expose this type of information, or whether this is a case where publishers are better off using media queries.

I've been against using media queries because publishers are really trying to discern the UI type. But if there's a UI type that works across multiple screen sizes (say, the FULLBLEED UI type), then publishers are really free to choose their own breakpoints, and there's not much use for us to define those breakpoints on their behalf.... right?

@gmajoulet
Copy link
Contributor Author

Thanks for all the feedback :)
I updated the initial PR description and updated the code.

PTAL

@newmuis
Copy link
Contributor

newmuis commented Mar 13, 2019

The new store state is a LANDSCAPE_STATE boolean when it could have been ORIENTATION_STATE enum, but it will never have more than two values

I think this is the part that I disagree with; we are really encoding more than just "landscape" vs. "portrait" into this attribute, and I think it will help us not have breakages in the future to truly treat this as an enum. It doesn't matter so much for the store, since its state is internal, but I think it matters just in terms of how we think about the attribute. By "landscape" here, we really mean "16:9ish, with a bit of flexibility/responsiveness", but would not, for example, expect 4:1 to work. Similarly, we'd want the flexibility down the line to be able to add a separate "4:1ish, with a bit of flexibility/responsiveness"

This orientation attribute describes how the story is displayed, and not the viewport, like would a CSS media query. Eg: for the desktop three panels UI, the orientation will be portrait, when the viewport is actually landscape.

I think this is a very important distinction; so important, that we should try to capture it in the name and/or values of the attribute.

@gmajoulet
Copy link
Contributor Author

I think this is the part that I disagree with; we are really encoding more than just "landscape" vs. "portrait" into this attribute, and I think it will help us not have breakages in the future to truly treat this as an enum. It doesn't matter so much for the store, since its state is internal, but I think it matters just in terms of how we think about the attribute. By "landscape" here, we really mean "16:9ish, with a bit of flexibility/responsiveness", but would not, for example, expect 4:1 to work. Similarly, we'd want the flexibility down the line to be able to add a separate "4:1ish, with a bit of flexibility/responsiveness"

Do you want to turn it into an enum now, or whenever we want to introduce these?

I think this is a very important distinction; so important, that we should try to capture it in the name and/or values of the attribute.

s/orientation/story-orientation? The orientation attribute is on the <story> element though, but I agree with your point

@newmuis
Copy link
Contributor

newmuis commented Mar 13, 2019

Doesn't seem like it would be too bad to migrate to an enum now? But, more importantly, the attribute values can basically never be changed after we release this, so I think I'd like them to not be "portrait" and "landscape", but rather something closer to what we really mean.

Maybe we say something like target-aspect-ratio="16:9", where we give guarantees that we will letterbox the story if it's more than +/- x%? That way publishers have a guarantee of the content sizing?

Sorry for the back and forth here, but I think this is actually really important to get right if it's how we tell publishers to style their stories

@gmajoulet
Copy link
Contributor Author

Sounds good, let's sync in person about this and update this thread. :)

A few thoughts though:
I'm no longer sure about what we're solving for. If we have target-aspect-ratio="16:9", do we need to provide an exhaustive list of all the ratios? How are publishers supposed to use this? I don't see them writing CSS for [target-aspect-ratio="16:9"] {} [target-aspect-ratio="2:3"] {} etc....

Also, what does it mean for hide-on attributes? hide-on="16:9" doesn't really make sense. If we introduce many ratios it becomes a nightmare for both developers and us. And we can't introduce new ratios as we go, since they'd be visible by default.

We need to find the sweet spot between what allows publishers to create great stories, and what's easy to use and maintain.

@newmuis
Copy link
Contributor

newmuis commented Mar 13, 2019

Synced offline and I'm thoroughly convinced that my feedback in my previous comment was a bad idea 😄

But, it alludes to a larger problem: orientation isn't quite granular enough to let publishers get what they want. For example, iPhone 5 and iPhone X are both portrait, but have drastically different displays.

The conclusion we came to is: media queries give this flexibility, but we need to figure out how to make them work based on the size of the story page instead of the size of the viewport. @gmajoulet and I will investigate.

@gmajoulet
Copy link
Contributor Author

I'll try to summarize all the conversations we had in a few sentences:

  1. We want publishers to be able to write code that works for any platform, and for that we want to provide a way to write media queries for stories, where the media query would match against the story element rather than the viewport.
  2. We want publishers to have easy lazy [landscape] or [portrait] attributes to position their images, ideally with a way to provide some cropping options.
  3. We want publishers to be able to use default templates, like the half/half on landscape.

(3) would rely on (2), so I think we still want this PR in, WDYT?

extensions/amp-story/1.0/amp-story.js Outdated Show resolved Hide resolved
extensions/amp-story/1.0/test/test-amp-story.js Outdated Show resolved Hide resolved
@gmajoulet
Copy link
Contributor Author

PTAL

@gmajoulet gmajoulet merged commit 47bd449 into ampproject:master Apr 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants