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

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

Closed
wants to merge 3 commits into from
Closed

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

wants to merge 3 commits into from

Conversation

emtei
Copy link
Contributor

@emtei emtei commented Oct 27, 2018

Added a wrapper for all svg's generated by gatsby-remark-graphviz so they can be styled.
Added global styling for [www] that prevents graphs to be wider than viewport;

Thank you to @pieh and @Yurickh for help.

closes #9271

@emtei emtei requested a review from a team as a code owner October 27, 2018 16:39
@pieh pieh changed the title fixes #9271 [www] Graphviz elements not responsive feat(gatsby-remark-graphviz): wrap svg in element so we can style it Oct 27, 2018
Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

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

Left a few comments; this is looking good!

@@ -348,6 +348,10 @@ const _options = {
".gatsby-resp-image-link + em a[href*='//']:after": {
content: `" " url("data:image/svg+xml,%3Csvg%20xmlns='http://www.w3.org/2000/svg'%20class='i-external'%20viewBox='0%200%2032%2032'%20width='14'%20height='14'%20fill='none'%20stroke='%23744C9E'%20stroke-linecap='round'%20stroke-linejoin='round'%20stroke-width='9.38%'%3E%3Cpath%20d='M14%209%20L3%209%203%2029%2023%2029%2023%2018%20M18%204%20L28%204%2028%2014%20M28%204%20L14%2018'/%3E%3C/svg%3E")`,
},
".gatsby-graphviz-container": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since these styles will (always?) need to be applied, why don't we apply them at the plugin level, i.e. something like in gatsby-remark-autolink-headers

Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, probably want to add a className argument so this can be customized (but defaulted) just like in that plugin! It'd be a simple fix in gatsby-ssr.js and index.js!

@LekoArts
Copy link
Contributor

LekoArts commented Jan 9, 2019

@emtei Hi 👋 Do you have time to address the comments? If not I could finish your PR and get it merged.

@emtei
Copy link
Contributor Author

emtei commented Jan 11, 2019

@LekoArts Hi, thanks for reminder! I will sort this out soon, please wait a while.

@LekoArts
Copy link
Contributor

@emtei Cool! Please let me know if you need assistance 👍

@LekoArts
Copy link
Contributor

LekoArts commented Feb 2, 2019

@emtei what‘s the status?

@raulrpearson
Copy link
Contributor

Hey, I might be missing something, but are you guys sure this code is achieving what's intended? Testing this approach with one of the graphs in the docs:

divsvg

Using the div wrapper approach, the CSS rule that works - if I'm not mistaken - targets the svg child itself not the wrapping div:

div.gatsby-graphviz-container > svg {
  max-width: 100%;
  height: auto;
}

I'm with @hasparus on the idea of removing width and height attributes off the SVG instead of adding a wrapper. These are being introduced by Viz.js, though, and there doesn't seem to be an API to overwrite or remove them. The SVG string could be stripped of those attributes before being saved into the node.value.

Although having a wrapper does help with styling. An alternative/complementary approach would be to read the meta field of the code node in the Markdown AST to enable the user to add things like class="myclass" and id="myid". Assuming this is possible, it would allow more flexibility and better CSS targeting.

The three strategies could work together:

  1. Removing height and width from the svg string
  2. Adding a div wrapper
  3. Reading the meta field if present to provide better targeting

Let me know what you think and if I can help out and how.

@pieh
Copy link
Contributor

pieh commented Feb 5, 2019

@raulrpearson if you can work out manipulating generated SVG html string to remove hardcoded width/height (and maybe replace it with viewBox - not sure if it's needed it's been a while since I worked with svgs :P) and add class (just so people have extra option for styling) that would be great

@raulrpearson
Copy link
Contributor

[...] and maybe replace it with viewBox - not sure if it's needed

I think you're right, it is needed, but luckily viz.js seems to be adding that already.

and add class (just so people have extra option for styling)

I'm not sure which class you're referring to here. One class would be on the wrapper div which @emtei already worked on (pending the changes suggested by @DSchau). [A]

Another would be potentially added by the user on a code-block by code-block basis by using the following syntax in their Markdown file:

```dot id="my-id" class="my-class" style="color: tomato;" data-gocrazy="here"
strict graph {
  a -- b
  a -- b
  b -- a [color=blue]
}
```

If I'm not mistaken, the part id="my-id" ... data-gocrazy="here" would end up in the meta field of the code node of the mdast and we could retrieve it and paste it into the svg tag (maybe as is?). [B]

So, are you referring to A or B?

Additionally, I need some guidance regarding what to do with Git/GitHub. Do I clone this branch (emtei:graphviz-elements-styling) into my fork of gatsby on GitHub and then work locally against that and then push to this branch?

I'll be able to work on this tomorrow.

@pieh
Copy link
Contributor

pieh commented Feb 5, 2019

@raulrpearson For the class I meant we could add by default gatsby-graphviz class to make it easy to target it globally. But I do like your idea of allowing more customization per code/viz block.

This plugin https://github.com/DSchau/gatsby-remark-code-titles does transform code "language" to enable passing metadata (to add header to code block) with

```js:title=example-file.js

so you could look into implementation details there (but it seems like string is saved as-is in AST node as node.lang)

As far as GitHub - I think it would make sense to branch from master and create separate PR because there wouldn't be much of overlap - we can still merge this with just adding div wrapper around svg, which wouldn't affect your PR that much (maybe would just require fixing potential merge conflict)

@KyleAMathews
Copy link
Contributor

This PR was replaced by #11624

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.

[www] Graphviz elements not responsive
7 participants