-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Remove measure APIs from amp-auto-lightbox #33663
Remove measure APIs from amp-auto-lightbox #33663
Conversation
4004594
to
80e6783
Compare
* @param {!Element} element | ||
* @return {!Promise<!ClientRect>} | ||
*/ | ||
export function measure(ampdoc, element) { |
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.
Should we promote this to src/dom.js
?
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.
Sorry, was going to comment on that and forgot. @samouri has already created src/utils/intersection
and I can reuse that. The only nit is that the src/utils/intersection
uses an IntersectionObserver{root:document}
, which triggers the polyfill path in more cases than a simple {root:null}
that I do here. So I wasn't sure if it's worth reusing the existing util or rolling this as a single-use utility for the extension. LMK what you think.
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.
I'm okay with whichever way you decide:
- Add a TODO comment re. merging the helper with
measureIntersection
once iOS support for document as root improves - and/or promoting the helper to a utils area
I did both: promoted it to |
* Remove measure APIs from amp-auto-lightbox * fix presubmits * moved measurer to src/utils * lints
Partial for #31540.
Partial for #33678.
This blocks #33649.