feat(gatsby-remark-graphviz): custom SVG attributes and default styling #11624

raulrpearson · 2019-02-07T15:55:23Z

Description

Adds support for custom SVG attributes placed in the lang string of
Markdown code blocks. This:

```dot id="my-id" class="my-class"

will add those attributes to the svg tag of the rendered Graphviz SVG.

It also adds an inline style="max-width: 100%; height: auto;" attribute
by default to every SVG. This inline style can be overwritten on a code
block by code block basis like this:

```dot style=""

The original approach discussed of removing width and height attributes
from SVGs doesn't work because small SVGs become huge when trying to
fill their parent's width.

This commit adds cheerio as a dependency to parse the string of
attributes and to manipulate the SVG returned by Viz.js.

Caveats

Named RegExp groups are used to parse the lang string. This might be a
problem if support for older Node.js versions is required.
Spaces are not trimmed from attribute values
Boolean attributes like data-mydata end up as data-mydata=""

Related Issues

This change was promted by the conversation in gatsbyjs/gatsby/#9480.
Fixes gatsbyjs/gatsby/#9271. The main problem was the lack of
responsiveness from the fixed size SVGs produced by Viz.js.

Adds support for custom SVG attributes placed in the lang string of Markdown code blocks. This: ```dot id="my-id" class="my-class" will add those attributes to the `svg` tag of the rendered Graphviz SVG. It also adds an inline style="max-width: 100%; height: auto;" attribute by default to every SVG. This inline style can be overwritten on a code block by code block basis like this: ```dot style="" This change was promted by the conversation in gatsbyjs/gatsby/#9480. Fixes gatsbyjs/gatsby/#9271. The main problem was the lack of responsiveness from the fixed size SVGs produced by Viz.js. The original approach discussed of removing width and height attributes from SVGs doesn't work because small SVGs become huge when trying to fill their parent's width. This commit adds cheerio as a dependency to parse the string of attributes and to manipulate the SVG returned by Viz.js. Caveats - Named RegExp groups are used to parse the lang string. This might be a problem if support for older Node.js versions is required. - Spaces are not trimmed from attribute values - Boolean attributes like data-mydata end up as data-mydata=""

raulrpearson · 2019-02-21T05:04:22Z

@pieh could we look into merging this PR? Should I go ahead and request a reviewer? Should I just wait? I'm not sure what the etiquette is, but I would like to close this before I totally forget what I did.

Having said that, I came up with an optimization that might be interesting: using startsWith as a preliminary test before matching every code block lang string with the regular expression.

Intuitively this sounds more performant, but I haven't come up with a way to benchmark to confirm it would indeed be better.

Thanks!

wardpeet · 2019-03-15T09:34:22Z

packages/gatsby-remark-graphviz/package.json

