-
-
Notifications
You must be signed in to change notification settings - Fork 167
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
Add the ability to configure swipe gestures #497
Conversation
@aeharding can you take a look at this and let me know your initial thoughts? |
Just a cursory glance at the code, looks good from a high level. I won't be able to review until this weekend though. Thanks! (Sorry, accidentally tapped ready for review) |
Thanks! |
6e76902
to
99ee634
Compare
Okay, so I just had some time to check this out. A couple thoughts on the UI:
|
With the none option you can disable only short or long swipes, allowing you to have a single action for each direction at either the short or long swipe trigger point. This would be useful for people who want a couple of swipe actions (perhaps upvote and downvote) but want to make it harder to accidentally trigger them (by only using long swipes). While it takes a little more effort to disable swipes (I can add a disable all option if you want), it allows for a lot more configurability.
I will fix these soon. |
That's what the "long swipe trigger point" option does (see second screen shot) |
How does that option work? I don't understand which swipes it disables. |
It doesn't disable any option, it just requires you to swipe further before changing to the second swipe option |
My point was that by changing individual things to None, you can set a single action for either direction, and you can choose whether you want it at the short swipe or long swipe distance. I for one would prefer not having a short swipe on comments because I accidentally trigger it sometimes, especially on the left when trying to go back. |
I understand, however I'd rather add an option to disable short swipe left universally. Otherwise you could have inconsistent behavior on inbox items vs posts vs comments. |
Looks great! |
Future work is allowing users to disable the back gesture on iOS. Note, I did leave none as an option so far, because I would really like to have that choice; given that it's in a pretty deep menu I imagine it won't bother anyone who doesn't choose it. |
*/ | ||
Total: "total", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this a bad merge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I realized the sorting was strange, not its least verbose -> most verbose when you look top to bottom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand, to me it makes sense to have default be at the top in general.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean that now it's in the order: no information (none) -> some information (only the total) -> all information (separate scores). The default is the middle, and you can choose go up or down a level to see less or more. I can change it if you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's revert this for now.
I'm experiencing some strange behavior:
Can you replicate? It's not a huge bug or anything just strange behabior |
I'm wondering if we should change hide post to bookmark as the default far right swipe item. That would match Apollo and also would avoid confusing new users when they accidentally swipe away a post and lose it. What do you think? |
There's still more to be done, but I wanted to push most of what I have. |
553969f
to
bef1818
Compare
It should already be like that, see the diff from my force push last night https://github.com/aeharding/voyager/compare/553969f2b5245bdab822ee36d4a0d444c20080e4..bef18183ce489791ad9350d9fb60968e1d6669d4. Is it not for you? |
About the bug you mentioned, I can replicate it but I have not been able to figure out why it happens. |
@aeharding can you take a look at #202? I would like to add share as a swipe option. Otherwise if this gets merged first I can add making it a swipe option to that PR. |
Let's do share in a separate PR so this PR isn't blocked |
6b6c726
to
4595c7a
Compare
4595c7a
to
03d4213
Compare
Change to centralized sliding item action creation based on a dictionary passed into BaseSlidingVote.
- Add swipe icons to show what each setting means - Add icons on swipe menu options - Add switches to completely turn off direction in one shot
03d4213
to
1df1fdf
Compare
Most recent push was to update to the latest main, I wanted to ensure there were no problems from 9154e0c since it sounded like it might touch something related to this. |
I'm still a bit concerned about perf, but I haven't been able to get any conclusive results on perf changes between this and main. @rsammelson Can you change the default far right swipe for posts from hide post to bookmark? I think this is getting pretty close now. |
1df1fdf
to
e029ffe
Compare
e029ffe
to
c0cae62
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, this is quite the enhancement.
Thanks! |
Fixes #64