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
Sync Lightbox with Carousel #13050
Sync Lightbox with Carousel #13050
Conversation
5f1dcaa
to
e700c75
Compare
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.
Overall looks good, but I want to bikeshed on the usage of hasOwnProperty
.
* mapping of the lightbox group id to the carousel element. | ||
* @private {!Object<string, !Element>} | ||
*/ | ||
this.lightboxSourceCarousels_ = {}; |
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 might need to be a src/object.js#map
to speed up lookups. map
is prototypeless so failed lookups happen faster. In the AMP codebase I see we generally prefer map
s and map[key]
notation vs {}
literals and hasOwnProperty
.
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.
@jridgewell any guidance?
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.
Discussed this and yes you're right. This should happen for security reasons too, not just performance.
Actually I realized that I forgot to handle a pretty major edge case here, which is making sure that sync-ing still behaves properly when we exclude items from the lightbox. Going to add logic for that. |
267f996
to
9249058
Compare
9249058
to
d6daf5d
Compare
* Sync slides to lightbox * Only exit if you're going back to your original element or you're a sync-ed carousel * Exclude scroll carousel * Add null check to shouldAnimate * Handle excluding elements from lightbox
* Sync slides to lightbox * Only exit if you're going back to your original element or you're a sync-ed carousel * Exclude scroll carousel * Add null check to shouldAnimate * Handle excluding elements from lightbox
* Sync slides to lightbox * Only exit if you're going back to your original element or you're a sync-ed carousel * Exclude scroll carousel * Add null check to shouldAnimate * Handle excluding elements from lightbox
Closes #12578 and #13052