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.

Sign up for GitHub

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

add touch events for brush component #864

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b16b6c8
add touch events for brush component
BrianRosamilia Oct 9, 2020
92ecd66
remove comments
BrianRosamilia Oct 10, 2020
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
13 changes: 13 additions & 0 deletions packages/visx-brush/src/BaseBrush.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,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 [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

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}
/>
)}
Expand Down
3 changes: 3 additions & 0 deletions packages/visx-brush/src/BrushHandle.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
11 changes: 11 additions & 0 deletions packages/visx-brush/src/BrushSelection.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,8 @@ export default class BrushSelection extends React.Component<
onMouseUp={dragEnd}
onMouseMove={dragMove}
onMouseLeave={dragEnd}
onTouchMove={dragMove}
onTouchEnd={dragEnd}
style={DRAGGING_OVERLAY_STYLES}
/>
)}
Expand All @@ -146,6 +148,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',
Expand Down
2 changes: 2 additions & 0 deletions packages/visx-drag/src/Drag.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
/>
)}
Expand Down