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

Update smoothscroll detect logic #944

Merged
merged 2 commits into from
Dec 21, 2020

Conversation

bricksroo
Copy link
Contributor

I noticed the smoothscroll polyfill wasn't patching properly in Safari when using the polyfill-service. The first part of the detect logic returns false (indicating the need for the polyfill), while the second returns true. However, when wrapping the detect logic with !() for the ?flags=gated case, it evaluates to false due to the || operator.

Screen Shot 2020-12-15 at 11 16 06 PM

This PR updates the logic from || to &&.

Please let me know if there is any more info I can provide or things I forgot to address with this PR.

@bricksroo bricksroo requested a review from a team as a code owner December 16, 2020 07:33
@CLAassistant
Copy link

CLAassistant commented Dec 16, 2020

CLA assistant check
All committers have signed the CLA.

@origamiserviceuser origamiserviceuser added this to incoming in Origami ✨ Dec 16, 2020
@github-actions github-actions bot added the library Relates to an Origami library label Dec 16, 2020
@JakeChampion JakeChampion self-requested a review December 16, 2020 10:57
@JakeChampion
Copy link
Owner

@bricksroo If the second part of the detect is returning true that means that Safari supports scroll-behaviors. Which version of Safari are you referring to by the way?

We have a long thread in a previous pull-request about why this uses an or operator (||), it is to avoid unnecessarily making the page scroll for browsers which do support scroll-behaviors -- #903

@bricksroo
Copy link
Contributor Author

bricksroo commented Dec 16, 2020

@JakeChampion This is in the latest version of safari on macOS.
Screen Shot 2020-12-16 at 9 08 38 AM

After a bit more testing, it seems that safari doesn't catch when trying the scrollTo(scrollOptions) with behavior: 'smooth'. It successfully scrolls the element, however without the actual "smooth" behavior. Safari seems to essentially ignore the behavior value of scrollOptions. Here is MDN's live example for ScrollToOptions.

Dec-16-2020 09-46-05

@JakeChampion JakeChampion enabled auto-merge (rebase) December 21, 2020 15:18
@JakeChampion JakeChampion moved this from incoming to active in Origami ✨ Dec 21, 2020
@JakeChampion JakeChampion merged commit 748b6a6 into JakeChampion:master Dec 21, 2020
Origami ✨ automation moved this from active to complete Dec 21, 2020
@bricksroo
Copy link
Contributor Author

Thank you for taking a look. Appreciate you!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 22, 2021
@robertboulton robertboulton removed this from Done in Origami ✨ Jul 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
library Relates to an Origami library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants