-
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
add textPath prop to Text component #926
base: master
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build 383662667Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - 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.
Hi @grsoares21, thanks for the PR! This looks good to me except for a minor change.
packages/visx-text/src/Text.tsx
Outdated
if(props.textPath) { | ||
this.setState({ textPathId: uniqueId('text-path-') }) | ||
} |
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.
I see this is called from componentDidMount()
. Ideally we could move this initial state to the constructor()
because it doesn't rely on the DOM.
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.
Sorry, it took me so long to fix this. I've moved the whole state initialization into the constructor with the id generation in there as well.
@grsoares21 I'm very sorry, |
Hello @susiwen8 . No problems :D. I'm glad to hear it's on hooks now. I'll try to reimplement this next weekend then. |
Sorry for the delayed review, will try and review asap once I see any new commits to avoid any future conflicts 👍 |
Came here for this very thing. Sad to see it has stalled. Are you planning to pick it up again @grsoares21 ? |
If someone else wants to pick this up I'm happy to review! it shouldn't be too complicated, tho as noted |
This was implemented as a solution for #759
It is my first PR to a big open-source project. I'd appreciate your feeback.
💥 Breaking Changes
🚀 Enhancements