Trim the unified toolbar to essentials - #397
Conversation
bvnaik05
changed the title
vibhavkatre
left a comment
vibhavkatre
left a comment
There was a problem hiding this comment.
Reviewed against the quality checklist. There is good work in here — but it can't merge in this shape, and the reasons are mostly about what the PR is rather than how the code is written.
It's several unrelated changes in one PR
The branch started as the #396 rename focus-ring fix and grew to 41 files. What's actually inside:
- The title-rename focus ring (#396) — the original, one-line fix.
- A toolbar trim: Undo/Redo, the Canvas theme menu, and the Square/Diamond insert tiles removed.
Rulers.vuedeleted outright, withdesign/SPEC.md§6 rewritten to match.- A per-shape corner-radius picker, plus square-by-Shift-drag.
- Flowchart work: live edge anchors in
ConnectorView,swapFlowchartNodeTypefor migrated nodes, flowchart drop-to-type,FlowchartHoverHandleschanges. - The whole of #401's whiteboard change.
Items 4 and 5 are genuinely useful and I'd like them. But they can't be reviewed or reverted as a unit while they ride along with a product change and a spec rewrite. Please split: the flowchart cluster is its own PR, the corner-radius picker is its own PR, and #396 is the one-line fix it started as.
Three of these are product decisions that were already made the other way
These were settled with Vibhav on 10 August and shipped in #392, #366 and #395:
- Undo/Redo on the toolbar is #392. Making them keyboard-only reverses it, and keyboard-only undo is not reachable for a pointer user at all.
- The Canvas theme menu is #366.
store.applyThemehad no caller for a long time even though SPEC requires theme presets; that group is what finally wired it up. DeletingCanvasGroup.vuestrands it again. - Removing the ruler is issue #398, which is open and undecided. This PR both implements it and rewrites the spec section that defines it.
None of these is a bad idea on its own — the bar genuinely is at its width limit (~1229px of 1280 in the densest state), so trimming is a real conversation. But it's Vibhav's call, and it needs to happen on #398 and as a toolbar decision, not inside a focus-ring branch. I'm raising it with him separately.
Two things to fix regardless of how it's split
The issue references are wrong almost everywhere. (#396) is cited in ConnectorView.vue, useShapeCreation.js, StyleGroup.vue and useDiagramStore.js for flowchart anchors, drop-to-type, the corner picker and node-type swapping. #396 is "Renaming file UI not Frappe compliant". These comments outlive us — a maintainer following one lands on an unrelated issue and loses the reasoning. Please point each at the issue it belongs to, or file one.
The corner radius won't survive an export. documentToSvg renders every rect with a hardcoded rx="8" (useThumbnail.js:72), so a shape set to 32 exports at 8 in PNG, PDF, the Home thumbnail and the minimap. Worth checking the new cornerRadius field survives a save/load round trip too, since no shape carried it before.
Also worth knowing
This PR contains #401's whiteboard change byte-for-byte (WhiteboardTools.vue, useWhiteboardUi.js, useWhiteboardInteraction.js, whiteboardColors.js all resolve to identical blobs). #401 is now merged, so please rebase — that part is already on main.
The manual browser check in the test plan is still unticked. For a change that removes controls from the toolbar, that check is the point.
|
Vibhav has now made the four product calls this PR was waiting on. All four go your way — the trim is approved as proposed:
Two consequences he accepted knowingly, worth writing down so nobody "fixes" them back later:
What the PR still needsThe product objection is resolved; the structural one isn't. Three things: 1. Rebase. #401 merged, so this branch's copy of the whiteboard change is now redundant — 2. Split off the flowchart and shape work. The toolbar trim is now decided and can go in as its own PR. The rest is independent of it and should be reviewed on its own merits:
Both are worth having. Neither should be able to hide inside a PR about the toolbar. 3. Two fixes carried over from the earlier review:
One addition now that the Canvas menu is going. Deleting Ping me when it's rebased and split and I'll review the pieces. |
Rebased onto main and reduced to the toolbar trim alone; the flowchart work is now frappe#423, the corner-radius picker frappe#420, and the frappe#396 focus-ring fix frappe#421. Closes frappe#398. Four product calls, all settled with Vibhav on 12 August: - Undo/Redo leave the bar and stay on ⌘Z / ⇧⌘Z. The bar was at its width limit (~1229px of 1280 in its densest state), and undo is the shortcut people already reach for. - The Canvas colour menu goes entirely. - The ruler goes, and in-shape text gets a fixed 12px horizontal padding. - The Square and Diamond insert tiles go. A square is a rectangle drawn with Shift held — boxBetween already locks the sides equal, so this needed no new drag logic, only a tooltip on the Rectangle tile so the gesture is findable. Guides survive the Canvas menu's deletion as their own entry, so the bar still says something useful when nothing is selected. Two consequences accepted knowingly, recorded so nobody restores them as bugs: - A plain diamond can no longer be inserted. The tile was its only route. The glyph stays reachable as a flowchart decision node; it is the free-standing block diamond that goes. - An existing diagram can no longer be recoloured. Settings' defaultThemePreset applies to new diagrams only, so a diagram's look is settled when it is created. Deleting CanvasGroup.vue takes store.applyTheme's only caller with it, so applyTheme goes in the same commit rather than being stranded a second time — along with restyleShapes, which nothing else reached, and the findThemePreset import that only restyleShapes used. attachThemeAndCanvas becomes attachCanvas now that setCanvas is all it holds. state.themePreset stays: the canvas's data-fdpreset, the flowchart and mind-map layers and the thumbnails all read it. The design docs are amended here rather than left to contradict the code, the way frappe#380 handled the catalog-search removal — CONVENTIONS says the design document wins, so leaving them would make the spec the bug. SPEC §5 loses the one-click diagram-wide restyle, §6 and step 6 lose the rulers, step 7 loses the theme-preset criterion, and the store API list loses applyTheme. Each amendment says why and what would justify reinstating it. Drive-by in DiagramCanvas: the empty-canvas prompt used text-md, which has no generated utility, so that line had been rendering at the default size. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
611ff7f to
60e4b8b
Compare
|
Rebased and split. Ready for review.
Rebase — done, onto The two carried-over fixes:
SPEC — amended here in the #380 style. §5 loses the one-click diagram-wide restyle, §6 and step 6 lose the rulers, step 7 loses the theme-preset criterion, and the store API list loses Both accepted consequences are written into the PR body so nobody restores them as bugs: no free-standing diamond insert, and no recolouring an existing diagram. One merge note: #397 and #423 both touch Full suite green on all four (1004 tests on this one) and |
…ng goes (#421) Split back out of #397 at review request — this is the one-file fix that branch started as. Closes #396. The rename box was a hand-rolled <input> with its own border, padding and focus:border classes, so it kept the browser's default focus ring on top of them: the reported symptom. frappe-ui's TextInput already applies focus:ring-0 along with the rest of the control's chrome, so adopting it removes the ring rather than papering over it, and the box now matches every other input in the app instead of approximating one. The display state was a hand-rolled <button> for the same reason and becomes a ghost Button, with the pencil affordance moving into its #suffix slot. Two details the swap forces: - The ref now points at the TextInput component rather than a DOM node, so startEditing focuses through its exposed `el`. A bare ref.focus() would fail silently and leave the box looking editable but unfocused and unselected, which is why the test pins it. - The title keeps 14px/600 (README's toolbar title style) through text-base-semibold; Button's own size classes stop at the regular weight. `shrink` overrides Button's shrink-0 so a long title still ellipsises inside its column instead of pushing the actions cluster off the bar. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…an export (#420) Split out of #397 at review request. Closes #411. The Corners popover existed but only opened for a mind-map or flowchart node, where it sets branch curve. A plain rounded rectangle had exactly one roundedness, 20, with no way to change it. It now picks from four presets, previewed as the literal corner it will get. shapeCornerRadius takes the shape's own cornerRadius and lets it win over the type default. Only a finite, non-negative number counts: the value arrives from a persisted document, and a string or a NaN would reach an rx attribute and drop the corner entirely. The export fix is the reason this is worth having. documentToSvg hardcoded rx="8" for every generic rect, and it is the single renderer behind PNG export, PDF export, the home tile and the minimap — so a radius never left the canvas. That was already broken before this picker: a rounded rectangle drew at 20 and exported at 8, visibly tighter in all four places. Routing the line through the same helper the canvas uses fixes both at once rather than restating the rule. isRoundedBoxShape lives in shapeGeometry.js next to the radius it guards. A mind-map or flowchart node can also carry type 'rounded', and its corners come from its own role-specific control, so a role disqualifies it and the two controls in the shared popover never fight over one shape. The swatch previews at a quarter scale on purpose. border-radius clamps to half the box height, so at full value 12 / 20 / 32 would all render as the same pill and three of the four presets would be indistinguishable. cornerRadius needed no schema work — createShape spreads its partial last and getDocument serialises state.shapes wholesale — but nothing pinned that, and a radius the canvas honours that a reload drops is worse than no picker at all. A test now round-trips it through getDocument -> JSON -> parseDiagramDocument -> loadDocument, the path the editor actually uses. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
vibhavkatre
left a comment
vibhavkatre
left a comment
There was a problem hiding this comment.
The split is exactly what was asked for, and the trim itself is now reviewable on its own terms. Approving.
The test rewrite is the part that makes this safe. Converting "HistoryGroup reaches store.undo()" into "HistoryGroup.vue no longer exists, and useKeyboard still binds ⌘Z" is the honest version: removing the buttons makes the shortcut the whole feature, so pinning the shortcut is what stops a later refactor quietly taking undo away entirely. Deleting the applyTheme assertion is correct for the same reason — the capability is gone by decision, so a test asserting it would be the bug.
Verified the claims I could check independently:
- Shift-drag squares already work on
main—useShapeCreation.js:59setsdrag.square = event.shiftKeyandboxBetweenlocks the sides. No new drag logic was needed, as claimed; the tooltip is the only thing the removal required. - Nothing else reaches the Square or Diamond tiles. No keyboard shortcut arms either type, so dropping them strands no other route.
applyThemeandrestyleShapesreally do go to zero callers, andstate.themePresetis still read by the canvas, the layers and the thumbnails — so removing the mutator without removing the field is the right cut.
Amending the design docs in the same PR is the right call, and the amendments are better than they had to be: each says what was removed, why, and what would justify reinstating it. The honesty about the old theme apply — that it could only repaint shapes still wearing the previous triad and silently skipped every override — is worth having on the record.
One consequence to keep in view, not a change request: shapes that already carry text.insetLeft / insetRight will re-flow to the fixed 12px, so any label a user hand-positioned moves. That is inherent to the decision rather than a defect, and the fields simply become inert data in existing documents.
Merged: verified alongside #421, #420 and #423 — 1043 tests, build and lint green on all four combined, with CanvasToolbar resolving correctly (GuidesGroup in, HistoryGroup and CanvasGroup gone, all three deleted files actually removed).
#423) * feat: a migrated flowchart routes, retypes and labels itself correctly Split out of #397 at review request. Closes #410. Three faults, one cause: the free-floating representation (#122) never got the controls the legacy framed one has, so each path reached the wrong model or nothing at all. Edges stopped routing off stale anchors. A flowchart edge's from/to anchor is stored on the connector but only ever WRITTEN at creation, or by an explicit whole-chart Tidy/flip. Dragging either node never updated it, so moving a child above its parent left the arrow leaving the bottom and arriving at the top — reading as a route that doubles back instead of flowing forward. ConnectorView now derives the pair live from the two shapes' current boxes through the edgeAnchors formula Tidy and flip already use, so every flowchart edge self-corrects on render however its nodes got where they are. The node type became reachable. useSelectionContext resolves a migrated node to 'block' chrome, so FlowchartNodeGroup — which reads the legacy sub-model — never rendered for one, and its picker only ever wrote into an empty state.flowchart. FlowchartNodeTypeGroup sits alongside the generic Fill/Border instead, calling a new store.swapFlowchartNodeType that rebuilds the model, runs the existing branch-aware swapNodeType rather than restating its rules, and writes back the shape's glyph, box and branches plus its outgoing edges' ports. It reads the swapped node back before committing, which is what distinguishes a real swap from swapNodeType's no-op on an unknown type, so an invalid type can never reach the shape. No route patching is needed here — that is what the live anchors above are for. Adding a node no longer forces Process and no longer eats the label. The hover "+" passed a fixed 'process', so every node added by pointer had to be converted afterwards, which the fault above made impossible; it now opens the type picker (spec B3/B4). Both it and the F5 keys left the new node merely selected, and Enter/D/T/I are build shortcuts while a node owns the keyboard — so typing "Deploy" silently added a Decision. The new node now lands in text-edit with its default label pre-selected, so the first keystroke names it. The picker's outside-close listener is capture-phase and excludes its own DOM on purpose: stopPropagation on the target cannot stop a listener that already ran on the way down, so without the exclusion choosing a type would clear the state before the click handler that reads it fired. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: let the E2E dismiss the new text-edit session before pressing Enter unified-canvas.spec.js:399 inserts a flowchart node and presses Enter expecting a connected step. That node now lands in text-edit with its label pre-selected, which is the point of the change — so Enter committed the label instead of building, and the test saw 1 node where it wanted 2. Escape first, so the key reaches the free-floating flowchart keydown handler rather than TextEditor. This is the spec following the behaviour, not a workaround: a user who wants to chain nodes by keyboard now presses Escape too, and that is the trade the change makes deliberately — the alternative was typing a label silently adding a Decision node. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Vibhav Katre <k.vibhav@gmail.com>
Rebased onto
mainand reduced to the toolbar trim alone, per @vibhavkatre's review. Closes #398.The split he asked for:
main— gone with the rebaseSummary
Four product calls, all settled with Vibhav on 12 August:
The bar was genuinely at its width limit — ~1229px of 1280 in its densest state — which is what made the trim worth having. Undo is the shortcut people already reach for.
A square is a rectangle drawn with Shift held.
boxBetweenalready locks the sides equal, so this needed no new drag logic — only a tooltip on the Rectangle tile so the gesture is findable now that the tile is gone.Guides survive the Canvas menu's deletion as their own toolbar entry, so the bar still says something useful when nothing is selected.
Two consequences, accepted knowingly
Recorded here so nobody later restores them as bugs:
defaultThemePresetapplies to new diagrams only, so a diagram's look is settled when it is created.applyThemeis not stranded a second timeDeleting
CanvasGroup.vuetakesstore.applyTheme's only caller with it, soapplyThemegoes in this same PR — along withrestyleShapes, which nothing else reached, and thefindThemePresetimport that onlyrestyleShapesused.attachThemeAndCanvasbecomesattachCanvasnow thatsetCanvasis all it holds.state.themePresetstays: the canvas'sdata-fdpreset, the flowchart and mind-map layers and the thumbnails all read it.Design docs amended here, not left to contradict the code
Following the pattern #380 used for the catalog-search removal — CONVENTIONS says the design document wins, so leaving them would make the spec the bug. Each amendment states what was removed, why, and what would justify reinstating it:
setCanvasis still there.applyTheme; the file tree and the "between the title bar and the ruler" description lose Rulers.Drive-by
DiagramCanvas's empty-canvas prompt usedtext-md, which has no generated Tailwind utility, so that line had been rendering at the default size. Nowtext-base.Note for whoever merges
CanvasToolbar.vue,components.d.ts,canvasToolbar.test.jsanduseDiagramStore.jsare also touched by #423. The edits sit in different regions of each file, so whichever of the two lands second needs a trivial rebase.Test plan
yarn vitest run— 1004 tests pass (full suite)yarn buildsucceeds🤖 Generated with Claude Code