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

Support for landscape orientation #2409

Closed
swissspidy opened this issue May 24, 2019 · 9 comments
Closed

Support for landscape orientation #2409

swissspidy opened this issue May 24, 2019 · 9 comments
Labels
Enhancement New feature or improvement of an existing one

Comments

@swissspidy
Copy link
Collaborator

Eventually we need to add support for supports-landscape to support landscape orientation and a full bleed desktop experience. There have been user requests for this already too.

Many things need to be considered for landscape support:

@swissspidy swissspidy added Enhancement New feature or improvement of an existing one AMP-Stories labels May 24, 2019
@westonruter
Copy link
Member

  • Calculation of block position for portrait and landscape (that's gonna be fun)
  • Landscape preview for templates (?)

Since the positions are already percentage-based, would there need to be any change? Granted, if you were making a supports-landscape story then you'd have to be even less particular about exact placement of elements across responsive displays. In this way, desktop could just be a portrait display that is super wide 😄

  • Landscape mode while editing

Being able to toggle between desktop and mobile aspect ratios is the key change here I think, if we don't maintain two separate layouts for each page.

  • Poster image support

Poster images for the stories should be fine as-is, since there is already a STORY_LANDSCAPE_IMAGE_SIZE. But poster image for a video will be different. Perhaps it should just be the same as a background image, except without the focal point picker (and just centered in the viewport). This then raises the question of whether or not background video should get a focal point picker. Or should it be presumed that a suitable video should always be shot with the center being acceptable focal point?

@westonruter
Copy link
Member

I created a test story:

image

And I modified the single_amp-story.php to add the attribute:

diff --git a/includes/templates/single-amp_story.php b/includes/templates/single-amp_story.php
index 8d015ee7..d25db1d8 100644
--- a/includes/templates/single-amp_story.php
+++ b/includes/templates/single-amp_story.php
@@ -39,6 +39,7 @@ the_post();
 		$poster_landscape = wp_get_attachment_image_url( $thumbnail_id, AMP_Story_Post_Type::STORY_LANDSCAPE_IMAGE_SIZE );
 		?>
 		<amp-story
+			supports-landscape
 			standalone
 			publisher-logo-src="<?php echo esc_url( $publisher_logo_src ); ?>"
 			publisher="<?php echo esc_attr( $publisher ); ?>"

When viewing in DevTools emulating as Pixel 2XL it looks as expected:

image

And in desktop (1280x1024):

image

So it's actually pretty good by default. That's the beauty of being mindful of the fundamentally responsive nature of stories.

@westonruter
Copy link
Member

As suggested by @jdelia in #2448, what if we just started out with a toggle for whether the story should have the supports-landscape attribute added? Maybe that would be premature and there should rather be a filter for whether the attribute is added? That would facilitate experimenting with landscape support now, and then we can explore the UI implications later for the editor.

@jdelia
Copy link

jdelia commented May 28, 2019

Thanks, @westonruter :) Perhaps if there was a setting in the plugin to enable supports-landscape globally and then a toggle in each story to override? In any event, a filter would be helpful immediately so that we do not have to modify the plugin.

@westonruter
Copy link
Member

@swissspidy What do you think of this:

diff --git a/includes/templates/single-amp_story.php b/includes/templates/single-amp_story.php
index 8d015ee7..8f4600dd 100644
--- a/includes/templates/single-amp_story.php
+++ b/includes/templates/single-amp_story.php
@@ -40,6 +40,17 @@ the_post();
 		?>
 		<amp-story
 			standalone
+			<?php
+			/**
+			 * Filters whether the story supports landscape.
+			 *
+			 * @param bool    $supports_landscape Whether supports landscape. Currently false by default, but this will change in the future (e.g. via user toggle).
+			 * @param wp_Post $post               The current amp_story post object.
+			 */
+			if ( apply_filters( 'amp_stories_supports_landscape', false, get_post() ) ) {
+				echo 'supports-landscape';
+			}
+			?>
 			publisher-logo-src="<?php echo esc_url( $publisher_logo_src ); ?>"
 			publisher="<?php echo esc_attr( $publisher ); ?>"
 			title="<?php the_title_attribute(); ?>"

A site could opt-in to landscape stories via just:

add_filter( 'amp_stories_supports_landscape', '__return_true' );

I hesitate a bit with the default value. It's a similar story for amp_skip_post. When that filter returns true then the “Enable AMP” toggle is disabled entirely (and conversely #2314). Similarly, when a post type does not have amp post type support, the toggle is disabled, and there is a amp_post_status_default_enabled to change the default state.

In the same way, if someone forcibly applies a filter for amp_stories_supports_landscape then should any such toggle be disabled (forced on or off)?

@swissspidy
Copy link
Collaborator Author

What do you think of this:

That works for me. It's a good first step.

if someone forcibly applies a filter for amp_stories_supports_landscape then should any such toggle be disabled (forced on or off)

While a user toggle would influence that filter's default value, I don't think the filter should necessarily influence the toggle.

But we can discuss the exact logic here at some other point in the future.

@westonruter
Copy link
Member

@jdelia Opened PR #2468 which introduces an amp_story_supports_landscape filter.

@westonruter
Copy link
Member

With #3003 the amp-story-grid-layer element supports the position attribute (being landscape-half-left or landscape-half-right).

@westonruter
Copy link
Member

Here's a plugin that allows you to force a story to support landscape even though we don't yet directly support it in the UI: https://gist.github.com/westonruter/4e4ee2b5790c6218fab40a7b3f90331f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or improvement of an existing one
Projects
None yet
Development

No branches or pull requests

4 participants