amp-bind: Support bindings in fixed layer #16130
Conversation
|
/to @jridgewell For review of fixed layer. Will add tests if you think this approach is ok. |
extensions/amp-bind/0.1/bind-impl.js
Outdated
| return this.initialize_(body, head && elementByTag(head, 'title')); | ||
| const title = head && elementByTag(head, 'title'); | ||
| const fixedLayer = this.viewport_.getFixedLayerContainer(); | ||
| return this.initialize_(body, fixedLayer, title); |
There was a problem hiding this comment.
Why not use document as the root, instead of body? Then you don't need to be aware of the fixed layer.
There was a problem hiding this comment.
I wanted to avoid scanning the head unnecessarily, but this is a lot cleaner. Thanks.
dreamofabear
force-pushed
the
bind-fixed-layer
branch
from
e544782
to
74571da
Compare
src/amp-events.js
Outdated
| // Values start at 801 for easier code searchability and to avoid conflation | ||
| // with HTTP status codes. | ||
| DOM_UPDATE: 801, | ||
| // The following codes are only used for testing. |
There was a problem hiding this comment.
I know we depend on the built signal for owner elements.
src/amp-events.js
Outdated
| ERROR: 'amp:error', | ||
| // Values start at 801 for easier code searchability and to avoid conflation | ||
| // with HTTP status codes. | ||
| DOM_UPDATE: 801, |
There was a problem hiding this comment.
Why are we changing to numbers?
There was a problem hiding this comment.
Travis build failed due to bundle-size. This saves 0.03KB. 😄 We'll end up doing the same with error messages which should save a bunch more.
Had to change back to strings to avoid conversion for event type.
There was a problem hiding this comment.
Actually realized FixedLayer doesn't need to dispatch DOM_UPDATE (at least for amp-bind) since it doesn't add new elements but just moves them on/off the transfer layer.
Still would be nice to keep this change for a small size reduction.
src/amp-events.js
Outdated
| LOAD_START: 'e5', | ||
| LOAD_END: 'e6', | ||
| ERROR: 'e7', | ||
| VISIBILITY_CHANGE: 'e8', |
There was a problem hiding this comment.
I only saw usage in test code but upon closer inspection it's actually externally visible. 🤦♂️
Additionally, one can observe the amp:visibilitychange on the window object to be notified about changes in visibility.
https://github.com/ampproject/amphtml/blob/master/ads/README.md#page-visibility
Updated the unit test to use the string literal to catch this.
src/amp-events.js
Outdated
| ERROR: 'amp:error', | ||
| // Use short, unique strings to reduce bundle size impact. | ||
| BUILT: 'e1', | ||
| DOM_UPDATE: 'e2', |
There was a problem hiding this comment.
Date picker, lightbox, list, forms, access, and FX uses this one.
src/amp-events.js
Outdated
| LOAD_END: 'amp:load:end', | ||
| ERROR: 'amp:error', | ||
| // Use short, unique strings to reduce bundle size impact. | ||
| BUILT: 'e1', |
There was a problem hiding this comment.
I'm not sure 30b is worth changing these, especially once we strip out the testing ones. Error messages are a special case because they are usually very long messages, these aren't.
There was a problem hiding this comment.
Fair enough, don't feel strongly enough to fight ya. 😉
|
@jridgewell Ping. :) |
Fixes #14888.