Skip to content
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

AMP and Best Practices #2819

Closed
photocoder opened this issue Aug 1, 2017 · 4 comments
Closed

AMP and Best Practices #2819

photocoder opened this issue Aug 1, 2017 · 4 comments

Comments

@photocoder
Copy link

Hello everybody,

I use AMP pages on my website, but after checking by Lighthouse I see in section "Best Practices":
Uses passive listeners to improve scrolling performance:

https://cdn.ampproject.org/v0/amp-carousel-0.1.js | line: 31
https://cdn.ampproject.org/v0/amp-carousel-0.1.js | line: 22

Could somebody tell me "why"? :) I can't control that scripts and I think teams Google products should to "synchronising" between each other...

@brendankenny
Copy link
Member

Sounds like an issue to file on https://github.com/ampproject/amphtml/ :)

More seriously, "best practices" are just that: ideal ways of setting things up, but sometimes reality gets in the way of doing the "best" thing. Maybe the performance impact is negligible, so no one has bothered to make the change yet. Maybe there's some reason those handlers need to not be passive (some functionality or some corner case of build tools or some issue with older browsers), so they're stuck like that.

Ideally Lighthouse would be able to measure the performance impact that not using passive scroll handers incurs, but it's not there yet (that's why the audit is in "Best Practices" and not "Performance" yet). In the meantime, it's worth keeping in mind the separation between Lighthouse score and that actual measurable performance. It may end up being worth living with a slightly lower Lighthouse score in order to get all the advantages that AMP can bring, especially if the scroll handlers have minimal impact.

Looks like there has been some discussion around this in AMP, so you may want to do some more investigating there:
ampproject/amphtml#4820
ampproject/amphtml#4895
ampproject/amphtml#5109

@stefany-newman
Copy link

Is that going to be fixed any time soon ?

@connorjclark
Copy link
Collaborator

Kinda looks like amp already fixed this here: ampproject/amphtml#10721

Please provide more details if that is not the case. If the issue still exists, what @brendankenny said is still relevant.

@stefany-newman
Copy link

@connorjclark Lighthouse still spits out the same error :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants