search

mermaid-js / mermaid

Generation of diagrams like flowcharts or sequence diagrams from text in a similar manner as markdown
https://mermaid.js.org
MIT License
72.28k stars 6.58k forks source link

Use typings for Styles #4487

Open sidharthv96 opened 1 year ago

sidharthv96 commented 1 year ago

Do you have anything on your mind about strict styling definitions? I’m sure that the current approach of writing CSS code as strings with no type safety or highlighting or etc.

I came across csstypes and vanilla-extract, not sure about bundling size or limitations. Do you have something in mind? I’m almost done with pie chart files, but I'm not sure what to do with the styles.js file yet.

_Originally posted by @Yokozuna59 in https://github.com/mermaid-js/mermaid/pull/4486#discussion_r1227179385_

sidharthv96 commented 1 year ago

I think vanilla-extract looks great, and we can use that as it won't be bundled because it's a build time dependency.

sidharthv96 commented 1 year ago

@Yokozuna59, I can assign this to you specifically if you'd like to, or we can leave it open for anyone else to pick as you can focus on the 2 ongoing PRs.

Yokozuna59 commented 1 year ago

@Yokozuna59, I can assign this to you specifically if you'd like to, or we can leave it open for anyone else to pick as you can focus on the 2 ongoing PRs.

Although I'd be happy to do so, but I guess I'll focusing on #4486 for now, so let's leave it for the community. I guess I'll some types for styling in each diagram so it could be starter code.

Aryangp commented 1 year ago

hey can i look into this issue @sidharthv96 @Yokozuna59

Aryangp commented 1 year ago

ok @sidharthv96 can you just give me bit more overview and assign me this issue

huynhicode commented 1 year ago

@Aryangp I have assigned this issue to you - thanks for your contribution!

Perhaps @sidharthv96 and/or @Yokozuna59 can provide more insight into this issue.

sidharthv96 commented 1 year ago

@Aryangp I didn't assign it to you as all issues without PRs are open for everyone to try, there is no need to ask :) We've had old issues that were assigned to people, who stopped working on it, and nobody picked it up later as it was already assigned.

@Aryangp you can get started with trying out vanilla-extract. The goal is to remove writing CSS as string templates, like we see in most styles.js.

To get started, you can convert the git/styles.js into a properly typed implementation with vanilla-extract, raise a PR and let's review that.

huynhicode commented 1 year ago

Oops, sorry - my bad @sidharthv96 @Aryangp !

Aryangp commented 1 year ago

Ok @sidharthv96 thanks for guidance I start working on this

Aryangp commented 1 year ago

No problem @huynhicode

Oops, sorry - my bad @sidharthv96 @Aryangp !

Aryangp commented 1 year ago

hey @sidharthv96 can you tell me this file git/styles in this the export getStyles is used in which file as i looked this one is not used in any file

Yokozuna59 commented 1 year ago

@Aryangp

https://github.com/mermaid-js/mermaid/blob/7b7e281ec79a6fb006bb4aefc716fafb1f233fb8/packages/mermaid/src/styles.spec.ts#L22

https://github.com/mermaid-js/mermaid/blob/7b7e281ec79a6fb006bb4aefc716fafb1f233fb8/packages/mermaid/src/diagram-api/diagram-orchestration.ts#L79

https://github.com/mermaid-js/mermaid/blob/7b7e281ec79a6fb006bb4aefc716fafb1f233fb8/packages/mermaid/src/styles.ts#L77-L81

Aryangp commented 1 year ago

Thanks for the reference @Yokozuna59

Aryangp commented 1 year ago

hey @sidharthv96 which bundler we use in package/mermaid as i have to add this dependency pnpm install --save-dev @vanilla-extract/webpack-plugin or pnpm install --save-dev @vanilla-extract/esbuild-plugin

sidharthv96 commented 1 year ago

In develop, we are using vite. In next (upcoming v11), we'll be using esbuild and vite.

Aryangp commented 1 year ago

ok then i will vite now and there is one more thing i want show you the approch i an thinking of taking as currently we use styles like this 

const getStyles=(options)=>
    `
    .${options[0]}{
      fill: lightgrey;
      color: lightgrey;
    }
    `;

    console.log(getStyles(["container"]))

This is just a example that we use

now what is think is like this 

const getStyles=(options)=>
   const var1=options[0];
   const var1=style({
        fill: lightgrey;
      color: lightgrey;
})

use this approch to type check it if you think it hinder the functioning than please let me know and any other input you have @sidharthv96 @Yokozuna59

sidharthv96 commented 1 year ago

Upon further inspection, it seems like vanilla-extract might not be the best fit for us.

We have the following requirements

csstype does seem to satisfy both, and might be the right fit for our particular usecase.

Aryangp commented 1 year ago

Ok than its best to try out with css types for now and see how it work and I will also try to find there is any other way than this two that we can use @sidharthv96

sidharthv96 commented 1 year ago

