🐛 [amp-image-slider] Conservative loading with tweak over hints #17477
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
|
/cc @nainar to verify behavior matches specs. |
cathyxz
approved these changes
Aug 14, 2018
| @@ -312,7 +312,7 @@ export class AmpImageSlider extends AMP.BaseElement { | |||
| const rightAmpImage = dev().assertElement(this.rightAmpImage_); | |||
| leftAmpImage.signals().whenSignal(CommonSignals.LOAD_END).then(() => { | |||
| if (leftAmpImage.childElementCount > 0) { | |||
| const img = leftAmpImage.firstChild; | |||
|
@kevinkassimo looks good to me as per the spec in the issue. (Could you link the issue here please?) However want to allow discussion on the main issue to conclude. Give me a bit to loop back on this? |
|
@nainar Sure |
|
Let's land this. The product is experimental we can always revert. |
1 task
Enriqe
pushed a commit
to Enriqe/amphtml
that referenced
this pull request
Nov 28, 2018
…roject#17477) * Use visibility trick on container to hide before images complete loading * Fix false DOM order assumption .firstChild causes issues for SSR * Hide hint until amp-img finished loading * Add fallback example
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.
Per #17446
1st slider below loads correctly.
2nd fails and toggles
amp-imgs' own fallbacks.Caveat:
layeris not enabled, three dots would not showlayeris not enabled, if amp-imgs' fallback are also amp-imgs or other amp components, they would not show (due to slider taking the ownership)