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

[feature][vx-brush] add Brush component #553

Merged
merged 22 commits into from
Dec 18, 2019

Conversation

geekplux
Copy link
Contributor

@geekplux geekplux commented Nov 1, 2019

Closes: #556

🚀 Enhancements

image

Inspired and migrated from data-ui
Please check and review.

@geekplux
Copy link
Contributor Author

geekplux commented Nov 1, 2019

Now the movement of BrushSelection in minimap is not smooth, I guess it is caused by the way update the main view here. I don't know if we change it to a new way e.g. using zoom or rescale might be better.

The code in docs would update once the demo code updated finally.

@williaster
Copy link
Collaborator

Thanks @geekplux for the migration/addition! I will take a look at the PR ASAP in the next day or two 🎉

@hshoff
Copy link
Member

hshoff commented Nov 6, 2019

Woo! Thanks for the PR. Haven't had a chance to do an full review but a quick read through looks good.

Looks like this PR needs to update tests for vx-vx and vx-brush and that should make test CI happy. a few minor fixes for the lint check to pass as well. Not blockers as these are relatively minor things.

Copy link
Collaborator

@williaster williaster left a comment

Choose a reason for hiding this comment

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

Did a quick pass, will try an get the @vx/drag re-write done so you can leverage the types.

packages/vx-brush/src/BaseBrush.tsx Outdated Show resolved Hide resolved
packages/vx-brush/src/BaseBrush.tsx Outdated Show resolved Hide resolved
packages/vx-brush/src/Brush.tsx Outdated Show resolved Hide resolved
packages/vx-brush/src/Brush.tsx Outdated Show resolved Hide resolved
packages/vx-brush/src/BrushHandle.tsx Outdated Show resolved Hide resolved
packages/vx-brush/src/BrushHandle.tsx Outdated Show resolved Hide resolved
@geekplux
Copy link
Contributor Author

I saw the lint test is still failed, but there is no detail info to figure out which lines caused these errors.

@hshoff
Copy link
Member

hshoff commented Nov 21, 2019

TypeError: Cannot set property 'hasAwait' of null
Occurred while linting ~/vx/packages/vx-responsive/test/ScaleSVG.test.tsx:12
     at Object.AwaitExpression (~/vx/node_modules/eslint/lib/rules/require-await.js:92:36)
     at ReturnStatement (~/vx/node_modules/@typescript-eslint/eslint-plugin/dist/rules/require-await.js:68:27)

https://github.com/hshoff/vx/blob/master/packages/vx-responsive/test/ScaleSVG.test.tsx

Can probably just disable the eslint rule for now:

  test('it should expose its ref via an innerRef prop', () => {
    // eslint-disable-next-line require-await
    return new Promise(done => {

@geekplux
Copy link
Contributor Author

geekplux commented Dec 2, 2019

seems no --clean and --extensions in babel options which caused all checks failed
https://babeljs.io/docs/en/options

@williaster williaster added this to the v0.0.193 milestone Dec 18, 2019
@williaster
Copy link
Collaborator

@geekplux thanks again for your work on this! I am going to try and wrap up the TS project over the next week and experiment with an alpha release which will include this. Will ping you when it's out! 🙌

@williaster
Copy link
Collaborator

Test coverage went down which is why the build is 🔴 . Going to merge and hope we can circle back on tests.

@williaster williaster merged commit 90365d8 into airbnb:master Dec 18, 2019
@geekplux
Copy link
Contributor Author

No problem, feel free to contact me if any issues. Merry Christmas!

@geekplux geekplux deleted the feat/vx-brush branch December 19, 2019 01:57
@williaster
Copy link
Collaborator

@geekplux playing with this locally I don't get the brush selection. I should have time to debug today but if you have thoughts lmk 👍

image

@williaster
Copy link
Collaborator

I think I figured it out. Will post a PR soon with a couple TS changes as well and tag you 👍

@geekplux
Copy link
Contributor Author

Thanks a lot. It's LGTM on my local windows PC and I didn't test it on other machines. It might be a CSS error if only selection area is hidden. Anyway, sounds good the solution you have found.

@jskrt
Copy link

jskrt commented Dec 21, 2019

Is there any documentation on how to use this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Re-write @vx/brush in TypeScript [brush] add <BrushX /> and <BrushY /> components
4 participants