refactor(graph): move the label, z-order and page members to their mixins - #1135
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
a231598 to
d88631b
Compare
…xins Continue emptying AbstractGraph of members whose only consumers live in an existing mixin. To LabelMixin, whose getLabel is the main caller of the first: convertValueToString and getLinkForCell. LabelMixin stops declaring convertValueToString as an external dependency. To OrderMixin, which was until now the smallest mixin with only orderCells and cellsOrdered: keepEdgesInForeground and keepEdgesInBackground. Their single consumer is GraphView, and z-order is exactly the concern this mixin covers. To the page mixin, which already picked six of the getters from the class: pageVisible, pageBreaksVisible, pageBreakColor, pageBreakDashed, minPageBreakDist, preferPageSize and pageScale with their accessors, plus getPreferredPageSize. Rename PageBreaksMixin to PageMixin as part of that last move. Once the mixin holds the page settings and not only the break rendering, its name no longer describes its scope. The rename is internal, mixins are not exported outside their directory and index.ts only imports the aggregated types for their side effect, so no public symbol changes. pageFormat stays in AbstractGraph for the same reason as warningImage: its default is a mutable Rectangle that mixInto would install on the prototype and share across every graph instance. Only getPageFormat moves, and a comment in the mixin records why the pair is split. AbstractGraph loses 142 lines and its last use of isNode.
d88631b to
4138c94
Compare
|
Note
Stacked on top of #1134, itself stacked on #1132 and #1131. The base of this PR is
refactor/move_cells_validation_overlays_members_to_mixins, so the diff shown here contains only this step.Fourth implementation step of the plan recorded in ADR 0003 (#1130). No behavior change, and the public API is unchanged.
Changes
LabelMixinconvertValueToString,getLinkForCellgetLabel, plusEditingMixinandTooltipHandlerOrderMixinkeepEdgesInForeground,keepEdgesInBackgroundGraphViewPageMixinpageVisible,pageBreaksVisible,pageBreakColor,pageBreakDashed,minPageBreakDist,preferPageSize,pageScale, their accessors, andgetPreferredPageSizeupdatePageBreaks,sizeDidChangeAbstractGraph.tsloses 142 lines and its last use ofisNode.LabelMixinstops declaringconvertValueToStringas an external dependency, and the page mixin stops declaring six accessors it now owns.OrderMixinwas the smallest mixin in the codebase, holding onlyorderCellsandcellsOrdered. Z-order is exactly its concern, so the two flags belong there.PageBreaksMixinbecomesPageMixinDecision D5 of the ADR. Once the mixin holds the page settings and not only the break rendering, its old name no longer describes its scope.
The rename is internal: mixins are not exported outside
view/mixin/, andindex.tsonly imports the aggregated_graph-mixins-types.jsfor its side effect. No public symbol changes, and the diff is the two mixin files plus the two registration files.The same deliberate asymmetry as #1134
getPageFormatmoves butpageFormatstays inAbstractGraph, in the per-instance group. Its default is a mutableRectangle, so installing it on the prototype would share one instance across every graph, which the tests from #1131 guard against.A comment in
PageMixinrecords why the pair is split. It will be reunited when the mixin becomes a plugin, step 6 of the ADR plan.Validation
Run locally on Node 24 (
.nvmrc):build,test-check, the full suite (505 tests, 60 suites),lint,check:circular-dependencies. All passing, and no test needed changing, which is the expected signal for a pure relocation.