-
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
Add Element Collapse API #3721
Add Element Collapse API #3721
Conversation
036c684
to
d4fe68e
Compare
* Collapses the element, and notifies its owner (if there is one) that the | ||
* element is no longer present. | ||
*/ | ||
collapse() { |
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 restrict access to this method via presubmits?
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.
Do you mean that it has to be marked /*OK*/
and /*REVIEW*/
?
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.
Yes.
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.
Done.
PTAL. |
ad3446b
to
d827453
Compare
@@ -100,11 +119,45 @@ class AmpFlyingCarpet extends AMP.BaseElement { | |||
this.assertPosition(); | |||
} catch (e) { | |||
// Collapse the element if the effect is broken by the viewport location. | |||
toggle(this.element, false); | |||
this./*REVIEW*/collapse(); |
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.
OK
LGTM |
e27ba4c
to
497a446
Compare
@jridgewell Sorry for the delay. This is LGTM |
68d432b
to
b5c55e8
Compare
* Fix flying carpet example * Add #collapse and #collapsedCallback * Use #collapse when collapsing AmpElement * Collapse Flying Carpet when all children collapse * Take ownership of the children * Fix imports * Keep track of childNodes, but own children * Tests * #collapse is not a protected API * Collapse a Flying Carpet if only whitespace is visible * OK #collapse * linting * Update presubmit * Update amp-ad * Add more docs
Adds the
AmpElement#collapse
API, which hides the element and notifies its owner that it has collapsed. Owner's may react to this by collapsing themselves, for example.Fixes #3338.