Low-hanging fruit replacements for getLayoutWidth/Box uses and deprecate some measurement APIs #31490
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
samouri
reviewed
Dec 8, 2020
| @@ -321,7 +321,7 @@ export class AmpScrollableCarousel extends BaseCarousel { | |||
| * @private | |||
| */ | |||
| withinWindow_(pos, callback) { | |||
| const containerWidth = this.element.getLayoutWidth(); | |||
There was a problem hiding this comment.
Are we guaranteed that these sync measures won't cause reflows/jank? Shouldn't we still be relying on vysnc?
There was a problem hiding this comment.
Not in this case, because:
- There's more measuring code is peppered there, which negates any benefits.
- It's a component in the deprecation track.
| @@ -114,7 +114,7 @@ export class AmpVizVega extends AMP.BaseElement { | |||
|
|
|||
| /** @override */ | |||
| onLayoutMeasure() { | |||
| const box = this.getLayoutBox(); | |||
| const box = this./*OK*/ getBoundingClientRect(); | |||
There was a problem hiding this comment.
Same question here re. sync measurement
There was a problem hiding this comment.
Measurements in the onLayoutMeasure are technically free since they are called in a version of vsync.measure from resources-impl. But it's also a deprecated component.
samouri
approved these changes
Dec 8, 2020
ed-bird
pushed a commit
to ed-bird/amphtml
that referenced
this pull request
Dec 10, 2020
…ate some measurement APIs (ampproject#31490) * Low-hanging fruit replace getLayoutWidth/Box uses and deprecated measurement APIs * test fixes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Mostly replaced
getLayoutBox/getLayoutWidthwith a direct measurement where the old code is doing additional measurements anyway.Partial for #31540.