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 types and demo: VictoryBrushContainer #1522

Merged
merged 6 commits into from
Apr 23, 2020

Conversation

wsparsons
Copy link
Member

@wsparsons wsparsons commented Apr 21, 2020

  • Adds a VictoryBrushContainerDemo
  • Updates some of the mislabelled props for the demos
  • Updates the type definition to use RangeTuple instead of DomainPropType since it is possible to have the prop to not be provided initially, therefore DomainPropType is the wrong type definition due to it requiring either x or y
  • Adds additional missing props for VictoryBrushContainerProps interface

@wsparsons wsparsons force-pushed the task/audit-victory-brush-container-typescript-props branch from 5544e5c to 06f6e94 Compare April 23, 2020 00:54
@wsparsons wsparsons marked this pull request as ready for review April 23, 2020 01:40
brushStyle?: React.CSSProperties;
clipContainerComponent?: React.ReactElement;
defaultBrushArea?: "all" | "none" | "disable";
Copy link
Contributor

Choose a reason for hiding this comment

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

defaultBrushArea should include the option "move"

Copy link
Member Author

Choose a reason for hiding this comment

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

I currently auditing Zoom-Container on a new branch. Would you like me to audit the Zoom-container on this branch as well? I figured I break it up in different PR. Also I don't see defaultBrushArea as an optional prop in the docs nor was it included here.

brushStyle?: React.CSSProperties;
clipContainerComponent?: React.ReactElement;
defaultBrushArea?: "all" | "none" | "disable";
disable?: boolean;
downsample?: number | boolean;
minimumZoom?: CursorData;
Copy link
Contributor

Choose a reason for hiding this comment

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

CursorData is a really confusingly named variable. I know you didn't name it, but do you mind changing it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure I can do that

Copy link
Contributor

@kale-stew kale-stew left a comment

Choose a reason for hiding this comment

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

🚢

@wsparsons
Copy link
Member Author

@boygirl We decided to merge this and keep VictoryZoomContainer work separate.

@wsparsons wsparsons merged commit 1bc1a70 into master Apr 23, 2020
@boygirl boygirl deleted the task/audit-victory-brush-container-typescript-props branch June 23, 2020 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants