new(zoom): add pinch and zoom support #1305

tonyneel923 · 2021-08-04T19:54:08Z

💥 Breaking Changes

zoom.containerRef needs to be added to whatever element needs zoom functionality.
props.transformMatrix was changed to => initialTransformMatrix
passive/style/className were removed

🚀 Enhancements

Zoom now supports mobile pinch.

📝 Documentation

I consolidated the handlers all into react-use-gesture. A user can opt out by not adding the containerRef so it seems like passive was no longer necessary.

In order for mobile zoom to work fully users must add style={{ touchAction: 'none' }} to the element in order for the browser to allow js to handle pinch and zoom.

Also I had to use the newest version of use-gesture which is technically in beta because of bugs in version 9. By the author's own admission it is ready for production so I do not see an issue with it.

Discussion which prompted this PR: #1292

tonyneel923 · 2021-08-04T23:02:35Z

It seems the build fails because it tries to download from my companies mirror registry. I only have a work laptop so I cannot fix that unless I remove the yarn lock changes completely but that would make it not in sync.

Is it possible for someone to fix that minor issue for me? Anything else I can do.

williaster · 2021-08-06T00:38:44Z

Wow thanks for the great contribution @tonyneel923 ! ❤️

I will try to take a look at this tomorrow (sorry it's a bigger change so want to look at it more closely!). I just pulled your branch to update the lock file and it allowed me to push to your branch. Sorry if that's not what you meant, my only other thought was to add a .npmrc file in the monorepo root which specified the npm or yarn registry so this wouldn't be a problem in the future (thanks for pointing it out! I may add that separately in another PR)

williaster

Overall this is looking killer! Just did a quick first pass and will check it out again tomorrow.

I think in the breaking changes (in addition to zoom.containerRef that you mentioned) we should also note that

props.transformMatrix was changed to => initialTransformMatrix
passive/style/className were removed

packages/visx-zoom/src/types.ts

williaster · 2021-08-06T00:46:48Z

packages/visx-zoom/src/types.ts

@@ -44,6 +46,8 @@ export interface ProvidedZoom {
  reset: () => void;
  /** Callback for a wheel event, updating scale based on props.wheelDelta, relative to the mouse position. */
  handleWheel: (event: React.WheelEvent | WheelEvent) => void;
+  /** Callback for a react-use-gesture on pinch event, updating scale based on props.pinchDelta, relative to the pinch position */
+  handlePinch: Parameters<typeof useGesture>[0]['onPinch'];


looks like they export UserHandlers from @use-gesture/react, so UserHandlers['onPinch'] might work

packages/visx-zoom/src/Zoom.tsx

williaster · 2021-08-06T01:11:22Z

packages/visx-zoom/src/Zoom.tsx

+      onPinch: handlePinch,
+      onWheel: ({ event }) => handleWheel(event),
+    },
+    { target: containerRef, eventOptions: { passive: false }, drag: { filterTaps: true } },


options seem legit to me

tonyneel923 · 2021-08-06T15:14:31Z

Wow thanks for the great contribution @tonyneel923 ! ❤️

I will try to take a look at this tomorrow (sorry it's a bigger change so want to look at it more closely!). I just pulled your branch to update the lock file and it allowed me to push to your branch. Sorry if that's not what you meant, my only other thought was to add a .npmrc file in the monorepo root which specified the npm or yarn registry so this wouldn't be a problem in the future (thanks for pointing it out! I may add that separately in another PR)

I am not sure why I did not think of a .npmrc file. We will go with I was tired. Thanks for the feedback on the pr. I will address it today.

…cess to zoom in callback

tonyneel923 · 2021-08-06T16:19:12Z

I have updated the pr with your requested changes. I also changed pinchDelta to match wheelDelta as I realized the way it was would not work since you would have access to zoom in the callback. I tested on example and it worked perfectly.

tonyneel923 · 2021-08-06T17:55:18Z

Final note is I changed the test in zoom to not fail but I am not sure why it failed at finding one div because it definitely renders the contents. I do not have much experience with enzyme though and generally use rtl.

williaster

Woot! @tonyneel923 this looks great to me 🔥 I pulled it locally and tested on my phone 💯

I had a couple super minor comments but then I think it's ready to land! Thanks again for the great addition 🙏 We are coordinating some breaking changes with react@17, so this may land first as an alpha but will be out soon.

packages/visx-zoom/src/Zoom.tsx

williaster · 2021-08-09T23:49:54Z

packages/visx-zoom/test/Zoom.test.tsx

@@ -33,8 +33,7 @@ describe('<Zoom />', () => {
      </Zoom>,
    );

-    expect(wrapper.find('div')).toHaveLength(1);


I think previously when passive=false (default), Zoom wrapped children in a div so that's why this is no longer there 👍

…moization to onPinch handler

tonyneel923 · 2021-08-11T15:08:09Z

I added in the changes you requested and I found a way to keep from running getBoundingClientRect all the time using the built in memo parameter in onPinch.

williaster

Nice approach on the memoization! working on a release issue then will merge this 🎉

github-actions · 2021-08-11T23:49:37Z

🎉 This PR is included in version v2.0.2-alpha.0 of the packages modified 🎉

github-actions · 2021-08-12T00:10:54Z

🎉 This PR is included in version v2.0.3-alpha.0 of the packages modified 🎉

github-actions · 2021-08-17T20:43:03Z

🎉 This PR is included in version v2.1.0 of the packages modified 🎉

boy51 · 2021-11-10T13:49:13Z

Hey I appreciate this change! I'm using zoom in a very tight component and it would be really helpful we could be more explicit about breaking changes. For now, I don't feel like I can implement this for fear of introducing bugs.

For example, I used passive because we are handling mousewheel manually. How am I supposed to migrate? I see a brief mention of omitting the containerRef. What does it do for me in the first place?

Also why the change to initialTransformMatrix? Is it purely semantic? Should I just rename transformMatrix, or does it change the behaviour? If not, why is a breaking change needed for this?`

Also there are undocumented breakign changes, the interface ProvidedZoom now requires generic.

If there is documentation surrounding this please let me know!

feat: added pinch and zoom support to visx-zoom

3f39f4f

williaster added @visx/zoom 🚀enhancement labels Aug 6, 2021

deps: update yarn.lock

7e40e1f

williaster reviewed Aug 6, 2021

View reviewed changes

Anthony Neel added 2 commits August 6, 2021 09:03

chore: pr comments

ddc6b61

feat: changed pinchDelta to match wheelDelta since you do not have ac…

56d640c

…cess to zoom in callback

fix: zoom test

0975196

tonyneel923 requested a review from williaster August 6, 2021 17:57

williaster reviewed Aug 9, 2021

View reviewed changes

williaster changed the title ~~feat: added pinch and zoom support to visx-zoom~~ new(zoom): add pinch and zoom support Aug 9, 2021

feat: enhanced performance by pulling out default props and adding me…

8f4a350

…moization to onPinch handler

williaster approved these changes Aug 11, 2021

View reviewed changes

williaster added alpha-release patch-release labels Aug 11, 2021

williaster merged commit 2fc69e9 into airbnb:master Aug 11, 2021

williaster mentioned this pull request Sep 7, 2021

Zoom event with key shift #1334

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new(zoom): add pinch and zoom support #1305

new(zoom): add pinch and zoom support #1305

tonyneel923 commented Aug 4, 2021 •

edited

Loading

tonyneel923 commented Aug 4, 2021

williaster commented Aug 6, 2021

williaster left a comment

williaster Aug 6, 2021

williaster Aug 6, 2021

tonyneel923 commented Aug 6, 2021

tonyneel923 commented Aug 6, 2021

tonyneel923 commented Aug 6, 2021

williaster left a comment

williaster Aug 9, 2021

tonyneel923 commented Aug 11, 2021

williaster left a comment

github-actions bot commented Aug 11, 2021

github-actions bot commented Aug 12, 2021

github-actions bot commented Aug 17, 2021

boy51 commented Nov 10, 2021

new(zoom): add pinch and zoom support #1305

new(zoom): add pinch and zoom support #1305

Conversation

tonyneel923 commented Aug 4, 2021 • edited Loading

💥 Breaking Changes

🚀 Enhancements

📝 Documentation

tonyneel923 commented Aug 4, 2021

williaster commented Aug 6, 2021

williaster left a comment

Choose a reason for hiding this comment

williaster Aug 6, 2021

Choose a reason for hiding this comment

williaster Aug 6, 2021

Choose a reason for hiding this comment

tonyneel923 commented Aug 6, 2021

tonyneel923 commented Aug 6, 2021

tonyneel923 commented Aug 6, 2021

williaster left a comment

Choose a reason for hiding this comment

williaster Aug 9, 2021

Choose a reason for hiding this comment

tonyneel923 commented Aug 11, 2021

williaster left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 11, 2021

github-actions bot commented Aug 12, 2021

github-actions bot commented Aug 17, 2021

boy51 commented Nov 10, 2021

tonyneel923 commented Aug 4, 2021 •

edited

Loading