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

bug fix: Link label not properly centering #486

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cjs8487
Copy link

@cjs8487 cjs8487 commented Jul 18, 2021

Resolves #420

Happened to run into the issue myself while working on a program that relies heavily on link labelling and stumbled across the open issue and figured I'd handle it since I needed the fix. Version still needs to be bumped, I imagine to 2.6.1 but let me know if 2.7 would be preferable for any reason and I'll take care of it

Copy link
Owner

@danielcaldas danielcaldas left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! To move forward we require another small iteration on this change.

@@ -99,14 +99,15 @@ export default class Link extends React.Component {
fill: this.props.fontColor,
fontSize: this.props.fontSize,
fontWeight: this.props.fontWeight,
textAnchor: "middle",
Copy link
Owner

Choose a reason for hiding this comment

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

For retro-compatibility to move forward with this PR I'll prefer that we allow clients to pass this prop alongside node.color and others. Please refer to the docs and a sample past PR to know what files require an update to make this work

Copy link
Author

Choose a reason for hiding this comment

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

Just to clarify, shouldn't the prop be passed alongside other link props, such as link.fontColor? Similarly, should clients be able to pass the startOffset (which is also currently locked to 50%)? That change seems to be of a similar nature to allowing clients to pass textAnchor as a prop, since it was previously locked to "middle". Glad to combine any or all of these changes as you see fit.

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry for the confusion. Yes it should be link.textAnchor, but the changeset should be similar. Here's a PR that introduced a property in the link for reference

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.

Link label is not positioned correctly
2 participants