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

fix(a11y): allow pinch to zoom #1515

Merged
merged 1 commit into from
Jan 30, 2023
Merged

fix(a11y): allow pinch to zoom #1515

merged 1 commit into from
Jan 30, 2023

Conversation

userquin
Copy link
Member

@userquin userquin commented Jan 29, 2023

This PR is related to #1501

/cc @ronilaukkarinen can you test pinch to zoom in this PR preview? Your fix not working on my Android device, there is some pan x/y in body styles that prevents it.

https://streamable.com/y6cnyy

@netlify
Copy link

netlify bot commented Jan 29, 2023

Deploy Preview for elk-docs canceled.

Name Link
🔨 Latest commit eb42d7a
🔍 Latest deploy log https://app.netlify.com/sites/elk-docs/deploys/63d65b3224c64a00085c75d0

@stackblitz
Copy link

stackblitz bot commented Jan 29, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@netlify
Copy link

netlify bot commented Jan 29, 2023

Deploy Preview for elk-zone ready!

Name Link
🔨 Latest commit eb42d7a
🔍 Latest deploy log https://app.netlify.com/sites/elk-zone/deploys/63d65b326391f90008aa7116
😎 Deploy Preview https://deploy-preview-1515--elk-zone.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@ronilaukkarinen
Copy link
Contributor

This PR is related to #1501

/cc @ronilaukkarinen can you test pinch to zoom in this PR preview? Your fix not working on my Android device, there is some pan x/y in body styles that prevents it.

https://streamable.com/y6cnyy

Maybe I missed something, I'll try it asap.

@ronilaukkarinen
Copy link
Contributor

@userquin Fetched and pulled this PR, tested on a real device (OnePlus 8T with Google Pixel Android 13).

It works: https://user-images.githubusercontent.com/1534150/215325700-cd23ea4c-2875-4469-8d97-ea4e83406593.mp4

Sorry for your trouble! I completely forgot touch-action: pan-x pan-y; from the commit...

@userquin
Copy link
Member Author

userquin commented Jan 29, 2023

@userquin Fetched and pulled this PR, tested on a real device (OnePlus 8T with Google Pixel Android 13).

Next time just test it using preview link above, you don't need to test in your local ;) Deploy Preview link

It works: https://user-images.githubusercontent.com/1534150/215325700-cd23ea4c-2875-4469-8d97-ea4e83406593.mp4

Sorry for your trouble! I completely forgot touch-action: pan-x pan-y; from the commit...

no problem, I didn't test it in your PR 😅

@ronilaukkarinen
Copy link
Contributor

@userquin Fetched and pulled this PR, tested on a real device (OnePlus 8T with Google Pixel Android 13).

Next time just test it using preview link above, you don't need to test in your local ;) Deploy Preview link

🤦‍♂️ Stupid me... so used to manual testing and often times running way too many projects at the same time so tend to overlook things. Elk has really fancy CI and deploy-previews I have not seen before, congrats for the well built workflow!

Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

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

Works on iOS PWA

I discussed on Discord with @userquin that he implements an opt-out setting later in a separate PR,
so that user can decide if they want this behavior or not

e.g. Discord or Twitter does not behave like that in App-Mode

@ronilaukkarinen
Copy link
Contributor

I discussed on Discord with @userquin that he implements an opt-out setting later in a separate PR,
so that user can decide if they want this behavior or not

It's a good thing to give the users more options but I think it should not disabled by default as it's an accessibility feature unless it's clear that it can be enabled in the settings, this meaning that the Accessibility related settings should be in their own category in the Settings, for example under it would be the font-size and zoom settings, leaving only color and possibly other UI related future settings under the Interface settings.

e.g. Discord or Twitter does not behave like that in App-Mode

That has been on of their biggest downsides and cannot be changed because of the big corporative decisions behind them. This is not by far the only thing wrong on those platforms accessibility-wise. But on the other note, they also have actual apps on mobile that usually don't have zoom enabled. Yet I loathe apon the idea. Instagram has zoom enabled on the feed view only on images, not on text, better than nothing I guess. Zoom should always be universally enabled and allowed by default. Many don't get that unless their eyesight goes bad.

@Shinigami92
Copy link
Member

@ronilaukkarinen yes, yes, sure be default its on, that's why I said opt-out, not opt-in

@ronilaukkarinen
Copy link
Contributor

@ronilaukkarinen yes, yes, sure be default its on, that's why I said opt-out, not opt-in

My brain picked it out it the wrong way 😄 Just wanted to underline the issue 👍
Keep up the good work!

@danielroe
Copy link
Member

At the moment I can't seem to pinch-to-zoom into images/media, only into arbitrary app elements.

@userquin
Copy link
Member Author

At the moment I can't seem to pinch-to-zoom into images/media, only into arbitrary app elements.

This PR is about accesibility, allow pinch to zoom on mobile: check #1501

@userquin
Copy link
Member Author

@ronilaukkarinen @Shinigami92 PR for setting here #1517

@danielroe danielroe merged commit 80a4ec5 into main Jan 30, 2023
@danielroe danielroe deleted the userquin/fix-prevent-zoom branch January 30, 2023 13:32
antfu pushed a commit that referenced this pull request Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants