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

[SwipeActions] Limit overscrolling to the right #270

Merged
merged 4 commits into from
Mar 2, 2021

Conversation

nononoah
Copy link
Collaborator

@nononoah nononoah commented Mar 1, 2021

Summary

Though Listable does not support actions beneath the left of the cell, it is currently possible to swipe the contents of a cell all the way to the right. This leads to unexpected and undesirable visual state.

This PR caps overscroll to the right to 20% of the width of the cell. As a result, we maintain a certain bounciness, but don't allow for wilder translations.

Demo

Before
before
https://user-images.githubusercontent.com/1542313/109521585-78221e00-7a7b-11eb-846c-c04962cf200d.mov

After
after
https://user-images.githubusercontent.com/1542313/109521619-7fe1c280-7a7b-11eb-8676-d7b00faf1e4c.mov

@kyleve kyleve requested a review from kylebshr March 1, 2021 16:45
@kyleve
Copy link
Collaborator

kyleve commented Mar 1, 2021

Mind adding a note to the changelog in the root of the repo as well?

@nononoah
Copy link
Collaborator Author

nononoah commented Mar 1, 2021

Mind adding a note to the changelog in the root of the repo as well?

Added!

@@ -68,7 +68,8 @@ extension ItemCell {
case .swiping:

let translation = configuration.panGestureRecognizer.translation(in: self)
xOriginOffset = contentView.frame.origin.x + translation.x
// No actions exist to the left, so limit overscrolling to the right to 20% of the width.
xOriginOffset = min(bounds.width / 5.0, contentView.frame.origin.x + translation.x)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can I suggest an additional change that doesn't allow you start a swipe in this direction, which mimics the built-in swipe actions? (This should stay since you can still swipe this was after starting a swipe). Happy to PR that change myself, or walk you through what I have in mind

Copy link
Collaborator Author

@nononoah nononoah Mar 2, 2021

Choose a reason for hiding this comment

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

Thanks for the suggestion,@kylebshr. It definitely improves the feel of the cell.

I took a shot at implementing this, and arrived at the following:

case .began, .changed:

    if swipeState == .closed && velocity > 0 {
        // The cell is closed and this is a swipe to the right. Ignore the swipe.
        sender.setTranslation(.zero, in: self)
    } else {
        let swipeState = SwipeActionState.swiping(willPerformAction: willPerformAction)
        set(state: swipeState)
    }

sender.setTranslation(.zero, in: self) is important so that we don't "jump" when the swipe that begins to the right starts moving back to the left.

If you have other ideas or would like the logic spelled differently (e.g. case .began, .changed where ...), please let me know and I'll update before merging.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My one thought is that those could perhaps be implemented in the gesture recognizer itself (HorizontalGestureRecognizer.swift I think?), in much the same way that it ignores vertical swipes. That might clean up the logic here, since the gesture will cancel and never start if the velocity is incorrect.

@nononoah nononoah force-pushed the nononoah/swipe-translation branch from e7d504e to 187c6fd Compare March 2, 2021 14:15
@nononoah nononoah force-pushed the nononoah/swipe-translation branch from 187c6fd to a15cb1c Compare March 2, 2021 14:17
Copy link
Collaborator

@kylebshr kylebshr left a comment

Choose a reason for hiding this comment

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

Awesome, thanks! We'll probably need to update this eventually when we add support for trailing and/or leading actions, but we can tackle that when we get to it!

@kylebshr kylebshr merged commit 39cb85d into square:main Mar 2, 2021
kyleve added a commit that referenced this pull request Mar 3, 2021
…-action

* origin/main:
  [SwipeActions] Limit overscrolling to the right (#270)
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.

3 participants