From 104aece46e99699bacd0a844964cc12cd89844b1 Mon Sep 17 00:00:00 2001 From: Nikolay Rozhkov Date: Thu, 22 Jun 2023 17:35:40 +0300 Subject: [PATCH] Cleanup sankey diagrams according code review --- demos/sankey.html | 1 - .../diagrams/sankey/parser/desired_syntax.md | 154 ------------------ .../sankey/parser/sankey-arrow.spec.js | 64 +------- .../src/diagrams/sankey/parser/sankey.spec.ts | 9 +- .../mermaid/src/diagrams/sankey/sankeyDB.ts | 9 +- .../src/diagrams/sankey/sankeyDiagram.ts | 12 -- .../src/diagrams/sankey/sankeyRenderer.ts | 30 ++-- .../src/diagrams/sankey/sankeyUtils.ts | 8 + packages/mermaid/src/setupGraphViewbox.js | 1 - 9 files changed, 24 insertions(+), 264 deletions(-) delete mode 100644 packages/mermaid/src/diagrams/sankey/parser/desired_syntax.md create mode 100644 packages/mermaid/src/diagrams/sankey/sankeyUtils.ts diff --git a/demos/sankey.html b/demos/sankey.html index a7d4d5537..e597c408f 100644 --- a/demos/sankey.html +++ b/demos/sankey.html @@ -21,7 +21,6 @@ Agricultural 'waste',Bio-conversion,124.729 Bio-conversion,Liquid,0.597 - %% line with comment Bio-conversion,Losses,26.862 Bio-conversion,Solid,280.322 Bio-conversion,Gas,81.144 diff --git a/packages/mermaid/src/diagrams/sankey/parser/desired_syntax.md b/packages/mermaid/src/diagrams/sankey/parser/desired_syntax.md deleted file mode 100644 index ccde1c59b..000000000 --- a/packages/mermaid/src/diagrams/sankey/parser/desired_syntax.md +++ /dev/null @@ -1,154 +0,0 @@ -# Sankey diagrams syntax proposal - -## What is used now - -**Circular graphs are not supported by d3. There are some alternatives for that.** -**Dropped flows are not supported by d3** - -This is example of data for Sakey diagrams from d3 author (simple csv): - -```csv -Berlin,Job Applications,102 -Barcelona,Job Applications,39 -Madrid,Job Applications,35 -Amsterdam,Job Applications,15 -Paris,Job Applications,14 -London,Job Applications,6 -Munich,Job Applications,5 -Brussels,Job Applications,4 -Dubai,Job Applications,3 -Dublin,Job Applications,3 -Other Cities,Job Applications,12 -Job Applications,No Response,189 -Job Applications,Responded,49,orange -Responded,Rejected,38 -Responded,Interviewed,11,orange -Interviewed,No Offer,8 -Interviewed,Declined Offer,2 -Interviewed,Accepted Offer,1,orange -``` - -GoJS uses similar approach: - -```json -{ -"nodeDataArray": [ -{"key":"Coal reserves", "text":"Coal reserves", "color":"#9d75c2"}, -{"key":"Coal imports", "text":"Coal imports", "color":"#9d75c2"}, -{"key":"Oil reserves", "text":"Oil\nreserves", "color":"#9d75c2"}, -{"key":"Oil imports", "text":"Oil imports", "color":"#9d75c2"} -], -"linkDataArray": [ -{"from":"Coal reserves", "to":"Coal", "width":31}, -{"from":"Coal imports", "to":"Coal", "width":86}, -{"from":"Oil reserves", "to":"Oil", "width":244} -} -``` - -## What do we need - -Mainly we need: - -- collection of nodes -- collection of links - -We also need graph and node attributes like this: - -- link sort -- node sort -- coloring strategy for links (source, target, transition) -- graph alignment (left, right, width) -- node color -- node title -- node width -- node padding -- graph margin - -## Desired syntax - -Graph is a list of flows (or links). -Flow is a sequence `node -> value -> node -> value...` - -``` -a -> 30 -> b -a -> 40 -> b -``` - -2 separate streams between 2 nodes they can be grouped as well: - -``` -a -> { - 30 - 40 -} -> b -``` - -All outflows from the node can be grouped: - -``` -a -> { - 30 -> b - 40 -> c -} -``` - -All inflows to the node can be grouped too: - -``` -{ - a -> 30 - b -> 40 -} -> c -``` - -Chaining example: - -``` -a -> { - 30 - 40 -} -> b -> { - 20 -> d -> 11 - 50 -> e -> 11 -} -> f -> 30 -``` - -**Probably ambiguous!** - -Does the sample below mean that total outflow from "a" is 60? - -``` -a -> 30 -> { - b - c -} -``` - -Or does this one mean that total outflow must be 140 (70 to "b" and "c" respectively)? - -``` -a -> { - 30 - 40 -} -> { - b - c -} -``` - -**Overcomplicated** - -Nested: - -``` -{ - { - a -> 30 - b -> 40 - } -> c -> 12 - { - d -> 10 - e -> 20 - } -> f -> 43 -} -> g -``` diff --git a/packages/mermaid/src/diagrams/sankey/parser/sankey-arrow.spec.js b/packages/mermaid/src/diagrams/sankey/parser/sankey-arrow.spec.js index 19f1ff357..a85a8d5c0 100644 --- a/packages/mermaid/src/diagrams/sankey/parser/sankey-arrow.spec.js +++ b/packages/mermaid/src/diagrams/sankey/parser/sankey-arrow.spec.js @@ -85,7 +85,7 @@ describe('Sankey diagram', function () { parser.parse(str); }); - it('parses truly complex example', () => { + it('parses real example', () => { const str = ` sankey @@ -95,68 +95,6 @@ describe('Sankey diagram', function () { "Bio-conversion" -> 280.322 -> "Solid" "Bio-conversion" -> 81.144 -> "Gas" "Biofuel imports" -> 35 -> "Liquid" - "Biomass imports" -> 35 -> "Solid" - "Coal imports" -> 11.606 -> "Coal" - "Coal reserves" -> 63.965 -> "Coal" - "Coal" -> 75.571 -> "Solid" - "District heating" -> 10.639 -> "Industry" - "District heating" -> 22.505 -> "Heating and cooling - commercial" - "District heating" -> 46.184 -> "Heating and cooling - homes" - "Electricity grid" -> 104.453 -> "Over generation / exports" - "Electricity grid" -> 113.726 -> "Heating and cooling - homes" - "Electricity grid" -> 27.14 -> "H2 conversion" - "Electricity grid" -> 342.165 -> "Industry" - "Electricity grid" -> 37.797 -> "Road transport" - "Electricity grid" -> 4.412 -> "Agriculture" - "Electricity grid" -> 40.858 -> "Heating and cooling - commercial" - "Electricity grid" -> 56.691 -> "Losses" - "Electricity grid" -> 7.863 -> "Rail transport" - "Electricity grid" -> 90.008 -> "Lighting & appliances - commercial" - "Electricity grid" -> 93.494 -> "Lighting & appliances - homes" - "Gas imports" -> 40.719 -> "Ngas" - "Gas reserves" -> 82.233 -> "Ngas" - "Gas" -> 0.129 -> "Heating and cooling - commercial" - "Gas" -> 1.401 -> "Losses" - "Gas" -> 151.891 -> "Thermal generation" - "Gas" -> 2.096 -> "Agriculture" - "Gas" -> 48.58 -> "Industry" - "Geothermal" -> 7.013 -> "Electricity grid" - "H2 conversion" -> 20.897 -> "H2" - "H2 conversion" -> 6.242 -> "Losses" - "H2" -> 20.897 -> "Road transport" - "Hydro" -> 6.995 -> "Electricity grid" - "Liquid" -> 121.066 -> "Industry" - "Liquid" -> 128.69 -> "International shipping" - "Liquid" -> 135.835 -> "Road transport" - "Liquid" -> 14.458 -> "Domestic aviation" - "Liquid" -> 206.267 -> "International aviation" - "Liquid" -> 3.64 -> "Agriculture" - "Liquid" -> 33.218 -> "National navigation" - "Liquid" -> 4.413 -> "Rail transport" - "Marine algae" -> 4.375 -> "Bio-conversion" - "Ngas" -> 122.952 -> "Gas" - "Nuclear" -> 839.978 -> "Thermal generation" - "Oil imports" -> 504.287 -> "Oil" - "Oil reserves" -> 107.703 -> "Oil" - "Oil" -> 611.99 -> "Liquid" - "Other waste" -> 56.587 -> "Solid" - "Other waste" -> 77.81 -> "Bio-conversion" - "Pumped heat" -> 193.026 -> "Heating and cooling - homes" - "Pumped heat" -> 70.672 -> "Heating and cooling - commercial" - "Solar PV" -> 59.901 -> "Electricity grid" - "Solar Thermal" -> 19.263 -> "Heating and cooling - homes" - "Solar" -> 19.263 -> "Solar Thermal" - "Solar" -> 59.901 -> "Solar PV" - "Solid" -> 0.882 -> "Agriculture" - "Solid" -> 400.12 -> "Thermal generation" - "Solid" -> 46.477 -> "Industry" - "Thermal generation" -> 525.531 -> "Electricity grid" - "Thermal generation" -> 787.129 -> "Losses" - "Thermal generation" -> 79.329 -> "District heating" - "Tidal" -> 9.452 -> "Electricity grid" - "UK land based bioenergy" -> 182.01 -> "Bio-conversion" - "Wave" -> 19.013 -> "Electricity grid" - "Wind" -> 289.366 -> "Electricity grid" `; parser.parse(str); diff --git a/packages/mermaid/src/diagrams/sankey/parser/sankey.spec.ts b/packages/mermaid/src/diagrams/sankey/parser/sankey.spec.ts index 873e99cb1..10a187622 100644 --- a/packages/mermaid/src/diagrams/sankey/parser/sankey.spec.ts +++ b/packages/mermaid/src/diagrams/sankey/parser/sankey.spec.ts @@ -4,10 +4,9 @@ import diagram from './sankey.jison'; import { parser } from './sankey.jison'; import db from '../sankeyDB.js'; import { cleanupComments } from '../../../diagram-api/comments.js'; -import { prepareTextForParsing } from '../sankeyDiagram.js'; +import { prepareTextForParsing } from '../sankeyUtils.js'; describe('Sankey diagram', function () { - // TODO - these examples should be put into ./parser/stateDiagram.spec.js describe('when parsing an info graph it', function () { beforeEach(function () { parser.yy = db; @@ -20,13 +19,7 @@ describe('Sankey diagram', function () { const path = await import('path'); const csv = path.resolve(__dirname, './energy.csv'); const data = fs.readFileSync(csv, 'utf8'); - - // Add \n\n + space to emulate possible possible imperfections const graphDefinition = prepareTextForParsing(cleanupComments('sankey\n\n ' + data)); - // const textToParse = graphDefinition - // .replaceAll(/^[^\S\r\n]+|[^\S\r\n]+$/g, '') - // .replaceAll(/([\n\r])+/g, "\n") - // .trim(); parser.parse(graphDefinition); }); diff --git a/packages/mermaid/src/diagrams/sankey/sankeyDB.ts b/packages/mermaid/src/diagrams/sankey/sankeyDB.ts index f3faf9c7a..f54034a3b 100644 --- a/packages/mermaid/src/diagrams/sankey/sankeyDB.ts +++ b/packages/mermaid/src/diagrams/sankey/sankeyDB.ts @@ -1,5 +1,3 @@ -// import { log } from '../../logger.js'; -// import mermaidAPI from '../../mermaidAPI.js'; import * as configApi from '../../config.js'; import common from '../common/common.js'; import { @@ -19,11 +17,13 @@ import { let links: Array = []; let nodes: Array = []; let nodesHash: Record = {}; -let nodeAlign: string = 'justify'; +let nodeAlign = 'justify'; const setNodeAlign = function (alignment: string): void { const nodeAlignments: string[] = ['left', 'right', 'center', 'justify']; - if (nodeAlignments.includes(alignment)) nodeAlign = alignment; + if (nodeAlignments.includes(alignment)) { + nodeAlign = alignment; + } }; const getNodeAlign = () => nodeAlign; @@ -96,7 +96,6 @@ export default { findOrCreateNode, setNodeAlign, getNodeAlign, - // TODO: If this is a must this probably should be an interface getAccTitle, setAccTitle, getAccDescription, diff --git a/packages/mermaid/src/diagrams/sankey/sankeyDiagram.ts b/packages/mermaid/src/diagrams/sankey/sankeyDiagram.ts index 7ec305db7..9c8fbaa2d 100644 --- a/packages/mermaid/src/diagrams/sankey/sankeyDiagram.ts +++ b/packages/mermaid/src/diagrams/sankey/sankeyDiagram.ts @@ -5,18 +5,6 @@ import db from './sankeyDB.js'; import styles from './styles.js'; import renderer from './sankeyRenderer.js'; -export const prepareTextForParsing = (text: string): string => { - const textToParse = text - .replaceAll(/^[^\S\r\n]+|[^\S\r\n]+$/g, '') // remove all trailing spaces for each row - .replaceAll(/([\n\r])+/g, '\n') // remove empty lines duplicated - .trim(); - - return textToParse; -}; - -const originalParse = parser.parse.bind(parser); -parser.parse = (text: string) => originalParse(prepareTextForParsing(text)); - export const diagram: DiagramDefinition = { parser, db, diff --git a/packages/mermaid/src/diagrams/sankey/sankeyRenderer.ts b/packages/mermaid/src/diagrams/sankey/sankeyRenderer.ts index 4956b6cf2..b034e8c24 100644 --- a/packages/mermaid/src/diagrams/sankey/sankeyRenderer.ts +++ b/packages/mermaid/src/diagrams/sankey/sankeyRenderer.ts @@ -1,14 +1,11 @@ // @ts-nocheck TODO: fix file import { Diagram } from '../../Diagram.js'; -// import { log } from '../../logger.js'; import * as configApi from '../../config.js'; import { select as d3select, scaleOrdinal as d3scaleOrdinal, schemeTableau10 as d3schemeTableau10, - // rgb as d3rgb, - // map as d3map, } from 'd3'; import { @@ -20,7 +17,7 @@ import { sankeyJustify as d3SankeyJustify, } from 'd3-sankey'; import { configureSvgSize } from '../../setupGraphViewbox.js'; -// import { debug } from 'console'; +import { prepareTextForParsing } from './sankeyUtils.js'; /** * Draws a sequenceDiagram in the tag with id: id based on the graph definition in text. @@ -31,20 +28,15 @@ import { configureSvgSize } from '../../setupGraphViewbox.js'; * @param diagObj - A standard diagram containing the db and the text and type etc of the diagram */ export const draw = function (text: string, id: string, _version: string, diagObj: Diagram): void { - // First of all parse sankey language - // Everything that is parsed will be stored in diagObj.DB - // That is why we need to clear DB first + // Clear DB before parsing // - if (diagObj?.db?.clear !== undefined) { - // why do we need to check for undefined? typescript complains - diagObj?.db?.clear(); - } + diagObj.db.clear?.(); + // Launch parsing - diagObj.parser.parse(text); - // debugger; - // log.debug('Parsed sankey diagram'); + const preparedText = prepareTextForParsing(text); + diagObj.parser.parse(preparedText); - // Figure out what is happening there + // TODO: Figure out what is happening there // The main thing is svg object that is a d3 wrapper for svg operations // const { securityLevel, sequence: conf } = configApi.getConfig(); @@ -95,10 +87,10 @@ export const draw = function (text: string, id: string, _version: string, diagOb }; const nodeAlign = nodeAligns[diagObj.db.getNodeAlign()]; - const nodeWidth = 10; // Construct and configure a Sankey generator // That will be a function that calculates nodes and links dimensions // + const nodeWidth = 10; const sankey = d3Sankey() .nodeId((d) => d.id) // we use 'id' property to identify node .nodeWidth(nodeWidth) @@ -109,10 +101,8 @@ export const draw = function (text: string, id: string, _version: string, diagOb [width - nodeWidth, height], ]); - // Compute the Sankey layout - // Namely calculate nodes and links positions - // Our `graph` object will be mutated by this - // and enriched with some properties + // Compute the Sankey layout: calculate nodes and links positions + // Our `graph` object will be mutated by this and enriched with other properties // sankey(graph); diff --git a/packages/mermaid/src/diagrams/sankey/sankeyUtils.ts b/packages/mermaid/src/diagrams/sankey/sankeyUtils.ts new file mode 100644 index 000000000..45ecf21dd --- /dev/null +++ b/packages/mermaid/src/diagrams/sankey/sankeyUtils.ts @@ -0,0 +1,8 @@ +export const prepareTextForParsing = (text: string): string => { + const textToParse = text + .replaceAll(/^[^\S\n\r]+|[^\S\n\r]+$/g, '') // remove all trailing spaces for each row + .replaceAll(/([\n\r])+/g, '\n') // remove empty lines duplicated + .trim(); + + return textToParse; +}; diff --git a/packages/mermaid/src/setupGraphViewbox.js b/packages/mermaid/src/setupGraphViewbox.js index 9294f8024..0396d654f 100644 --- a/packages/mermaid/src/setupGraphViewbox.js +++ b/packages/mermaid/src/setupGraphViewbox.js @@ -25,7 +25,6 @@ export const calculateSvgSizeAttrs = function (height, width, useMaxWidth) { if (useMaxWidth) { attrs.set('width', '100%'); attrs.set('style', `max-width: ${width}px;`); - // TODO: when using max width it does not set height? Is it intended? } else { attrs.set('height', height); attrs.set('width', width);