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

🐛 Position offscreen amp-story-pages using viewport widths #16092

Merged
merged 5 commits into from Jun 21, 2018

Conversation

sanjsanj
Copy link
Contributor

Closes #16067

@newmuis To accommodate for varying desktop widths I have refactored the CSS to use viewport units to position offscreen pages.

What I did was to replace instances of:

translateX(300%)  // Push offscreen to the right

With:

translateX(50vw)

And instances of:

translateX(-350%)  // Push offscreen to the left

With:

translateX(calc(-50vw - 100%))

The -100% is to push the <amp-story-page> further to the left by its own width.

In some cases the scale is set to 0.9 in which case I multiply the values by 1.1 in order to compensate.

Before

before

After

after

(nb: There is some artifacting in the screenshots due to compression)

@sanjsanj sanjsanj changed the title 🐛 16067 - Position offscreen pages using viewport widths 🐛 16067 - Position offscreen pages using viewport widths Jun 16, 2018
@sanjsanj sanjsanj changed the title 🐛 16067 - Position offscreen pages using viewport widths 🐛 Position offscreen amp-story-pages using viewport widths Jun 16, 2018
@@ -82,7 +82,7 @@ amp-story[standalone][desktop] {
}

[desktop] amp-story-page {
transform: scale(1.0) translateX(-350%) translateY(0%) !important;
transform: scale(1.0) translateX(calc(-50vw - 100%)) translateY(0%) !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM, but I wonder if the precision is worth the extra computation? Since it's offscreen, we could do something like -200vw and it should be okay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@newmuis Sounds good to me!

@newmuis newmuis merged commit fa85965 into ampproject:master Jun 21, 2018
alabiaga added a commit that referenced this pull request Jun 21, 2018
alabiaga added a commit that referenced this pull request Jun 22, 2018
…6227)

* Revert "Revert "Update dependencies for default 🌴 (#16221)"

This reverts commit f8da39e.

* Revert "Mark 4 more visual diff tests as flaky (#16214)"

This reverts commit 867e903.

* Revert "Update dependencies for default 🌴 (#16195)"

This reverts commit 34d1977.

* Revert "Fix selector (#16197)"

This reverts commit f741868.

* Revert "🐛 Position offscreen amp-story-pages using viewport widths (#16092)"

This reverts commit fa85965.

* Revert "Use performance.now over Date.now in performance metrics (#16119)"

This reverts commit 0d2b567.

* fix lint errors

* fix lint error in constructor

* Update performance-impl.js

fix more lint errors

* Update url-replacements-impl.js

more lint..

* Update test-amp-access.js

fix test error.

* Update test-amp-access-source.js

fix more tests...
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

3 participants