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

add touch events for brush component #864

Closed
wants to merge 2 commits into from
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
add touch events for brush component
BrianRosamilia committed Oct 9, 2020
commit b16b6c8d6a75c83fe2eef020989f1106bbdebeec
14 changes: 14 additions & 0 deletions packages/visx-brush/src/BaseBrush.tsx
Original file line number Diff line number Diff line change
@@ -376,6 +376,7 @@ export default class BaseBrush extends React.Component<BaseBrushProps, BaseBrush
onDragMove={this.handleDragMove}
onDragEnd={this.handleDragEnd}
>
{/* resize */}
{({ dragStart, isDragging, dragMove, dragEnd }) => (
<Bar
className="visx-brush-overlay"
@@ -405,6 +406,19 @@ export default class BaseBrush extends React.Component<BaseBrushProps, BaseBrush
if (onMouseUp) onMouseUp(event);
dragEnd(event);
}}
onTouchStart={(event: MouseHandlerEvent) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 react@16.4. 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

this.mouseDownTime = Date.now();
dragStart(event);
}}
onTouchMove={(event: MouseHandlerEvent) => {
if (!isDragging && onMouseMove) onMouseMove(event);
if (isDragging) dragMove(event);
}}
onTouchEnd={(event: MouseHandlerEvent) => {
this.mouseUpTime = Date.now();
if (onMouseUp) onMouseUp(event);
dragEnd(event);
}}
style={BRUSH_OVERLAY_STYLES}
/>
)}
3 changes: 3 additions & 0 deletions packages/visx-brush/src/BrushHandle.tsx
Original file line number Diff line number Diff line change
@@ -144,6 +144,9 @@ export default class BrushHandle extends React.Component<BrushHandleProps> {
onMouseDown={dragStart}
onMouseMove={dragMove}
onMouseUp={dragEnd}
onTouchStart={dragStart}
onTouchMove={dragMove}
onTouchEnd={dragEnd}
style={{
cursor,
pointerEvents: !!brush.activeHandle || !!brush.isBrushing ? 'none' : 'all',
12 changes: 12 additions & 0 deletions packages/visx-brush/src/BrushSelection.tsx
Original file line number Diff line number Diff line change
@@ -112,6 +112,7 @@ export default class BrushSelection extends React.Component<
onDragMove={this.selectionDragMove}
onDragEnd={this.selectionDragEnd}
>
{/* location */}
{({ isDragging, dragStart, dragEnd, dragMove }) => (
<g>
{isDragging && (
@@ -122,6 +123,8 @@ export default class BrushSelection extends React.Component<
onMouseUp={dragEnd}
onMouseMove={dragMove}
onMouseLeave={dragEnd}
onTouchMove={dragMove}
onTouchEnd={dragEnd}
style={DRAGGING_OVERLAY_STYLES}
/>
)}
@@ -146,6 +149,15 @@ export default class BrushSelection extends React.Component<
onClick={event => {
if (onClick) onClick(event);
}}
onTouchStart={disableDraggingSelection ? undefined : dragStart}
Copy link
Collaborator

Choose a reason for hiding this comment

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

same comment on remove these additions and updating the above mouse handlers to the pointer equivalents 👍

onTouchMove={event => {
dragMove(event);
if (onMouseMove) onMouseMove(event);
}}
onTouchEnd={event => {
dragEnd(event);
if (onMouseUp) onMouseUp(event);
}}
style={{
pointerEvents: brush.isBrushing || brush.activeHandle ? 'none' : 'all',
cursor: disableDraggingSelection ? undefined : 'move',
2 changes: 2 additions & 0 deletions packages/visx-drag/src/Drag.tsx
Original file line number Diff line number Diff line change
@@ -120,6 +120,8 @@ export default class Drag extends React.Component<DragProps, DragState> {
height={height}
onMouseMove={this.handleDragMove}
onMouseUp={this.handleDragEnd}
onTouchMove={this.handleDragMove}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

onTouchEnd={this.handleDragEnd}
fill="transparent"
/>
)}