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

Fix for non-string SankeyDiagram nodeLabelText prop #54

Merged
merged 2 commits into from
Mar 12, 2018

Conversation

dandelany
Copy link
Contributor

Starting in v0.4.2 we allowed the SankeyDiagram nodeLabelText prop to return an arbitrary HTML element (as opposed to a string) in order to support custom labels which are more complicated than a single text string. These were rendered inside a <foreignObject> tag which was positioned using a translate transform which attempted to follow all node placement rules and allow the user to use HTML instead of SVG text.

However, @krissalvador27 noticed that positioning of these labels was broken in Chrome when the SankeyDiagram is inside of a ZoomContainer. Further investigation revealed that Chrome's support for <foreignObject> is rather poor, likely due to issues Webkit (see eg. Webkit issues 71819 and 93358).

The most flexible fix for this is to simplify our handling of these custom labels and render them as arbitrary SVG elements instead of <foreignObject>s. This still allows the user to render <foreignObject>s on their own, if they want to figure out the placement issues, but takes the responsibility for this positioning out of the hands of Reactochart.

Dan Delany added 2 commits March 12, 2018 14:00
@krissalvador27
Copy link
Contributor

👍 changes look good!

@dandelany dandelany merged commit fac4214 into master Mar 12, 2018
@krissalvador27 krissalvador27 deleted the sankey-label-change branch May 2, 2018 18:49
install pushed a commit that referenced this pull request Feb 25, 2020
Fix for non-string SankeyDiagram nodeLabelText prop
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.

2 participants