@@ -8,6 +8,7 @@
  },
  "dependencies": {
    "@babel/runtime": "^7.0.0",
+    "cheerio": "^1.0.0-rc.2",


I'm not sure we want to include this dependency as it has a lot of dependencies. I'm a bit worried that this will introduce some security issues in the long ron

@wardpeet several of the other gatsby-remark plugins also use cheerio https://github.com/gatsbyjs/gatsby/search?q=cheerio&unscoped_q=cheerio so this isn't a big deal

packages/gatsby-remark-graphviz/src/index.js

raulrpearson · 2019-03-15T10:52:03Z

Thanks for your comments, @wardpeet. What would you like us to do with this PR? Some options:

Discard this PR and instead add this custom SVG attributes feature directly to viz.js. What do you think about this, @mdaines? I'm not sure I'd have the bandwidth to contribute in that way right now. This will still require a change to gatsby-remark-graphviz to account for that new API.
Go ahead with this PR but in a way that doesn't require adding cheerio as a dependency. I too am in favour of adding the leanest possible dependencies. Is your concern mainly about dependency size, performance, security or all three? Anything else? Why are you specifically concerned about security issues? If we chose this option, any recommendations on what to use instead of cheerio? I did some research of alternative HTML/XML parsers/manipulators, but I struggled to make sense of all the options.

The commit message provided some indication of the motivation and thought process for this PR:

This change was promted by the conversation in gatsbyjs/gatsby/#9480. Fixes gatsbyjs/gatsby/#9271. The main problem was the lack of responsiveness from the fixed size SVGs produced by Viz.js.

The referenced issue and PR remain open. The main problem was lack of responsiveness of the resulting SVGs. I think that adding support for custom SVG attributes would be a great way to solve that issue, with the bonus of adding extra flexibility for users of the plugin.

I'm in favour of option 2, but I'll understand if you prefer to drop the PR. Let me know what you think and how you'd like to proceed.

wardpeet · 2019-03-15T11:12:07Z

it would be great if could go for option 1 but instead of discarding put everything in place so we can use an updated viz version.

I can make a PR in viz.js.
Lets go for this api for now

const svgString = await viz.renderString(value, { engine: lang, svgAttributes: ourAttrs })

raulrpearson · 2019-03-15T11:25:48Z

OK, I think I'll be able to work on this during the weekend.

raulrpearson · 2019-03-26T08:29:30Z

@wardpeet, how are we doing on this? I just checked the viz.js repo and it says it's been archived:

This repository has been archived by the owner. It is now read-only.

Which means that putting in that PR into viz.js might no longer be possible. viz.js' README seems to recommend Dagre instead, but that probably belongs in a new plugin.

What do you think? Should we abandon this PR or finish it off as best we can?

wardpeet · 2019-03-26T13:03:18Z

This is some unfortunate news, it might need us to do a hack as well. let met get that in and we get this shipped! 👍

LekoArts · 2019-04-03T12:54:36Z

Hi @wardpeet 👋
Did you mean to merge this? 😊

wardpeet · 2019-04-03T20:41:58Z

@raulrpearson when I ran this PR it didn't compile graphviz sources with cheerio so any chance you can update this? Or perhaps it's something I did wrong

raulrpearson · 2019-04-05T03:30:30Z

when I ran this PR it didn't compile graphviz sources with cheerio so any chance you can update this?

I'm not sure what you mean, @wardpeet. By graphviz sources do you mean dot code blocks in Markdown files that should be turned into embedded SVGs?

I tested the code on a local Gatsby site, but that was a while ago so this might have changed. I'll see if I can reproduce, but I'd appreciate if you could clarify/expand 😊

KyleAMathews · 2019-04-24T04:55:34Z

Is there a site that we can test this against?

wardpeet · 2019-04-24T06:04:00Z

@raulrpearson if I remember correctly I got errors on gatsbyjs.org when running this PR.

raulrpearson · 2019-04-24T06:23:27Z

Is there a site that we can test this against?

Locally I added a page to examples/using-remark to test with a couple of examples. I stashed it at the time because I thought it didn't make sense to PR until the updated version of the plugin had shipped.

if I remember correctly I got errors on gatsbyjs.org when running this PR

I'll look into reproducing.

If you guys can give me some guidance as to what to do next or how to help move this forward, I can make some time for it.

raulrpearson · 2019-04-24T06:43:38Z

So, I think my gatsbyjs fork on GitHub is up to date. My local repo is also up to date. Do I need to rebase this branch to the latest commit on master?

Would you like me to commit my using-remark example in a new branch so that you can test the plugin yourselves?

raulrpearson · 2019-05-06T09:27:01Z

@KyleAMathews, @wardpeet I'd love to complete this PR one day 😅 . No pressure, I know you guys are busy.

If you can give me some guidance on how to proceed---i.e. how/where to set up "a site that we can test this against", how to reproduce those gatsby.org errors you were running up against---I can do the work required to get this PR ready for merge 🙏

One situation I'm not sure how to go about is that the latest commit in this branch is from February. I can update my local master branch to the latest commit but this branch will still be in the past, so I can't really test my changes against the latest gatsby.org.

That's why I asked about rebasing the branch to the latest commit on master. But I've read that one should not rebase commits that have made their way to other people's computers.

So, do I instead create a new commit on this PR's branch by merging the latest commit from master into it? That seems to make more sense.

…vg-attributes

KyleAMathews · 2019-05-18T01:59:48Z

@raulrpearson could you add a page to using-remark that we could test this against?

This commit adds a new page to the using-remark example to demo the new features for gatsby-remark-graphviz. I tested this (using gatsby-dev) with my local dev versions of gatsby and gatsby-remark-graphviz: - gatsby@2.4.2-dev-1558152684586 - gatsby-remark-graphviz@1.0.9-dev-1558152684586 The versions provided in the package.json with this commit won't render the new graphviz example correctly. I guess we should update these versions (mainly gatsby-remark-graphviz) before merging into master?

KyleAMathews · 2019-05-29T00:20:13Z

@wardpeet could you review this when you have a chance?

wardpeet · 2019-06-03T08:53:15Z

👍 going through my emails and i'll finish this one up.

wardpeet

Works like a charm! Testing it on gatsbyjs.org to make sure we don't break anything.

raulrpearson · 2019-06-03T12:32:12Z

Thanks for spending the time to review and merge this PR 🙏 I hope people find this useful!

…ng (gatsbyjs#11624) let's get this merged, thanks for your patience!

wardpeet reviewed Mar 15, 2019

View reviewed changes

packages/gatsby-remark-graphviz/src/index.js Show resolved Hide resolved

wardpeet added the status: awaiting author response Additional information has been requested from the author label Mar 15, 2019

LekoArts removed the status: awaiting author response Additional information has been requested from the author label Apr 3, 2019

KyleAMathews mentioned this pull request Apr 24, 2019

feat(gatsby-remark-graphviz): wrap svg in element so we can style it #9480

Closed

Merge remote-tracking branch 'upstream/master' into topics/graphviz-s…

081b0c8

…vg-attributes

wardpeet self-assigned this May 6, 2019

wardpeet approved these changes Jun 3, 2019

View reviewed changes

wardpeet merged commit e64ac14 into gatsbyjs:master Jun 3, 2019

raulrpearson deleted the topics/graphviz-svg-attributes branch June 3, 2019 12:31

johno pushed a commit to johno/gatsby that referenced this pull request Jul 17, 2019

feat(gatsby-remark-graphviz): custom SVG attributes and default styli…

42f718b

…ng (gatsbyjs#11624) let's get this merged, thanks for your patience!

This was referenced Mar 7, 2020

[Snyk] Security upgrade eslint from 4.19.1 to 6.0.0 ajesse11x/gatsby#117

Open

[Snyk] Security upgrade eslint from 4.19.1 to 6.0.0 saurabharch/gatsby#7

Open

saurabharch mentioned this pull request Nov 30, 2023

[Snyk] Fix for 1 vulnerabilities saurabharch/gatsby#2433

Open

ajesse11x mentioned this pull request Dec 1, 2023

[Snyk] Fix for 1 vulnerabilities ajesse11x/gatsby#2291

Open

MaxMood96 mentioned this pull request Dec 1, 2023

[Snyk] Security upgrade eslint from 5.16.0 to 6.0.0 MaxMood96/gatsby#1246

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(gatsby-remark-graphviz): custom SVG attributes and default styling #11624

feat(gatsby-remark-graphviz): custom SVG attributes and default styling #11624

raulrpearson commented Feb 7, 2019

raulrpearson commented Feb 21, 2019

wardpeet Mar 15, 2019

KyleAMathews May 18, 2019

raulrpearson commented Mar 15, 2019

wardpeet commented Mar 15, 2019

raulrpearson commented Mar 15, 2019

raulrpearson commented Mar 26, 2019

wardpeet commented Mar 26, 2019

LekoArts commented Apr 3, 2019

wardpeet commented Apr 3, 2019

raulrpearson commented Apr 5, 2019

KyleAMathews commented Apr 24, 2019

wardpeet commented Apr 24, 2019

raulrpearson commented Apr 24, 2019

raulrpearson commented Apr 24, 2019

raulrpearson commented May 6, 2019

KyleAMathews commented May 18, 2019

KyleAMathews commented May 29, 2019

wardpeet commented Jun 3, 2019

wardpeet left a comment

raulrpearson commented Jun 3, 2019

feat(gatsby-remark-graphviz): custom SVG attributes and default styling #11624

feat(gatsby-remark-graphviz): custom SVG attributes and default styling #11624

Conversation

raulrpearson commented Feb 7, 2019

Description

Related Issues

raulrpearson commented Feb 21, 2019

wardpeet Mar 15, 2019

Choose a reason for hiding this comment

KyleAMathews May 18, 2019

Choose a reason for hiding this comment

raulrpearson commented Mar 15, 2019

wardpeet commented Mar 15, 2019

raulrpearson commented Mar 15, 2019

raulrpearson commented Mar 26, 2019

wardpeet commented Mar 26, 2019

LekoArts commented Apr 3, 2019

wardpeet commented Apr 3, 2019

raulrpearson commented Apr 5, 2019

KyleAMathews commented Apr 24, 2019

wardpeet commented Apr 24, 2019

raulrpearson commented Apr 24, 2019

raulrpearson commented Apr 24, 2019

raulrpearson commented May 6, 2019

KyleAMathews commented May 18, 2019

KyleAMathews commented May 29, 2019

wardpeet commented Jun 3, 2019

wardpeet left a comment

Choose a reason for hiding this comment

raulrpearson commented Jun 3, 2019