Skip to content

fix: rename the diagram title through frappe-ui, so the blue focus ring goes - #421

Merged
vibhavkatre merged 1 commit into
frappe:mainfrom
bvnaik05:fix-396-title-rename-frappe-ui
Aug 12, 2026
Merged

fix: rename the diagram title through frappe-ui, so the blue focus ring goes#421
vibhavkatre merged 1 commit into
frappe:mainfrom
bvnaik05:fix-396-title-rename-frappe-ui

Conversation

@bvnaik05

Copy link
Copy Markdown
Contributor

Split back out of #397 at review request — "#396 is the one-line fix it started as". Closes #396.

Summary

  • The rename box was a hand-rolled <input> carrying its own border, padding and focus:border classes, so the browser's default focus ring sat on top of them. That ring was the reported symptom. frappe-ui's TextInput already applies focus:ring-0 as part 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 changes what it points at. It now holds the TextInput component rather than a DOM node, so startEditing focuses through its exposed el (TextInput.vue does defineExpose({ el: inputRef })). A bare ref.focus() would fail silently and leave the box looking editable but unfocused and unselected — which is why there's a test pinning it.

The title keeps its type. 14px/600 is the toolbar title style in design/README.md, and Button's own size classes stop at the regular weight, so the label carries text-base-semibold. shrink overrides Button's shrink-0 so a long title still ellipsises inside its column instead of pushing the actions cluster off the bar.

Enter / blur still commit and Escape still cancels — TextInput forwards listeners onto the inner input, and the test pins all three.

Test plan

  • yarn vitest run src/components/toolbar src/frappe-ui-tokens.test.js — 107 tests pass, including 4 new
  • yarn build succeeds
  • Manual browser check: click the title, confirm no blue ring, confirm the text is focused and selected, confirm Enter / Escape / click-away behave

🤖 Generated with Claude Code

…ng goes

Split back out of frappe#397 at review request — this is the one-file fix that branch
started as. Closes frappe#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>
Copilot AI lite review requested due to automatic review settings August 12, 2026 07:58

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

bvnaik05 added a commit to bvnaik05/draw that referenced this pull request Aug 12, 2026
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>

@vibhavkatre vibhavkatre left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Verified the two claims the swap rests on, against beta.19 source — a source-scanning test can pin that @blur="commit" is written, but not that the listener reaches anything:

  • The listeners do reach the input. TextInput sets inheritAttrs: false and re-binds attrsWithoutClassStyle onto the inner <input>, so onBlur lands on the control itself. This mattered: blur does not bubble, so had the attrs stayed on the wrapper, commit-on-click-away would have silently stopped working.
  • defineExpose({ el: inputRef }) is there, so input.value?.el?.focus() is the correct reach.

text-base-semibold is a real utility, and it is the right one. I built the branch and read it out of the emitted CSS: font-size:14px; font-weight:600. That matches design/README.md:100 ("Toolbar title | 14px / 600") exactly. Worth checking rather than assuming, given the dead-token-class family that #287 cleaned up — a class with no generated utility renders nothing and looks like a styling opinion rather than a bug.

Also confirmed Button wraps its default slot in a truncate span (Button.vue:270), so a long title still ellipsises with min-w-0 shrink — the claim in the description holds.

One thing to expect visually: the title gets smaller. It was text-lg font-medium (18px/500) and is now 14px/600. That is a correction toward the design spec rather than a side effect, but it is a visible change beyond "the blue ring is gone", so don't be surprised by it.

Merged: verified alongside #420, #397 and #423 — 1043 tests, build and lint green on all four combined.

@vibhavkatre
vibhavkatre merged commit fc5d7a7 into frappe:main Aug 12, 2026
3 checks passed
vibhavkatre pushed a commit that referenced this pull request Aug 12, 2026
Rebased onto main and reduced to the toolbar trim alone; the flowchart work is
now #423, the corner-radius picker #420, and the #396 focus-ring fix #421.
Closes #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 #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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[UI fix]: Renaming file UI not Frappe compliant

3 participants