Flying carpet: make container full viewport and center content#8292
Flying carpet: make container full viewport and center content#8292dvoytenko merged 1 commit intoampproject:masterfrom
Conversation
| // Hmm, can the page height change and affect us? | ||
| user().assert( | ||
| layoutBox.top >= viewportHeight, | ||
| layoutBox.top >= viewportHeight * 0.8, |
There was a problem hiding this comment.
@jridgewell I'm okay with this since the overlap and the box could be a max of 0.2 * viewport. Worst case will look like this, which still shows the bottom of the content on scroll.
Missing anything? (btw, didn't find any documentation asking not to add flying carpet in the first and last viewport - did we just miss it?)
There was a problem hiding this comment.
Worse case is actually when it appears on the first viewport and does not touch the bottom of the viewport (so the whole carpet is visible in first viewport). Then, you'll never see the bottom of the content.
No documentation was written, just an issue you brought up in hangouts.
| -webkit-transform: translateZ(0) !important; | ||
|
|
||
| /* Center contents in the container */ | ||
| display: flex; |
There was a problem hiding this comment.
Will this play well with other centering techniques? If not, we should definitely bump the version.
Alternatively, this could be done with a container element and a flex-item amp-child.
There was a problem hiding this comment.
I think it will play well. But I'm still testing around. Just added a good improvement to support responsive layout as well. PTAL.
jridgewell
left a comment
There was a problem hiding this comment.
I have no understanding of flex-box. If you say this will work, LGTM. Defering to @jasti about the broken implementation in the first/last viewport.
|
@jridgewell @jasti as mentioned, I'm splitting this fix from positioning fixes. Will run those in a separate issue/PR. Will merge this fix as soon as green. Thanks! |
* master: (34 commits) Prevent amp-carousel next/previous icons fade away on desktop (ampproject#8428) Turn on flag slidescroll-disable-css-snap” (ampproject#8436) Revert "temporarily turn off yarn (ampproject#8356)" (ampproject#8384) initial commit (ampproject#8404) Upgrades for Index Exchange amp-ad tags to report load statistics (ampproject#8054) amp-bind validation tweak (ampproject#8414) Fix an amp-instagram race condition (ampproject#8192) Use whitelist to restrict urlReplacement for scoped analytics element (ampproject#8360) Report active experiments in error logs (ampproject#8108) amp-bind: Catch exceptions in mutatedAttributesCallback (ampproject#8383) Fixing custom scroll-snap on IOS (ampproject#8391) Add experiment for using AmpContext class in integration.js (ampproject#8348) add (ampproject#8349) swipe api (ampproject#8357) skip 3 flaky tests (ampproject#8388) amp-bind: Expression complexity limit (ampproject#8321) add margin-bottom (ampproject#8350) Flying carpet: make container full viewport and center content (ampproject#8292) Service Registration: Document Click (ampproject#7882) Add a8ad (ampproject#8036) ...
Closes #8097.
The flying carpet's container has to be full screen and center its content.