-
Notifications
You must be signed in to change notification settings - Fork 719
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
[xychart/annotation] Fix LineSubject types and strokeWidth #991
Conversation
Pull Request Test Coverage Report for Build 475866565
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix @Janpot. The strokeWidth
fix definitely makes sense but I had one question about the type change.
...restProps | ||
}: LineSubjectProps & Omit<React.SVGProps<SVGLineElement>, keyof LineSubjectProps>) { | ||
}: LineSubjectProps) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you send a link that reproduced the TS error you are seeing? It works okay for me in the /annotation
codesandbox
For other packages/components we have preferred to set the native SVG props on the component as a union with its own exported prop types, rather than on the exported prop types themselves to keep them separate and simplify re-use of the export where relevant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be reproduced in the xychart
codesandbox https://codesandbox.io/s/github/airbnb/visx/tree/master/packages/visx-demo/src/sandboxes/visx-xychart?file=/Example.tsx:7459-7501
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@williaster Update the PR to update AnnotationLineSubject
instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks @Janpot ! LGTM. Will get this out in 1.4.0
hopefully later today.
🐛 Bug Fix
It was impossible to set
strokeWidth
onLineSubject
s because the property was never passed through. I also augmented the types with the svg line properties because I was getting type errors when trying to setstrokeDasharray