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

feat(scroll-on-touch): use finger to scroll on canvas #104

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

DaAnda97
Copy link
Contributor

@DaAnda97 DaAnda97 commented Dec 23, 2022

As a user who works on a tablet, I want to be able to use my touch pen for writing and my finger for scrolling.

This feature is deactivated by default.

@vinothpandian
Copy link
Owner

@DaAnda97 Thanks for the PRs. Sorry, I just noticed them. I am currently working on improving the repo with turborepo & tsup. I'll merge them along with those changes after review.

@vinothpandian
Copy link
Owner

@DaAnda97 It doesn't work on the Mac touchpad.

@DaAnda97
Copy link
Contributor Author

@vinothpandian I meant "tablet" instead of a "touchpad". I have now revised the feature description.

Andreas Riepl and others added 3 commits January 30, 2023 09:41
…feat/scroll-on-touch-sync

# Conflicts:
#	packages/react-sketch-canvas/src/Canvas/index.tsx
feat(scroll-on-touch): use finger to scroll
@DaAnda97 DaAnda97 changed the base branch from master to main January 30, 2023 09:16
@DaAnda97
Copy link
Contributor Author

@vinothpandian I resolved the merge conflicts

@vinothpandian
Copy link
Owner

Thanks, @DaAnda97. That was very helpful! I will add some tests and merge them by this week.

@@ -59,6 +60,7 @@ export const Canvas = React.forwardRef<CanvasRef, CanvasProps>((props, ref) => {
const {
paths,
isDrawing,
scrollOnTouch,
Copy link
Owner

Choose a reason for hiding this comment

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

Users must not be able to set allowOnlyPointerType as touch or all while they set scrollOnTouch. ReactSketchCanvas must throw an error with some meaningful error message to avert users from running into this scenario. CanvasProps must also be updated to be conditional here.

if (scrollOnTouch) {
canvasRef.current?.addEventListener("touchstart", listener, { passive: false });
}
return () => canvasRef.current?.removeEventListener("touchstart", listener);
Copy link
Owner

Choose a reason for hiding this comment

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

canvasRef.current might get cleaned up before this cleanup is run. so canvasRef.current must be copied to a variable in the scope of this function. and then you can add or remove event listeners.

canvasRef.current?.addEventListener("touchstart", listener, { passive: false });
}
return () => canvasRef.current?.removeEventListener("touchstart", listener);
}, []);
Copy link
Owner

Choose a reason for hiding this comment

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

As the dependency array is empty, if the developer changes scrollOnTouch dynamically, this function will fail.

@@ -291,13 +311,31 @@ release drawing even when point goes out of canvas */
);
}, [paths]);

// avoid pen from scrolling if scrollOnTouch
useEffect(() => {
const listener = function(
Copy link
Owner

Choose a reason for hiding this comment

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

It's better to move this listener as a callback using useCallback with empty dependency array.

Copy link
Owner

@vinothpandian vinothpandian left a comment

Choose a reason for hiding this comment

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

Thanks again for the PR @DaAnda97. I have some comments that will improve this PR. Also, it'll be great if you can add a test and update the documentation.

If you don't have much time to spend on it, I'll resolve the comments over the coming weekends.

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.

2 participants