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(dragging): Create (new) IDragger and IDraggable interfaces #7953

Merged
merged 3 commits into from
Mar 22, 2024

Conversation

cpcallen
Copy link
Contributor

The basics

The details

Resolves

Fixes #7838.

Proposed Changes

  • Rename existing core/interfaces/i_draggable.ts to core/interfaces/i_draggable.old.ts and update imports of it.
  • Introduce new IDragable interface in core/interfaces/i_draggable.ts.
  • Introduce new IDragger interface in core/interfaces/i_dragger.ts.

Reason for Changes

API changes to support custom dragging.

Test Coverage

Passes npm test.

Additional Information

  1. I wasn't sure about whether to make the target parameter of IDraggable's drag method optional or nullable. Opinions welcome.

  2. I wasn't clear whether the totalDelta parameter of IDragger's onDrag method was intended to be cumulative or not, but I supposed it was given that the non-cumulative drag distance is presumably available via the (non-optional) PointerEvent parameter. (It's slightly unclear to me why we are making the gesture responsible for keeping track of the cumulative drag, but perhaps it has reasons to do so already.)

@cpcallen cpcallen requested a review from BeksOmega March 20, 2024 00:09
@cpcallen cpcallen requested a review from a team as a code owner March 20, 2024 00:09
@github-actions github-actions bot added the PR: feature Adds a feature label Mar 20, 2024
core/interfaces/i_draggable.ts Outdated Show resolved Hide resolved
core/interfaces/i_dragger.ts Outdated Show resolved Hide resolved
Rename core/interfaces/i_draggable.ts to
core/interfaces/i_draggable.old.ts to make room for new
IDraggable.  Do not rename actual interface as it's not yet
clear that it will be necessary for both to coexist as
imports in the same file.

core/interfaces/i_draggable.ts Show resolved Hide resolved
* be supplied as an alternative to providing a PointerEvent for
* programatic drags.
*/
drag(newLoc: Coordinate, e?: PointerEvent): void;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you want to structure the method signature like this, should the event be non-optional? Otherwise you basically have three function signatures:

drag(newLoc: Coordinate);
drag(newLoc: Coordinate, e: PointerEvent);
drag(newLoc: Coordinate, target: IDragTarget);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That looks like what I intended.

If e: PointerEvent is non-optional then target would need to be nullable (or otherwise optional) to cover the case where you are doing a programmatic drag (no PointerEvent, unless maybe you synthesise one) and you don't happen to be over a drag target.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make more sense to explicitly provide the three function signatures then? Small preference from me, but not a big enough deal to block. I'm going to switch this review back to approve.

Comment on lines 41 to 43
* @param e PointerEvent that continued the drag; could be used to
* look up any IDragTarget the pointer is over, check modifier
* keys, etc.
Copy link
Collaborator

Choose a reason for hiding this comment

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

If provided, they /should/ look up the drag target and deal with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

@cpcallen cpcallen enabled auto-merge (squash) March 22, 2024 16:19
@cpcallen cpcallen merged commit 11c219c into google:rc/v11.0.0 Mar 22, 2024
6 checks passed
@cpcallen cpcallen deleted the feat/drag-interfaces branch March 22, 2024 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feature Adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants