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

Enable CSS scroll snap code across more browsers #17058

Merged
merged 5 commits into from
Aug 3, 2018

Conversation

nainar
Copy link
Contributor

@nainar nainar commented Jul 24, 2018

Safari/Firefox/Edge have scroll-snap enabled. Chrome now has it enabled starting 69.

This PR enables scroll snap code for all browsers/OSes.

Addresses: #16508

@codecov-io
Copy link

Codecov Report

Merging #17058 into master will decrease coverage by 1.37%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master   #17058      +/-   ##
==========================================
- Coverage   77.88%    76.5%   -1.38%     
==========================================
  Files         562      563       +1     
  Lines       41156    43863    +2707     
==========================================
+ Hits        32054    33558    +1504     
- Misses       9102    10305    +1203
Flag Coverage Δ
#integration_tests 36.13% <ø> (-0.05%) ⬇️
#unit_tests 76.44% <0%> (-0.49%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c3e0e0...8fcb75d. Read the comment docs.

@cathyxz
Copy link
Contributor

cathyxz commented Aug 2, 2018

This change looks fine, but will probably require very extensive testing on all relevant devices / browsers to see if it breaks anything. Do you mind commenting with a checklist here (or just assigning the PR to me) after that's been done?

@nainar
Copy link
Contributor Author

nainar commented Aug 2, 2018

Tracking manual testing on the following platforms:

  • iOS + Safari
  • iOS + Chrome
  • iOS + Firefox
  • iOS + Edge
  • Android + Opera
  • Android + Chrome
  • Android + Firefox
  • Android + Edge

@nainar
Copy link
Contributor Author

nainar commented Aug 2, 2018

Logging offline discussion:

Turning this into an experiment with following movement:

  • 2 weeks: 1% canary
  • 2 weeks: 10% prod
  • 2 weeks: 50% prod
  • 4 weeks: 100% prod
  • TURN ON FULL! 🎆

@cathyxz take a look?

@cathyxz
Copy link
Contributor

cathyxz commented Aug 2, 2018

I think we made a new rule banning creating experiments and flipping flags in the same PR. It makes for easier reverts.

Copy link
Contributor

@cathyxz cathyxz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just move the flag flip to a different PR.

@nainar
Copy link
Contributor Author

nainar commented Aug 3, 2018

@cathyxz - Split up the canary-config change into it's own PR.

@nainar nainar merged commit fd9ec1d into ampproject:master Aug 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants