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.

Sign up for GitHub

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

SVG element selection refactoring #4609

Closed
wants to merge 1 commit into from
Closed

SVG element selection refactoring #4609

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 2 additions & 13 deletions packages/mermaid/src/diagrams/c4/c4Renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { select } from 'd3';
import svgDraw from './svgDraw.js';
import { log } from '../../logger.js';
import { parser } from './parser/c4Diagram.jison';
Expand All @@ -8,6 +7,7 @@ import * as configApi from '../../config.js';
import assignWithDepth from '../../assignWithDepth.js';
import { wrapLabel, calculateTextWidth, calculateTextHeight } from '../../utils.js';
import { configureSvgSize } from '../../setupGraphViewbox.js';
import { selectElementsForRender } from '../../rendering-util/selectElementsForRender.js';

let globalBoundaryMaxX = 0,
globalBoundaryMaxY = 0;
Expand Down Expand Up @@ -581,16 +581,6 @@ function drawInsideBoundary(
*/
export const draw = function (_text, id, _version, diagObj) {
conf = configApi.getConfig().c4;
const securityLevel = configApi.getConfig().securityLevel;
// Handle root and Document for when rendering in sandbox mode
let sandboxElement;
if (securityLevel === 'sandbox') {
sandboxElement = select('#i' + id);
}
const root =
securityLevel === 'sandbox'
? select(sandboxElement.nodes()[0].contentDocument.body)
: select('body');

let db = diagObj.db;

Expand All @@ -601,8 +591,7 @@ export const draw = function (_text, id, _version, diagObj) {

log.debug(`C:${JSON.stringify(conf, null, 2)}`);

const diagram =
securityLevel === 'sandbox' ? root.select(`[id="${id}"]`) : select(`[id="${id}"]`);
const { svg: diagram } = selectElementsForRender(id);

svgDraw.insertComputerIcon(diagram);
svgDraw.insertDatabaseIcon(diagram);
Expand Down
16 changes: 2 additions & 14 deletions packages/mermaid/src/diagrams/class/classRenderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import { select } from 'd3';
import { layout as dagreLayout } from 'dagre-d3-es/src/dagre/index.js';
import * as graphlib from 'dagre-d3-es/src/graphlib/index.js';
import { log } from '../../logger.js';
import svgDraw from './svgDraw.js';
import { configureSvgSize } from '../../setupGraphViewbox.js';
import { getConfig } from '../../config.js';
import { selectElementsForRender } from '../../rendering-util/selectElementsForRender.js';

let idCache = {};
const padding = 20;
Expand Down Expand Up @@ -144,19 +144,7 @@ export const draw = function (text, id, _version, diagObj) {

log.info('Rendering diagram ' + text);

const securityLevel = getConfig().securityLevel;
// Handle root and Document for when rendering in sandbox mode
let sandboxElement;
if (securityLevel === 'sandbox') {
sandboxElement = select('#i' + id);
}
const root =
securityLevel === 'sandbox'
? select(sandboxElement.nodes()[0].contentDocument.body)
: select('body');

// Fetch the default direction, use TD if none was found
const diagram = root.select(`[id='${id}']`);
const { svg: diagram } = selectElementsForRender(id);
insertMarkers(diagram);

// Layout graph, Create a new directed graph
Expand Down
18 changes: 3 additions & 15 deletions packages/mermaid/src/diagrams/er/erRenderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import * as graphlib from 'dagre-d3-es/src/graphlib/index.js';
import { line, curveBasis, select } from 'd3';
import { line, curveBasis } from 'd3';
import { layout as dagreLayout } from 'dagre-d3-es/src/dagre/index.js';
import { getConfig } from '../../config.js';
import { log } from '../../logger.js';
Expand All @@ -8,6 +8,7 @@ import erMarkers from './erMarkers.js';
import { configureSvgSize } from '../../setupGraphViewbox.js';
import { parseGenericTypes } from '../common/common.js';
import { v5 as uuid5 } from 'uuid';
import { selectElementsForRender } from '../../rendering-util/selectElementsForRender.js';

/** Regex used to remove chars from the entity name so the result can be used in an id */
const BAD_ID_CHARS_REGEXP = /[^\dA-Za-z](\W)*/g;
Expand Down Expand Up @@ -555,21 +556,8 @@ const drawRelationshipFromLayout = function (svg, rel, g, insert, diagObj) {
export const draw = function (text, id, _version, diagObj) {
conf = getConfig().er;
log.info('Drawing ER diagram');
// diag.db.clear();
const securityLevel = getConfig().securityLevel;
// Handle root and Document for when rendering in sandbox mode
let sandboxElement;
if (securityLevel === 'sandbox') {
sandboxElement = select('#i' + id);
}
const root =
securityLevel === 'sandbox'
? select(sandboxElement.nodes()[0].contentDocument.body)
: select('body');
// const doc = securityLevel === 'sandbox' ? sandboxElement.nodes()[0].contentDocument : document;

// Get a reference to the svg node that contains the text
const svg = root.select(`[id='${id}']`);
const { svg } = selectElementsForRender(id);

// Add cardinality marker definitions to the svg
erMarkers.insertMarkers(svg, conf);
Expand Down
3 changes: 2 additions & 1 deletion packages/mermaid/src/diagrams/error/errorRenderer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import { select } from 'd3';
import { log } from '../../logger.js';
import { getErrorMessage } from '../../utils.js';
import { selectElementsForRender } from '../../rendering-util/selectElementsForRender.js';

/**
* Merges the value of `conf` with the passed `cnf`
Expand All @@ -24,7 +25,7 @@ export const draw = (_text: string, id: string, mermaidVersion: string) => {
try {
log.debug('Renering svg for syntax error\n');

const svg = select('#' + id);
const { svg } = selectElementsForRender(id);

const g = svg.append('g');

Expand Down
20 changes: 3 additions & 17 deletions packages/mermaid/src/diagrams/gantt/ganttRenderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
import common from '../common/common.js';
import { getConfig } from '../../config.js';
import { configureSvgSize } from '../../setupGraphViewbox.js';
import { selectElementsForRender } from '../../rendering-util/selectElementsForRender.js';

export const setConf = function () {
log.debug('Something is calling, setConf, remove the call');
Expand Down Expand Up @@ -59,19 +60,9 @@ let w;
export const draw = function (text, id, version, diagObj) {
const conf = getConfig().gantt;

const securityLevel = getConfig().securityLevel;
// Handle root and Document for when rendering in sandbox mode
let sandboxElement;
if (securityLevel === 'sandbox') {
sandboxElement = select('#i' + id);
}
const root =
securityLevel === 'sandbox'
? select(sandboxElement.nodes()[0].contentDocument.body)
: select('body');
const doc = securityLevel === 'sandbox' ? sandboxElement.nodes()[0].contentDocument : document;
const { svg, doc } = selectElementsForRender(id);
const elem = svg.node();

const elem = doc.getElementById(id);
w = elem.parentElement.offsetWidth;

if (w === undefined) {
Expand Down Expand Up @@ -122,7 +113,6 @@ export const draw = function (text, id, version, diagObj) {

// Set viewBox
elem.setAttribute('viewBox', '0 0 ' + w + ' ' + h);
const svg = root.select(`[id="${id}"]`);

// Set timescale
const timeScale = scaleTime()
Expand Down Expand Up @@ -447,10 +437,6 @@ export const draw = function (text, id, version, diagObj) {

// Wrap the tasks in an a tag for working links without javascript
if (securityLevel === 'sandbox') {
let sandboxElement;
sandboxElement = select('#i' + id);
const doc = sandboxElement.nodes()[0].contentDocument;

rectangles
.filter(function (d) {
return links[d.id] !== undefined;
Expand Down
22 changes: 3 additions & 19 deletions packages/mermaid/src/diagrams/info/infoRenderer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { select } from 'd3';
import { log } from '../../logger.js';
import { getConfig } from '../../config.js';
import type { DrawDefinition, HTML, SVG } from '../../diagram-api/types.js';
import type { DrawDefinition } from '../../diagram-api/types.js';
import { selectElementsForRender } from '../../rendering-util/selectElementsForRender.js';

/**
* Draws a an info picture in the tag with id: id based on the graph definition in text.
Expand All @@ -14,22 +13,7 @@ const draw: DrawDefinition = (text, id, version) => {
try {
log.debug('rendering info diagram\n' + text);

const { securityLevel } = getConfig();
// handle root and document for when rendering in sandbox mode
let sandboxElement: HTML | undefined;
let document: Document | null | undefined;
if (securityLevel === 'sandbox') {
sandboxElement = select('#i' + id);
document = sandboxElement.nodes()[0].contentDocument;
}

// @ts-ignore - figure out how to assign HTML to document type
const root: HTML =
sandboxElement !== undefined && document !== undefined && document !== null
? select(document)
: select('body');

const svg: SVG = root.select('#' + id);
const { svg } = selectElementsForRender(id);
svg.attr('height', 100);
svg.attr('width', 400);

Expand Down
29 changes: 6 additions & 23 deletions packages/mermaid/src/diagrams/sankey/sankeyRenderer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,7 @@
import { Diagram } from '../../Diagram.js';
import * as configApi from '../../config.js';

import {
select as d3select,
scaleOrdinal as d3scaleOrdinal,
schemeTableau10 as d3schemeTableau10,
} from 'd3';
import { scaleOrdinal as d3scaleOrdinal, schemeTableau10 as d3schemeTableau10 } from 'd3';

import {
sankey as d3Sankey,
Expand All @@ -18,7 +14,8 @@ import {
} from 'd3-sankey';
import { configureSvgSize } from '../../setupGraphViewbox.js';
import { Uid } from '../../rendering-util/uid.js';
import type { SankeyLinkColor, SankeyNodeAlignment } from '../../config.type.js';
import type { SankeyNodeAlignment } from '../../config.type.js';
import { selectElementsForRender } from '../../rendering-util/selectElementsForRender.js';

// Map config options to alignment functions
const alignmentsMap: Record<
Expand All @@ -40,26 +37,12 @@ const alignmentsMap: Record<
* @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 {
const { svg } = selectElementsForRender(id);

// Get Sankey config
const { securityLevel, sankey: conf } = configApi.getConfig();
const { sankey: conf } = configApi.getConfig();
const defaultSankeyConfig = configApi!.defaultConfig!.sankey!;

// TODO:
// This code repeats for every diagram
// Figure out what is happening there, probably it should be separated
// The main thing is svg object that is a d3 wrapper for svg operations
//
let sandboxElement: any;
if (securityLevel === 'sandbox') {
sandboxElement = d3select('#i' + id);
}
const root =
securityLevel === 'sandbox'
? d3select(sandboxElement.nodes()[0].contentDocument.body)
: d3select('body');
// @ts-ignore TODO root.select is not callable
const svg = securityLevel === 'sandbox' ? root.select(`[id="${id}"]`) : d3select(`[id="${id}"]`);

// Establish svg dimensions and get width and height
//
const width = conf?.width || defaultSankeyConfig.width!;
Expand Down
32 changes: 32 additions & 0 deletions packages/mermaid/src/rendering-util/selectElementsForRender.ts
View file
Open in desktop
Copy link
Member

@Yokozuna59 Yokozuna59 Jul 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The file name is kinda long, the folder containing this is already rendering-util, so ForRendering part is kinda clear. The selectElements part is kinda referring to it could select and return multiple elements, whereas it it's only one.

Not sure what to suggest :).

For lines at the end of file, see #4514 to update because it's currently incorrect.

Sorry I could refer to specific lines in review but github in phone isn't as PC, I don't know how to review here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't know that you had similar PR and ideas about that. This is simply a draft. There is a complete mess inside draw functions so I put it aside for now

Copy link
Member

Choose a reason for hiding this comment

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

tbh, I didn't thought of creating a file for selecting, although we should since it's a common lines in all diagram :), so thanks for that!

I'll be working in convert it files and delete some of the randomness there, but I don't have time rn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Separate file is not an ultimate dream.
Too many side effects inside draw function is happening

draw takes svg element

To my mind draw function of each particular graph must accept the target element, may be along with parent element, document etc. They don have to call this "getElements" function directly, base class or interface must provide it to them.

render function with parameters

Even better would be a render function that:

  • Takes parsed graph definition
  • Takes options (dimensions and whatever is needed for the graph), extracted from element beforehand
  • Returns some data, including string with svg content, title or any other external attributes for html elements, some additional properties such as callbacks for clicks

Copy link
Contributor Author

@nirname nirname Jul 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Besides we are selecting these elements in mermaidAPI, by the time of calling draw function we already know what they are.
And render function in mermaidAPI does actually the same

  return {
    svg: svgCode,
    bindFunctions: diag.db.bindFunctions,
  };

I don see a reason why diagram-dedicated functions would violate this interface, so that I could call something like mermaidAPI.render as well as mermaid.Info.render

Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import { select } from 'd3';
import { getConfig } from '../config.js';
import type { HTML, SVG } from '../diagram-api/types.js';

interface SelectedElements {
doc: Document;
svg: SVG;
}

export const selectElementsForRender = (id: string): SelectedElements => {
const { securityLevel } = getConfig();
// Handle root and document for when rendering in sandbox mode
let sandboxElement: HTML | undefined;
let doc: Document = document;

if (securityLevel === 'sandbox') {
sandboxElement = select('#i' + id);
}

if(sandboxElement) {
doc = sandboxElement.nodes()[0].contentDocument || doc;
}

// @ts-ignore - figure out how to assign HTML to document type
const root: HTML = sandboxElement ? select(doc) : select('body');

const svg: SVG = root.select('#' + id);
return {
doc,
svg,
};
};
Loading