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

SVG link rendering #791

Merged
merged 3 commits into from
May 28, 2019
Merged

SVG link rendering #791

merged 3 commits into from
May 28, 2019

Conversation

flying-sheep
Copy link
Contributor

@flying-sheep flying-sheep commented Feb 6, 2019

This continues #765. Currently, links don’t cover the full node area, just a line within the foreignObject. This PR changes that.

Design

I don’t pass labelType to the renderer anymore, but construct SVG in all cases. I use addHtmlLabel from dagre-d3-renderer instead of duplicating its code for this.

That’s necessary because dagre-d3-renderer doesn’t offer a hook to postprocess rendered SVG nodes – if it did, we could simply use that hook to wrap the SVG group in an SVG link.

TODOs

  • Unfortunately this needs to render the vertices in a live SVG instead of delaying that to the renderer: I had to add a parameter to addVertices to achieve that, I hope that’s acceptable. The reason is that in addHtmlLabel, the renderer needs to retrieve the bounding box of the foreignObject contents in order to set the node size. Without a live SVG there’s no bounding box, so if we want to call addHtmlLabel ourselves instead of letting the renderer do it, we need the live SVG.

  • I’m not sure if we do labelType: text, but if we do, I need to adapt this PR for it (currently it’s not handled I think)

  • Finally, the sizes aren’t perfectly determined for clickable non-link nodes. I don’t know what causes that, maybe someone can help:

    Before This PR
    Before After

    (note the g in „Shopping“)

Alternatives

  1. We do some postprocessing, so instead of this, we could save the link into a node’s data-link attribute and wrap the nodes in a postprocessing step here

    https://github.com/knsv/mermaid/blob/12b58a17e10988b1cc7790f721e4e59fc6150d52/src/diagrams/flowchart/flowRenderer.js#L405-L410

    That would be easier but less elegant and less performant. (we’d have to touch each label, replace it with a link node, and insert it below that link node again. Many many DOM manipulations)

  2. We could enhance dagre-d3-renderer with first-class link support or a postprocessing hook. The hook has the same problem as alternative 1 and it’s unclear if enhancing it by a link property has a place in the current dagre node model. Would need some investigation.

Tests

By introducing ES6 imports from node_modules, I had to change the way the babel config is loaded (see d78b33c). What I did is described here.

@coveralls
Copy link

coveralls commented Apr 26, 2019

Pull Request Test Coverage Report for Build 749

  • 1 of 23 (4.35%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.2%) to 54.424%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/diagrams/flowchart/flowRenderer.js 1 23 4.35%
Files with Coverage Reduction New Missed Lines %
src/diagrams/flowchart/flowRenderer.js 2 19.54%
Totals Coverage Status
Change from base Build 716: 0.2%
Covered Lines: 2051
Relevant Lines: 3744

💛 - Coveralls

@flying-sheep
Copy link
Contributor Author

Hi! Finally figured out what to do to fix the tests!

I think this looks good apart from the sizing issue.

@knsv
Copy link
Collaborator

knsv commented May 28, 2019

Thx, mergin!

@knsv knsv merged commit ee0cb87 into mermaid-js:master May 28, 2019
@flying-sheep flying-sheep deleted the svg-links branch May 28, 2019 21:20
mgenereu pushed a commit to mgenereu/mermaid that referenced this pull request Jun 25, 2022
…yarn/develop/sveltejs/adapter-static-1.0.0-next.30

chore(deps-dev): bump @sveltejs/adapter-static from 1.0.0-next.29 to 1.0.0-next.30
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.

3 participants