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
amp-lightbox: Better resource scheduling for scrollable lightbox #12751
Conversation
@erwinmombay Can we wait to get this into Canary if not cut yet? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one question. Can we follow up with tests?
// Check whether child element is visible in the lightbox given | ||
const descendants = this.getComponentDescendants_(); | ||
for (let i = 0; i < descendants.length; i++) { | ||
const child = descendants[i]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: s/child/descendant
const child = descendants[i]; | ||
let offsetTop = 0; | ||
for (let n = child; n && this.element.contains(n); | ||
n = n./*OK*/offsetParent) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Formatting.
for (let n = child;
n && this.element.contains(n);
n = n./*OK*/offsetParent) {
offsetTop += n./*OK*/offsetTop;
}
|
||
this.element.addEventListener(AmpEvents.DOM_UPDATE, () => { | ||
this.takeOwnershipOfDescendants_(); | ||
this.updateChildrenInViewport_(this.pos_, this.pos_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means we won't call updateInViewport
for new children. WAI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would, on debounced scroll we also calculate visible children and call scheduleLayout and updateInViewport on them.
Fixes #12685
Temporary solution for high priority #12685. Layers will fix all the crazyness here.
Previous solution had the following bugs:
offsetTop
calculation were wrong and only worked when there was no otheroffsetParent
(Unfortunately all the problems above also exist for scrollable version of
amp-carousel
. Given layers is the right solution and there hasn't been a high priority need, I don't recommend porting these toamp-carousel
)