Repository navigation
Give each diagram its own SVG, listeners and ids - #1156
Merged
Merged
Conversation
The diagram renderer kept one module-level SVG element that every new graph replaced, plus window globals and fixed element ids. When a page drew more than one diagram, one graph's layout, zoom and export could act on another graph's SVG, and ids such as the grid pattern collided. Each GraphData now owns its SVG element, its selection listener and an id prefix for the definitions and references it creates. Helpers that read the shared element take the graph instead. exportSVG restores the existing names on its clone, so exported files and saved layouts are unchanged. The window.graph and window.gdata globals are removed; the browser tests drive the editor through its toolbar instead.
The preview dot shown on Alt-hover is now created with a per-graph id, so the editor style sheet's circle#prj rule no longer matched and the dot was drawn black. The dot carries a class and the rule selects it. The #icon-cube rule repeated the .icon rule on the same element and is removed. Export no longer rewrites href values and leaves user-authored node links untouched. The unused dragging flag and its callback are removed. Browser tests check the preview dot's colors after a real Alt-hover, that each graph is fitted inside its own container, and that a node link starting with the live id prefix survives export unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The diagram renderer behind
mdl serve,mdl svgand the static page keeps one SVG element in a module-level variable. Every new graph replaces it, every graph's SVG has the idgraph, and the renderer also setswindow.graphandwindow.gdata.That is fine while a page draws one diagram. The static page, however, draws every
.c4-diagramelement in the document, and its diagrams are laid out asynchronously. Once the second graph is built, the first graph's layout, zoom and export act on the second graph's SVG. Ids collide too: each graph writes its owngrid-pattern,arrowandicon-*definitions under the same names, so aurl(#…)reference in one diagram can resolve to another diagram's definition.A test added here shows the failure on the previous code: with two views in one document, the first view's nodes are no longer in the page when its layout runs.
What changes
Each graph now owns what it draws:
GraphDataholds its own SVG element and its selection listener. The module-level SVG and listeners are gone, and so arewindow.graphandwindow.gdata.g1-arrow,g2-grid-pattern, node, edge and vertex ids) and the references to them, so two diagrams never share an id.exportSVGremoves the prefix on the copy it already makes and gives the root the idgraph. Exported files are unchanged:mdl svgoutput for both views ofexamples/basicis byte-identical before and after. Saved layouts load as before.The helpers that read the shared element take the graph as their first argument:
getZoom,setZoom,setZoomCentered,getZoomAuto,addCursorInteraction,saveViewState,restoreViewStateandclearViewState.buildGraphandbuildGraphViewkeep their signatures; the static and headless entry points are not edited.Two style rules selected by id. The relationship-point preview shown on Alt-hover is now selected by class, and the
#icon-cuberule, which repeated the.iconrule on the same element, is removed.Tests
TestTwoGraphsInOneDocumentputs two views with different shapes and layout directions in one document, turns the grid on in both and lays them out at the same time. It checks that each SVG holds only its own nodes, that no id appears twice in the page, that everyurl(#…)reference resolves inside its own SVG, that each graph is fitted inside its own container, and that each export carries the saved ids.TestEdgePointPreviewStylemoves the mouse over a relationship with Alt held and compares the preview dot's computed colors with the style sheet's rule.window.gdata. They use the editor's toolbar (Auto layout, Save) where that is enough. Two tests read the editor's graph from React's component data, so they depend on React internals:TestManualEditAfterAutoLayoutneeds a value no control exposes, andTestTwoGraphsInOneDocumentneeds both graph objects.Limits
webpack.config.static.js) loads its layout chunk from a wrong path, so automatic layout fails there. That defect predates this change and is not fixed here; no test builds that bundle.The rebuilt files under
cmd/mdl/webapp/distcome frommake build-ui.