-
Notifications
You must be signed in to change notification settings - Fork 715
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
fix(brush): use PointerEvents instead of MouseEvents #1155
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for starting a new PR for this long-standing fix @RemaaBdair ! 🙏
I had one suggestion to simplify but otherwise looks good to me. Note, too, that it is currently failing our prettier
linting. I think after my suggested changes it may want the type definitions to each be on a single line. You should be able to run yarn format
in your visx
root directory to fix it after the suggested changes, and then commit any changes that come from running that.
@@ -10,9 +10,10 @@ import { MarginShape, Point, BrushShape, ResizeTriggerAreas, PartialBrushStartEn | |||
|
|||
const BRUSH_OVERLAY_STYLES = { cursor: 'crosshair' }; | |||
|
|||
type MouseHandlerEvent = | |||
type PointerHandlerEvent = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all of this can be simplified to React.PointerEvent<SVGRectElement>
, which is already equivalent to MouseEvent | TouchEvent
type MouseHandler = ( | ||
event: React.MouseEvent<SVGRectElement, MouseEvent> | React.TouchEvent<SVGRectElement>, | ||
type PointerHandler = ( | ||
event: React.MouseEvent<SVGRectElement, MouseEvent> | React.TouchEvent<SVGRectElement> | React.PointerEvent<SVGRectElement>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment here
event: React.MouseEvent<SVGRectElement, MouseEvent> | React.TouchEvent<SVGRectElement> | React.PointerEvent<SVGRectElement>, | |
event: React.PointerEvent<SVGRectElement>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing, thanks for the fix @RemaaBdair !
🎉 This PR is included in version |
I opened this pr with the same changes as in this one #864, but with addressing the reviews as well in order to speed things up.
💥 Breaking Changes
🚀 Enhancements
📝 Documentation
🐛 Bug Fix
🏠 Internal