Schedule nested amp placeholder for managed scheduling #2954
Conversation
| @@ -1327,6 +1327,13 @@ export class Resources { | |||
| // Breadth-first search. | |||
| if (element.classList.contains('-amp-element')) { | |||
| callback(this.getResourceForElement(element)); | |||
| // Also schedule amp-element that is a placeholder for the element. | |||
| const placeholder = element.getPlaceholder(); | |||
| const hasAmpPlaceholder = (!element.classList.contains('-amp-layout') && | |||
There was a problem hiding this comment.
Can we move the -amp-layout check earlier, to avoid doing a DOM lookup for a placeholder that we may not need.
|
Code LGTM, but I don't know this code well enough to give final approval. @dvoytenko? |
|
Thanks @jridgewell assigned for @dvoytenko to give the final LGTM |
| // Also schedule amp-element that is a placeholder for the element. | ||
| if (!element.classList.contains('-amp-layout')) { | ||
| const placeholder = element.getPlaceholder(); | ||
| if (placeholder && placeholder.classList.contains('-amp-element')) { |
There was a problem hiding this comment.
Almost. But I believe it should be:
if (placeholder) {
this.discoverResourcesForElement_(placeholder, callback);
}
There was a problem hiding this comment.
That makes sense no need to double check the recursive call will do that.
There was a problem hiding this comment.
Sorry, could you please rephrase this sentence?
There was a problem hiding this comment.
I misunderstood your comment initially but got it after you explained the required test part. Done.
|
Very close. Just one correction. PTAL and add a test for that nuance. |
|
Added the test PTAL |
| @@ -1327,6 +1327,13 @@ export class Resources { | |||
| // Breadth-first search. | |||
| if (element.classList.contains('-amp-element')) { | |||
| callback(this.getResourceForElement(element)); | |||
| // Also schedule amp-element that is a placeholder for the element. | |||
| if (!element.classList.contains('-amp-layout')) { | |||
There was a problem hiding this comment.
Why if (!element.classList.contains('-amp-layout'))? Generally, elements themselves decide when placeholder is visible or not. It's typical that placeholder is hidden after layout, but not guaranteed. Otherwise we'd simply support it via global CSS. I think there's no risk of re-discovering placeholder here in all cases. If it's currently hidden - it won't be processed.
There was a problem hiding this comment.
I think it was just to avoid discovering it multiple times as you mentioned, but explanation sounds good to me! Dropped it.
|
@mkhatib One more comment. PTAL. |
|
Thanks @dvoytenko! Done |
|
LGTM! |
Fixes #2828
As @dvoytenko mentioned in the issue #2828 resources should only auto schedule the nested placeholders and not nested elements - these are still left to the owner element to schedule.
You can demo this on this deployment and see nested placeholder of
amp-animinsideamp-carouselbeing scheduled correctly.