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

new(drag): Add restrictToPath as a Drag parameter #1379

Merged
merged 3 commits into from
Jan 20, 2022

Conversation

valtism
Copy link
Contributor

@valtism valtism commented Nov 23, 2021

🚀 Enhancements

Allow the user to pass an SVGGeometryElement to restrict the drag area
to following the path of an SVG.

Useful for constraining the drag of an object to a curved line.

Allow the user to pass an SVGGeometryElement to restrict the drag area
to following the path of an SVG.

Useful for constrining the drag of an object to a curved line.
@valtism
Copy link
Contributor Author

valtism commented Nov 23, 2021

@williaster Hardly had the last drag enhancement merged when I realised I needed another way to constrain my drag. This PR adds a way to constrain drag to an SVGGeometry like path. Simply constraining on onDragMove leads to jitter like this:

Jittery.drag.mov

With the constraint applied, the drag becomes much smoother.

Screen.Recording.2021-11-23.at.4.57.57.pm.mov

This should work even if the path is nested deeply within the SVG, as it applies a transform based on the parent SVG it is relative to.

Calculate and cache sample points along restrictToPath.

This turns out to be fairly tricky as the path is a ref and we need to
use the getTotalLength function as an estimate to when the path has changed.

Remove getParentSvg. Turns out we can just use the DOMMatrix from the
path element itself, saving the need to traverse the DOM.

Fix bug with summing points for offset, rather than finding the difference.
@valtism
Copy link
Contributor Author

valtism commented Dec 1, 2021

Unsure of what the issues with Happo are.

Also, only now realised that I totally don't need to even use these restrict properties. As long as I am updating the underlying data in React, I can just use <Drag> without applying dx and dy to the transform and not get any of the jitter from React and the transform fighting. 🤦

Oh well, I think this is still a useful feature, especially if only making changes with onDragEnd.

Copy link
Collaborator

@williaster williaster left a comment

Choose a reason for hiding this comment

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

Hey @valtism 👋 sorry for such a long time getting this in. Glad you found another workaround so weren't blocked by it, but agree it's useful so happy to land it. I had one small docs nit but can commit it my self with the UI if you're over it!

As always, thanks for the great contribution ❤️

packages/visx-drag/src/useDrag.ts Outdated Show resolved Hide resolved
x: number;
y: number;
}>({ x: 0, y: 0 });
const [dragStartPointerOffset, setDragStartPointerOffset] = useState<Point>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks for making Point more consistent (rather than mixing { x, y } objects)

const samples = [];
const pathLength = restrictToPath.getTotalLength();
for (let sampleLength = 0; sampleLength <= pathLength; sampleLength += precision) {
const sample = restrictToPath.getPointAtLength(sampleLength);
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, looks like the returned DOMPoint is not supported in IE, tho .getPointAtLength supposedly is for SVGPathElements (not SVGGeometryElement generally) and I'm pretty sure we use it elsewhere. also IE end of life is in June 2022 so maybe no need to worry about it 🎉

@williaster
Copy link
Collaborator

also, no happo problems – sometimes with chrome version changes we get some spurious diffs because font rendering looks different or something.

Complete sentence for docs

Co-authored-by: Chris Williams <williaster@users.noreply.github.com>
@williaster williaster changed the title Add restrictToPath as a Drag parameter new(drag): Add restrictToPath as a Drag parameter Jan 20, 2022
@williaster
Copy link
Collaborator

thanks again!

@williaster williaster merged commit 3ac3f5e into airbnb:master Jan 20, 2022
@github-actions
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants