add touch events for brush component #864
Conversation
Pull Request Test Coverage Report for Build 298488210
💛 - Coveralls |
|
just wanted to report this as an issue, glad a fix is already on the way 👍
I'd personally prefer that as it makes the code easier to follow and is in line with how the non functional components handle it. |
There was a problem hiding this comment.
Hey @BrianRosamilia apologies for such a delay in reviewing this, we really appreciate the PR! 🙌
I had a couple suggestions here for DRY-ness, lmk what you think and I'll keep an eye out for any updates to merge this ASAP hopefully for the 1.3.0 release.
| @@ -405,6 +405,19 @@ export default class BaseBrush extends React.Component<BaseBrushProps, BaseBrush | |||
| if (onMouseUp) onMouseUp(event); | |||
| dragEnd(event); | |||
| }} | |||
| onTouchStart={(event: MouseHandlerEvent) => { | |||
There was a problem hiding this comment.
on thought I have here for DRY-ness is to update all of these to PointerEvent handlers, which are a combination of MouseEvents + TouchEvents introduced in [email protected]. so onMouseDown => onPointerDown, onMouseLeave => onPointerLeave, onMouseMove => onPointerMove. I think that MouseHandlerEvent will also need to be updated to support React.PointerEvent.
To avoid breaking changes we could could keep the props as onMouseXXX, but could consider also adding onPointerXXX handlers to match, and then deprecate onMouseXXX in a future 2.0 release. This would also require that we bump the react peerDependency in @visx/brush to ^16.4.0, which is theoretically breaking but I just checked @visx/drag (which brush relies on) and it already requires ^16.8.0 so I think technically @visx/brush should already actually be ^16.8.0.
cc @hshoff
| @@ -120,6 +120,8 @@ export default class Drag extends React.Component<DragProps, DragState> { | |||
| height={height} | |||
| onMouseMove={this.handleDragMove} | |||
| onMouseUp={this.handleDragEnd} | |||
| onTouchMove={this.handleDragMove} | |||
There was a problem hiding this comment.
Since this change Drag was re-implemented/re-factored to use the useDrag hook so there are unfortunately conflicts now. I think an easy change might be to simply update onMouseMove/onMouseUp to onPointerMove/onPointerUp, similar to my suggestion in @visx/brush
| @@ -146,6 +148,15 @@ export default class BrushSelection extends React.Component< | |||
| onClick={event => { | |||
| if (onClick) onClick(event); | |||
| }} | |||
| onTouchStart={disableDraggingSelection ? undefined : dragStart} | |||
There was a problem hiding this comment.
same comment on remove these additions and updating the above mouse handlers to the pointer equivalents 👍
|
Hi @BrianRosamilia just wanted to ping this PR again to see if we can get it past the finish line, or if I should take over. Thanks & happy New Year! |
|
@williaster we've been facing the same issue, and we need this pr to get approved and merged ASAP. Is there any way to speed things up? |
|
@williaster Hi again, i created a new pr to address the reviews if you could check it please: #1155 |
|
This will be superseded by #1155 |
Fixes #845
The fix seems fairly simple and I think covers all scenarios.
One thing to note: it's hard to DRY out this code into member functions because of the hooks that the event handlers rely on :O I could create local functions in the jsx expression itself to DRY it up a little bit, or use higher order member functions, but I figured that this PR is actually the simplest/best to at least get started on fixing this and get some feedback first since I am totally new to this project 😄
💥 Breaking Changes
🐛 Bug Fix