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

Use typings for Styles #4487

Open
sidharthv96 opened this issue Jun 13, 2023 · 22 comments
Open

Use typings for Styles #4487

sidharthv96 opened this issue Jun 13, 2023 · 22 comments
Labels
Area: Development Status: Pending Is not to be executed as it currently is

Comments

@sidharthv96
Copy link
Member

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 #4486 (comment)

@github-actions github-actions bot added the Status: Triage Needs to be verified, categorized, etc label Jun 13, 2023
@sidharthv96
Copy link
Member Author

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 sidharthv96 added Status: Approved Is ready to be worked on Good first issue! Area: Development Status: Awaiting PR and removed Status: Triage Needs to be verified, categorized, etc labels Jun 13, 2023
@sidharthv96

This comment was marked as outdated.

@Yokozuna59

This comment was marked as outdated.

@Aryangp

This comment was marked as resolved.

@Aryangp
Copy link

Aryangp commented Aug 16, 2023

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

@huynhicode
Copy link
Member

@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
Copy link
Member Author

@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
Copy link
Member

Oops, sorry - my bad @sidharthv96 @Aryangp !

@Aryangp
Copy link

Aryangp commented Aug 17, 2023

Ok @sidharthv96 thanks for guidance I start working on this

@Aryangp
Copy link

Aryangp commented Aug 17, 2023

No problem @huynhicode

Oops, sorry - my bad @sidharthv96 @Aryangp !

@Aryangp
Copy link

Aryangp commented Aug 17, 2023

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
Copy link
Member

@Aryangp

import git from './diagrams/git/styles.js';

export const addStylesForDiagram = (type: string, diagramTheme?: DiagramStylesProvider): void => {
if (diagramTheme !== undefined) {
themes[type] = diagramTheme;
}
};

@Aryangp
Copy link

Aryangp commented Aug 17, 2023

Thanks for the reference @Yokozuna59

@Aryangp
Copy link

Aryangp commented Aug 20, 2023

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
Copy link
Member Author

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

@Aryangp
Copy link

Aryangp commented Aug 21, 2023

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
Copy link
Member Author

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

We have the following requirements

  • No runtime size increase
  • Type safe CSS properties

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

@Aryangp
Copy link

Aryangp commented Aug 22, 2023

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
Copy link
Member Author

sidharthv96 commented Aug 22, 2023

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.

@sidharthv96 sidharthv96 added Status: Pending Is not to be executed as it currently is and removed Status: Approved Is ready to be worked on Good first issue! Status: Awaiting PR labels Aug 22, 2023
@Aryangp
Copy link

Aryangp commented Aug 22, 2023

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
Copy link
Member Author

sidharthv96 commented Aug 22, 2023

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 #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
Copy link

Aryangp commented Aug 23, 2023

Sure I love to work on issue #4756 @sidharthv96

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Development Status: Pending Is not to be executed as it currently is
Projects
None yet
Development

No branches or pull requests

4 participants