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

Prevent scroll prop #240

Merged
merged 13 commits into from
Feb 21, 2022
Merged

Prevent scroll prop #240

merged 13 commits into from
Feb 21, 2022

Conversation

stefvhuynh
Copy link
Contributor

@stefvhuynh stefvhuynh commented Apr 2, 2021

  • Renames preventDefaultTouchmoveEvent to preventScrollOnSwipe
  • Makes all touch handlers passive by default except for touchmove when preventScrollOnSwipe is true
  • Removes and reattaches touch handlers when preventScrollOnSwipe value changes (fixes the main example)
  • Updates README

TODO before merge

  • Currently, this PR sets passive: true on touchstart and touchend. We need to decide what these should be and if we should introduce an eventOptions object to allow users to set these themselves.
  • I was not able to run tests locally. Jest would just hang when I execute the command, so test need to be updated before this merges.

@fivethreeo
Copy link

Any progress on this?

@hartzis
Copy link
Collaborator

hartzis commented Aug 16, 2021

Any progress on this?

It's on my mind 😸 . It will be a major version change so i'm seeing what else we can/should get in like #267

@jdesherlia
Copy link

@hartzis I am running into the need for this in our implementation, and the example given here is exactly what I am trying to accomplish. Scrollable list of swipeable items that expose additional options, a la a mobile to do list or something similar.

Looking at the documentation you referenced for use-gesture, it seems that in conjunction with setting touch-action: pan-y; on the swipeable element, they have an option to specify the axis of swipe when configuring their useDrag hook. Would your solution here mimic or mitigate the need for something that? If you have a build I can test, I am happy to do so.

@bhj
Copy link

bhj commented Dec 18, 2021

Thank you all for your work on this awesome library. There's a tangential issue I ran into that maybe this PR can address: In trying to create a generic <Swipeable> component that sets some defaults, siphons off all the possible swipe-related props and passes the rest through, e.g.:

const Swipeable = ({
  onSwiped,
  onSwipedLeft,
  onSwipedRight,
  onSwipedUp,
  onSwipedDown,
  onSwipeStart,
  onSwiping,
  onTap,
  delta = 10,
  preventDefaultTouchmoveEvent = true,
  trackTouch = true,
  trackMouse = true,
  rotationAngle = 0,
  ...props
}) => {
  ...
}

I noticed that the preventDefaultTouchmoveEvent test is only checking for the presence of the onSwiped* properties so in this case all scrolling gets prevented. It would be nicer if it checked whether their values are actually defined, I think?

@hartzis
Copy link
Collaborator

hartzis commented Jan 17, 2022

I noticed that the preventDefaultTouchmoveEvent test is only checking for the presence of the onSwiped* properties so in this case all scrolling gets prevented. It would be nicer if it checked whether their values are actually defined, I think?

@bhj thanks for the feedback, curious what you mean by if it checked whether their values are actually defined, the current presence check sounds similar. Do you think you could code up a small example?

@bhj
Copy link

bhj commented Jan 18, 2022

@bhj thanks for the feedback, curious what you mean by if it checked whether their values are actually defined, the current presence check sounds similar.

Given my example Swipeable HoC, say it eventually passes all possible swipe-related props to useSwipeable. If I only give Swipeable an onSwipedLeft prop, then most of the config properties useSwipeable receives have the value undefined. Yet all scrolling will still be prevented, because in only checks for the presence of a property, and returns true even if its value is undefined.

So, in this conditional:

if (props.onSwiping || props.onSwiped || `onSwiped${dir}` in props)

The third check is doing something very different from the first two. I think it should check whether the properties' values are actually defined, like the first two, rather than simply whether or not the properties exist.

@hartzis
Copy link
Collaborator

hartzis commented Feb 1, 2022

So, in this conditional:

if (props.onSwiping || props.onSwiped || `onSwiped${dir}` in props)

The third check is doing something very different from the first two. I think it should check whether the properties' values are actually defined, like the first two, rather than simply whether or not the properties exist.

giphy

yup, lol. welp that's been hiding in there for a while 😬

@bhj Thank you for pointing that out, apologies for not fully grasping what you were getting at before.

We'll get this fixed in the next version for sure 👍

src/index.ts Outdated
props.onSwiping && props.onSwiping(eventData);

// track if a swipe is cancelable(handler for swiping or swiped(dir) exists)
// track if a swipe is cancelable (handler for swiping or swiped(dir) exists)
// so we can call preventDefault if needed
let cancelablePageSwipe = false;
if (props.onSwiping || props.onSwiped || `onSwiped${dir}` in props) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (props.onSwiping || props.onSwiped || `onSwiped${dir}` in props) {
if (props.onSwiping || props.onSwiped || props[`onSwiped${dir}`]) {

📓 NOTE to @hartzis fix this and add test for { onSwipedLeft: undefined }

Copy link
Collaborator

Choose a reason for hiding this comment

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

  • added fix! and documented in changelog

@hartzis hartzis changed the base branch from main to v7 February 18, 2022 22:47
@hartzis hartzis self-assigned this Feb 21, 2022
@hartzis hartzis merged commit 9e7d848 into v7 Feb 21, 2022
@github-actions
Copy link
Contributor

size-limit report 📦

Path Size
dist/react-swipeable.js 1.11 KB (0%)
dist/react-swipeable.umd.js 1.12 KB (0%)
dist/react-swipeable.module.js 1.14 KB (0%)

This was referenced Feb 25, 2022
Merged
hartzis added a commit that referenced this pull request Apr 27, 2022
* Replace microbundle with rollup (#273)

* Prevent scroll prop (#240)

* update changelog with callouts

* update format, fix lint config issue, update contributing

* set ts output target to es2015, update changelog

* add comments to types for more info (#293)

* setup touchEventOptions prop (#290)

* setup touchEventOptions prop

* add onTouchStartOrOnMouseDown prop and test

* add onTouchEndOrOnMouseUp callback

* Feature swipeDuration (#291)

* update docs and migration

* 7.0.0-alpha.0

* update deps, add react 18 peer

* 7.0.0-alpha.1

* update examples to use react 18

* update deps, update LICENSE dates

* update readme, types, example types

* export/outputs housekeeping and cleaning

* 7.0.0-alpha.2

* fix issue #304

* 7.0.0-alpha.3

* bump deps

Co-authored-by: Binoy <me@binoy.io>
Co-authored-by: Stefan Huynh <stefan.huynh@formidable.com>
Co-authored-by: Brian Emil Hartz <brianhartz@gmail.com>
@github-actions github-actions bot mentioned this pull request May 18, 2023
@paulmarsicloud paulmarsicloud mentioned this pull request Jun 1, 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.

5 participants