✨ Update amp-access-scroll #26810
Merged
✨ Update amp-access-scroll #26810
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
junoatwork
force-pushed
the
juno/dynamic
branch
2 times, most recently
from
2c6556b
to
8535f4f
Compare
This PR - adds a protocol version query param to requests made from amp-access-scroll for compatibility - adds a "holdback" flag and styles to prevent mismatched rendeirng with new layout - renames ScrollAudio to ScrollSheet - adds ability to dynamically size horizontal layout for ScrollComponents. Special care was taken to ensure these properties cannot interrupt vertical browser layout; only width, left, and right are allowed. Dynamic sizing is necessary to pervent invisible iframes being drawn over usable parts of the underlying page, which would cause UX and accessibility problems in certain layout scenarios. Dynamic sizing solves this by shrinking the iframe to fit the size neccessary to fit the rendered controls.
junoatwork
force-pushed
the
juno/dynamic
branch
from
8535f4f
to
7ec8d4b
Compare
junoatwork
force-pushed
the
juno/dynamic
branch
from
b32000b
to
288854d
Compare
junoatwork
force-pushed
the
juno/dynamic
branch
from
72d0271
to
d0c42b5
Compare
dbow
reviewed
Feb 14, 2020
| width: 100%; | ||
| } | ||
|
|
||
| .amp-access-scroll-sheet.amp-access-scroll-holdback { | ||
| position: fixed; |
There was a problem hiding this comment.
seems like these styles are unnecessary with the .amp-access-scroll-sheet classname providing them too?
dbow
reviewed
Feb 14, 2020
| bottom: 63px; | ||
| height: 56px; | ||
| left: auto; | ||
| right: 16px; | ||
| width: 475px; | ||
| } | ||
| } | ||
|
|
||
| .amp-access-scroll-sheet { |
There was a problem hiding this comment.
could you put this classname above the modifications to it?
|
To clarify, the holdback code / css classes will be removed in a future PR. |
kushal
approved these changes
Feb 14, 2020
junoatwork
force-pushed
the
juno/dynamic
branch
from
2ae3590
to
b6dee79
Compare
dvoytenko
approved these changes
Feb 21, 2020
|
LGTM from me. Ready to be merged? |
|
@dvoytenko yep, thanks! |
|
Yup! |
|
/cc @jpettitt |
robinvanopstal
added a commit
to jungvonmatt/amphtml
that referenced
this pull request
Feb 24, 2020
* master: (41 commits) custom-element: Minor test improvements (ampproject#26923) amp-pixel: Minor test improvements (ampproject#26918) viewer: Minor test improvements (ampproject#26906) dom: Minor test improvements (ampproject#26913) amp-action: Support whitelist lookup in AmpDocShadow (ampproject#26684) ✨ Update amp-access-scroll (ampproject#26810) 🚀 Remove doc css and base css from ESM build (ampproject#26889) 📖 [amp-story-player] Initial docs (ampproject#26606) Amp consent restrict fullscreen prod flag (ampproject#26909) 📖 Clarify SXG duration minimum (ampproject#26890) Improve test vendor requests macros (ampproject#26828) 🚀 Move scroll left and top macros out of url-replacement-impl (ampproject#25594) Update consent string maximum size to 200 bytes (ampproject#26741) ✨[amp-story-player] Adds tap-to-next/previous story (ampproject#26865) update owners file with correct syntax (ampproject#26899) amp-sticky-ad: Fix unit test (ampproject#26855) Add performance metrics to README (ampproject#26891) 🐛 Bug fix: check links test (ampproject#26739) ✨Idealmedia uniq ad (ampproject#25838) 📦 Update dependency jsdom to v16.2.0 (ampproject#26591) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR
was taken to ensure these properties cannot interrupt vertical browser layout;
only width, left, and right are allowed.
Dynamic sizing is necessary to pervent invisible iframes being drawn over usable
parts of the underlying page, which would cause UX and accessibility problems in
certain layout scenarios. Dynamic sizing solves this by shrinking the iframe to fit
the size neccessary to fit the rendered controls.