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

Jump to bottom

typescript(vx-drag): re-write package in TypeScript #499

Merged
merged 13 commits into from
Nov 13, 2019
Merged

typescript(vx-drag): re-write package in TypeScript #499

merged 13 commits into from
Nov 13, 2019

Conversation

williaster
Copy link
Collaborator

@williaster williaster commented Sep 18, 2019 •
edited
Loading

🚀 Enhancements

This PR builds off of #488 which added TypeScript build support, and converts the @vx/drag package to TypeScript.

Depends on #493 (for @vx/event types), and closes #498.

💥 Breaking Changes

  • This PR introduces React.Fragments, which requires bumping the peerDep for react to ^16.3.0-0
  • Empty parent <g> wrapper around Drag children was replaced with a React.Fragment which removes a DOM element.

Testing

@hshoff @schillerk @milesj @techniq @kristw

kristw
kristw reviewed Sep 18, 2019
View reviewed changes
packages/vx-drag/src/Drag.tsx Outdated
y: resetOnStart ? point.y : -dy + point.y,
};
if (onDragStart) onDragStart({ ...nextState, event });
this.setState(() => nextState);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be this.setState(nextState);?

Copy link
Collaborator Author

@williaster williaster Sep 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah good question, I'm not sure there's a compelling reason to use the functional variant here since we are not referencing any existing values.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I refactored this to use functional setState with a callback for onDrag*. This should use freshest state and throttle onDrag* calls in sync with setState updates. Have to invoke event.persist()

packages/vx-drag/src/util/raise.ts Outdated Show resolved Hide resolved
packages/vx-drag/src/Drag.tsx Outdated Show resolved Hide resolved
@williaster williaster changed the base branch from chris--typescript to master October 4, 2019 20:36
@hshoff hshoff added this to In progress in TypeScript via automation Oct 9, 2019
@hshoff hshoff added this to the v0.0.193 milestone Oct 9, 2019
@williaster williaster removed this from In progress in TypeScript Oct 9, 2019
@williaster
Copy link
Collaborator Author

omg finally lol

hshoff
hshoff requested changes Nov 13, 2019
View reviewed changes
Copy link
Member

@hshoff hshoff left a comment

Choose a reason for hiding this comment

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

LGTM! one minor type issue about MouseEvent & TouchEvent. Should be good to go after that.

packages/vx-drag/src/Drag.tsx Outdated
isDragging: false,
};

handleDragStart = (event: React.MouseEvent) => {
Copy link
Member

Choose a reason for hiding this comment

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

these could also be TouchEvent

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good catch, did a sweep 🧹

@williaster williaster mentioned this pull request Nov 11, 2019
Merged
@williaster
Copy link
Collaborator Author

Oi, type errors for vx-responsive + vx-text 🤔

@williaster
Copy link
Collaborator Author

williaster commented Nov 13, 2019 •
edited
Loading

I added types to the lodash usage (seems better to just install the packages + types for the methods we use vs the full lodash lib?), not sure why they were failing all of a sudden 😢

@hshoff
Copy link
Member

hshoff commented Nov 13, 2019

RE: lodash dep #66

@williaster
Copy link
Collaborator Author

ahhh, ic. reverted and just added @types/lodash to vx-text + vx-responsive.

I think this is also failing the build in #512 .

@hshoff hshoff merged commit bb779ee into master Nov 13, 2019
@hshoff hshoff deleted the chris--typescript--vx-drag branch November 13, 2019 22:42
@williaster williaster mentioned this pull request Jan 11, 2020
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hshoff hshoff hshoff approved these changes

@kristw kristw kristw approved these changes

Assignees
No one assigned
Labels
💥 breaking changes 🚀enhancement 🤖 typescript
Projects
None yet
Milestone
v0.0.193
Development

Successfully merging this pull request may close these issues.

Re-write @vx/drag in TypeScript
3 participants
@williaster @hshoff @kristw