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

feat: allow users to disable swipe gesture on segment #22048

Closed
SansDK opened this issue Sep 8, 2020 · 17 comments · Fixed by #22087
Closed

feat: allow users to disable swipe gesture on segment #22048

SansDK opened this issue Sep 8, 2020 · 17 comments · Fixed by #22087
Labels
help wanted a good issue for the community package: core @ionic/core package type: feature request a new feature, enhancement, or improvement

Comments

@SansDK
Copy link

SansDK commented Sep 8, 2020

Feature Request

Ionic version:

[x] 5.x

Describe the Feature Request

Related to #21251.
This issue has been closed as the issue matched "the native iOS spec.", which is fine. But as @lincolnthree already mentioned in the comments on his issue, this behavior does not seem to match the native Android spec.

I feel like we can and should give Ionic users the option to choose wether they want the ionChange event to be triggered on this swipe gesture or not. It's basically already disabled in the scrollable=true mode, so the code is already there (I guess). The problem with the scrollable mode is that it isn't always desirable because of the scroll bar.

If we were to add this feature, I think we should default to the current behavior for backwards compatibility.

Describe Preferred Solution

In the ion-segment component, there should be an option changeOnSwipe or triggerChangeOnSwipe (or something similar like that).

The default value should be true, to keep it the same as it is now.

When this is set to false, the behavior seen in #21251 should not trigger an ionChange event. Also, I think swiping between different ion-segment-buttons should be disabled in this case.

Describe Alternatives

I tried using the scrollable attribute, and tried hiding the scroll bar, but this is a bad and hacky "solution".

Related Code

Additional Context

@ionitron-bot ionitron-bot bot added the triage label Sep 8, 2020
@lincolnthree
Copy link

+1

@liamdebeasi
Copy link
Contributor

Thanks for the feature request. Which app are you testing this with on Android? I am testing on the phone app on Android and the swipe behavior matches iOS.

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Sep 8, 2020
@ionitron-bot ionitron-bot bot removed the triage label Sep 8, 2020
@lincolnthree
Copy link

The default Google dialer app comes to mind (Pixel 2 XL), so possibly a version difference?

Facebook Messenger Lite. Google Analytics. Google Photos app.

None of these apps allow swiping segments/tabs on my device..

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Sep 8, 2020
@liamdebeasi
Copy link
Contributor

Hmm can you provide a video of what you mean? I do not see these on my end.

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Sep 8, 2020
@ionitron-bot ionitron-bot bot removed the triage label Sep 8, 2020
@SansDK
Copy link
Author

SansDK commented Sep 9, 2020

Not a whole lot apps use this kind of component on Android it seems. I can't find many good examples, but Facebook and Google Play Store could be seen as some.

I use the ion-segment a bit like how Facebook uses it, but initially mine is in the middle of the view (and then sticks to the top when scrolling down). This makes scrolling a bit difficult, because if you start the scroll on one of the segments and later let go, it will change the whole view instead of just scrolling the current view. My ion-segment is quite tall, so this is pretty easy to do, especially on smaller devices. For this case it would be very nice if we could disable the swipe triggering the ionChange.

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Sep 9, 2020
@lincolnthree
Copy link

@SansDK That's exactly my use-case. A sticky segment that scrolls with content until it hits the top and stays there. (Not a trivial feature, I might add, with how buggy position:sticky is on iOS.

I'll try to upload a video if I can. I need to find another device that can make a recording (Unless you can recommend some decent screen recorder software. Maybe I can do this via remote debugging...)

@liamdebeasi
Copy link
Contributor

Oh I see, so you would like to disable the swipe gesture and just use the segment by clicking? I think I may have misunderstood the original request.

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Sep 9, 2020
@ionitron-bot ionitron-bot bot removed the triage label Sep 9, 2020
@lincolnthree
Copy link

Yes, I think that would be enough to solve the problem. Exactly!

If there are some apps that DO use swipe to change on Android, then it makes sense to keep that functionality, and the ability to disable the feature would be enough to prevent spurious swipes in the vertical direction from actually changing segments.

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Sep 9, 2020
@liamdebeasi
Copy link
Contributor

Ok makes sense, I think this is something we can easily add.

@liamdebeasi liamdebeasi added package: core @ionic/core package type: feature request a new feature, enhancement, or improvement labels Sep 9, 2020
@ionitron-bot ionitron-bot bot removed the triage label Sep 9, 2020
@liamdebeasi liamdebeasi changed the title feat: disabling ionChange trigger while swiping on ion-segment component feat: allow users to disable swipe gesture on segment Sep 9, 2020
@lincolnthree
Copy link

lincolnthree commented Sep 9, 2020

Here's a video I recorded of the issue with scrolling on a sticky ion-segment, now that you already understand what's going on, hehe 👍

Notice how it scrolls with the content, then also notice how it SNAPS to "Ideas" after scrolling where the touch gesture starts on the segment:

https://media.giphy.com/media/TGL6V1ndLPJyEzFbjV/source.mov

@lincolnthree
Copy link

Also, awesome. Thank you. Glad this should be doable!

@TakumaKira
Copy link
Contributor

Hi there!
I'd like to submit a pull request for this!

@liamdebeasi
Copy link
Contributor

Thank you! Here is some getting started info on creating a PR: https://github.com/ionic-team/ionic-framework/blob/master/.github/CONTRIBUTING.md#creating-a-pull-request

One thing to be careful of is just disabling the gesture will also disable the slide animation of the indicator from one segment button to the next. Maybe @brandyscarney has some thoughts on how we can get around this?

@liamdebeasi liamdebeasi added the help wanted a good issue for the community label Sep 14, 2020
@ionitron-bot
Copy link

ionitron-bot bot commented Sep 14, 2020

This issue has been labeled as help wanted. This label is added to issues that we believe would be good for contributors.

If you'd like to work on this issue, please comment here letting us know that you would like to submit a pull request for it. This helps us to keep track of the pull request and make sure there isn't duplicated effort.

For a guide on how to create a pull request and test this project locally to see your changes, see our contributing documentation.

Thank you!

@TakumaKira
Copy link
Contributor

One thing to be careful of is just disabling the gesture will also disable the slide animation of the indicator from one segment button to the next. Maybe @brandyscarney has some thoughts on how we can get around this?

Thank you for letting me know, @liamdebeasi.

I allowed running checkButton method inside onClick when changeOnSwipe === false to enable the slide animation.
On top of that, I noticed this change also causes breaking the behavior of the indicator animation when swiped with changeOnSwipe === false(and have broken when scrollable === true as well), so I terminated onClick if current.tagName === 'ION-SEGMENT'.

I will soon submit the PR, including the changes above.

TakumaKira pushed a commit to TakumaKira/ionic-framework that referenced this issue Sep 14, 2020
TakumaKira pushed a commit to TakumaKira/ionic-framework that referenced this issue Sep 15, 2020
TakumaKira pushed a commit to TakumaKira/ionic-framework that referenced this issue Sep 16, 2020
TakumaKira pushed a commit to TakumaKira/ionic-framework that referenced this issue Nov 13, 2020
liamdebeasi pushed a commit that referenced this issue Nov 13, 2020
@liamdebeasi
Copy link
Contributor

Thanks for the issue. This has been resolved via #22087, and the feature will be available in an upcoming release of Ionic Framework.

@ionitron-bot
Copy link

ionitron-bot bot commented Dec 13, 2020

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Dec 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted a good issue for the community package: core @ionic/core package type: feature request a new feature, enhancement, or improvement
Projects
None yet
4 participants