Even with csstype, converting our CSS didn't have a clean/easy approach (I tried on gitstyles). And even if we manage to pull off a conversion, we don't have the ability nor the capacity to verify the changes. So, I recommend stopping work on this for the moment, and focusing on something that would help us catch more bugs, like converting JS files into TS.

Aryangp commented 1 year ago

Ok no problem if you can tell me some issues like one that you mention above js to ts one there no on which I can start working on would be great @sidharthv96

sidharthv96 commented 1 year ago

For future reference, this is the POC I tried, and I am not convinced that was a suitable approach. So, once we figure out a nicer way, we can proceed. 

--- a/packages/mermaid/src/diagrams/git/styles.js
+++ b/packages/mermaid/src/diagrams/git/styles.ts
@@ -1,19 +1,21 @@
-const getStyles = (options) =>
-  `
-  .commit-id,
-  .commit-msg,
-  .branch-label {
-    fill: lightgrey;
-    color: lightgrey;
-    font-family: 'trebuchet ms', verdana, arial, sans-serif;
-    font-family: var(--mermaid-font-family);
-  }
+import type * as CSS from 'csstype';
+
+// JSON.stringify is a placeholder, it will not generate valid CSS.
+const css = (style: CSS.Properties) => JSON.stringify(style);
+
+const getStyles = (options: any) => {
+  return `
+  .branch-label, .commit-id, .commit-msg ${css({
+    fill: 'lightgrey',
+    color: 'lightgrey',
+    fontFamily: "var(--mermaid-font-family) 'trebuchet ms', verdana, arial, sans-serif",
+  })}
   ${[0, 1, 2, 3, 4, 5, 6, 7]
     .map(
       (i) =>
         `
-        .branch-label${i} { fill: ${options['gitBranchLabel' + i]}; }
-        .commit${i} { stroke: ${options['git' + i]}; fill: ${options['git' + i]}; }
+        .branch-label${i} ${css({ fill: options['gitBranchLabel' + i] })}
+        .commit${i} ${css({ stroke: options['git' + i], fill: options['git' + i] })}
         .commit-highlight${i} { stroke: ${options['gitInv' + i]}; fill: ${options['gitInv' + i]}; }
         .label${i}  { fill: ${options['git' + i]}; }
         .arrow${i} { stroke: ${options['git' + i]}; }
@@ -57,5 +59,5 @@ const getStyles = (options) =>
     fill: ${options.textColor};
   }
 `;
-
+};
 export default getStyles;

@Aryangp I'd really like some help on https://github.com/mermaid-js/mermaid/issues/4756 if you don't mind. And here is the list of all JS files, ordered by size. 

ls -laShRr packages/mermaid/src/**/*.js

147B src/diagrams/c4/styles.js
156B src/themes/theme-helpers.js
179B src/dagre-wrapper/intersect/intersect-node.js
223B src/dagre-wrapper/intersect.js
235B src/dagre-wrapper/intersect/intersect-circle.js
260B src/diagrams/state/id-cache.js
360B src/dagre-wrapper/intersect/index.js
519B src/diagrams/git/layout.js
588B src/dagre-wrapper/intersect/intersect-ellipse.js
718B src/themes/index.js
719B src/dagre-wrapper/intersect/intersect-rect.js
876B src/setupGraphViewbox.spec.js
918B src/diagrams/state/stateRenderer-v2.spec.js
949B src/diagrams/requirement/styles.js
968B src/diagrams/er/styles.js
1.0K src/dagre-wrapper/shapes/note.js
1.4K src/dagre-wrapper/patterns.js
1.6K src/diagrams/c4/parser/c4Diagram.spec.js
1.8K src/diagrams/requirement/requirementMarkers.js
1.8K src/dagre-wrapper/intersect/intersect-polygon.js
1.8K src/diagrams/timeline/styles.js
1.9K src/dagre-wrapper/edges.spec.js
1.9K src/diagrams/mindmap/styles.js
1.9K src/diagrams/class/classDiagram-styles.spec.js
2.0K src/dagre-wrapper/intersect/intersect-line.js
2.1K src/diagrams/flowchart/parser/flow-md-string.spec.js
2.2K src/diagrams/flowchart/flowDb.spec.js
2.3K src/diagrams/sequence/styles.js
2.3K src/diagrams/er/erDb.js
2.3K src/diagrams/timeline/timelineDb.js
2.5K src/diagrams/flowchart/parser/flow-direction.spec.js
2.6K src/setupGraphViewbox.js
2.6K src/diagrams/c4/parser/c4Boundary.spec.js
2.6K src/diagrams/user-journey/journeyDb.js
2.6K src/diagrams/c4/parser/c4Person.spec.js
2.7K src/diagrams/class/styles.js
2.8K src/diagrams/user-journey/styles.js
2.8K src/dagre-wrapper/createLabel.js
2.8K src/diagrams/user-journey/journeyDb.spec.js
2.8K src/diagrams/state/stateDb.spec.js
2.9K src/diagrams/c4/parser/c4PersonExt.spec.js
3.3K src/diagrams/c4/parser/c4System.spec.js
3.3K src/diagrams/timeline/timeline.spec.js
3.4K src/diagrams/requirement/requirementDb.js
3.5K src/diagrams/c4/parser/c4Container.spec.js
3.5K src/diagrams/flowchart/parser/flow-lines.spec.js
3.6K src/diagrams/mindmap/mindmapDb.js
3.6K src/diagrams/git/mockDb.js
3.8K src/diagrams/flowchart/flowChartShapes.spec.js
3.8K src/diagrams/state/styles.js
4.2K src/dagre-wrapper/shapes/util.js
4.3K src/diagrams/user-journey/parser/journey.spec.js
4.5K src/diagrams/er/erMarkers.js
4.6K src/diagrams/gantt/styles.js
4.9K src/diagrams/flowchart/flowRenderer.addEdges.spec.js
5.0K src/diagrams/flowchart/parser/flow-comments.spec.js
5.3K src/diagrams/state/parser/state-parser.spec.js
5.5K src/diagrams/mindmap/mindmapRenderer.js
5.8K src/diagrams/flowchart/parser/flow-interactions.spec.js
5.8K src/diagrams/flowchart/parser/flow.spec.js
6.1K src/diagrams/sequence/svgDraw.spec.js
6.5K src/dagre-wrapper/index.js
6.6K src/diagrams/gantt/parser/gantt.spec.js
7.2K src/diagrams/class/classRenderer.js
7.2K src/dagre-wrapper/markers.js
7.3K src/diagrams/flowchart/parser/flow-vertice-chaining.spec.js
7.3K src/dagre-wrapper/clusters.js
8.3K src/diagrams/flowchart/flowChartShapes.js
8.3K src/diagrams/state/stateRenderer.js
8.3K src/diagrams/flowchart/flowRenderer.spec.js
9.1K src/diagrams/flowchart/parser/flow-arrows.spec.js
9.1K src/diagrams/state/parser/state-style.spec.js
9.6K src/diagrams/requirement/requirementRenderer.js
9.8K src/diagrams/user-journey/svgDraw.js
 10K src/diagrams/git/gitGraphRenderer-old.js
 10K src/diagrams/flowchart/parser/subgraph.spec.js
 11K src/diagrams/mindmap/svgDraw.js
 11K src/diagrams/state/stateDiagram.spec.js
 11K src/diagrams/class/svgDraw.spec.js
 11K src/diagrams/state/stateDiagram-v2.spec.js
 11K src/diagrams/flowchart/parser/flow-singlenode.spec.js
 12K src/diagrams/mindmap/mindmap.spec.js
 12K src/diagrams/flowchart/parser/flow-style.spec.js
 13K src/diagrams/class/svgDraw.js
 14K src/dagre-wrapper/mermaid-graphlib.js
 14K src/diagrams/timeline/svgDraw.js
 14K src/themes/theme-neutral.js
 15K src/diagrams/state/stateRenderer-v2.js
 15K src/diagrams/flowchart/flowRenderer.js
 15K src/dagre-wrapper/mermaid-graphlib.spec.js
 15K src/diagrams/flowchart/flowRenderer-v2.js
 15K src/themes/theme-dark.js
 15K src/themes/theme-forest.js
 15K src/diagrams/state/shapes.js
 15K src/diagrams/state/stateDb.js
 16K src/diagrams/git/gitGraphAst.js
 16K src/themes/theme-default.js
 17K src/diagrams/sequence/sequenceDb.js
 17K src/themes/theme-base.js
 18K src/diagrams/flowchart/parser/flow-edges.spec.js
 19K src/diagrams/requirement/parser/requirementDiagram.spec.js
 19K src/diagrams/flowchart/flowDb.js
 19K src/dagre-wrapper/edges.js
 19K src/diagrams/gantt/ganttDb.js
 20K src/diagrams/git/gitGraphRenderer.js
 20K src/diagrams/c4/c4Db.js
 21K src/diagrams/flowchart/parser/flow-text.spec.js
 21K src/diagrams/gantt/ganttRenderer.js
 22K src/diagrams/c4/c4Renderer.js
 23K src/diagrams/er/erRenderer.js
 28K src/dagre-wrapper/nodes.js
 28K src/diagrams/git/gitGraphParserV2.spec.js
 29K src/diagrams/flowchart/elk/flowRenderer-elk.js
 30K src/diagrams/er/parser/erDiagram.spec.js
 34K src/diagrams/c4/svgDraw.js
 40K src/diagrams/sequence/svgDraw.js
 66K src/diagrams/sequence/sequenceDiagram.spec.js
281K src/diagrams/flowchart/parser/flow-huge.spec.js
Aryangp commented 1 year ago

Sure I love to work on issue #4756 @sidharthv96