Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants