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

adds allowDirection parameter #41

Closed
wants to merge 6 commits into from
Closed

Conversation

maeddin
Copy link
Contributor

@maeddin maeddin commented Jan 12, 2022

Sorry for the commit chaos. There were problems with the first merge and I repeated that. I added the allowDirection parameter. With this you can now disallow directions independent of the allowVertical parameter. What is new here is that also the overlay can be built correctly as you can see in the following. Also you can still move the card in all directions unlike with allowVertical.

Before:

old_example

After:

example_horizontal example_vertical

@HeavenOSK HeavenOSK self-requested a review January 13, 2022 14:31
@HeavenOSK
Copy link
Owner

@MaddinMade
Thanks for making this PR!

I understand your point from this PR. And I agree with you that overlay should work well even if some directions are rejected. So I also want to fix the current behavior of SwipableStack.

However, I feel there is a better solution than this change. For this PR, I thought about the following:

  • It seems counter-intuitive that the name AllowDirection is used, but the priority is determined internally.
  • When we pass the function that always returns false, the priority logic doesn't work.

Your point is so good but I feel this solution is not the best.

@HeavenOSK
Copy link
Owner

Now, you can change the behavior of overlay by adjusting verticalSwipeThreshold, and horizontalSwipeThreshold. However, this is not easy to understand, and I think there is a better way 🤔

@maeddin
Copy link
Contributor Author

maeddin commented Jan 13, 2022

@HeavenOSK

Now, you can change the behavior of overlay by adjusting verticalSwipeThreshold, and horizontalSwipeThreshold.

You can change thresholds, but you can only control vertical and horizontal. For example, you cannot allow swiping up and forbid swiping down. In addition, this currently works only limited well, because the maximum value is 1, which of course can be adjusted. I'm not as well versed in the package as you, but I couldn't think of a good way to add this functionality without another big breaking change.

When we pass the function that always returns false, the priority logic doesn't work.

In this case, the logic should actually behave the same as with ()=>true, if I haven't missed anything.

It seems counter-intuitive that the name AllowDirection is used, but the priority is determined internally.

How about prioritizeDirection?

@maeddin
Copy link
Contributor Author

maeddin commented Jan 13, 2022

How about a function where you are passed two directions in the correct order and have to return the one you want?
Or a function where you have to return numbers instead of booleans for a proper priority system. But both would not be user friendly.

@maeddin
Copy link
Contributor Author

maeddin commented Jan 13, 2022

In general, I would regulate it in some way with a function, because it allows you to distinguish between different indices. For other attributes, you can of course use the controller, since it's always about the current card anyway.
At first I wanted to handle it with onWillMoveNext, but there would be a breaking change, since actions are currently performed here that should only be performed once per card.

@HeavenOSK
Copy link
Owner

@MaddinMade
Sorry for late response.

How about using a whitelist in the swipe direction? I feel it's easier to understand for users. I think the priority concept is too complex for UI library.

I make a draft PR. Could you review this PR please?

#43

@maeddin
Copy link
Contributor Author

maeddin commented Jan 17, 2022

#43 is the overall better solution to this problem.

@maeddin maeddin closed this Jan 17, 2022
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.

None yet

2 participants