Redesign Slides toolbar with single morphing contextual layout#244
Merged
Conversation
The current Slides toolbar is dense and contextually-flat: eight align/distribute buttons stay visible regardless of selection, Fill/Font show as hint-bearing placeholders even with nothing selected, and several Google Slides parity affordances (Undo/Redo, Image insert, Slide background, Shape Border, text formatting in text-edit mode) are missing entirely. The spec replaces this with a single morphing toolbar: fixed global zones on the outside, a contextual middle that swaps for Idle / Object-selected / Text-editing states. Eight align/distribute icons collapse into one Arrange dropdown. Text formatting controls are extracted from the docs toolbar into shared components so docs and slides stop duplicating the same widgets. Delivery is a single PR with ~13 task-sized commits in dependency order, tracked in docs/tasks/active/.
The Slides toolbar redesign needs Border dash for shapes and a stroke field on text elements for the box-level border control. Both fields are optional so existing Yorkie documents remain valid without migration. The new Stroke type accepts color as ThemeColor | string so legacy stored ThemeColor objects continue to render correctly via resolveStrokeColor() while new toolbar paths can write plain hex strings directly. Renderers (shape, connector, action button) updated to use resolveStrokeColor() from render-context.ts instead of resolveColor() for stroke color fields.
Promotes Stroke to the package public surface so toolbar tasks can import it. Replaces the assign-and-read type tests with a real behavior test for resolveStrokeColor that exercises both the ThemeColor and string branches. Documents why resolveStrokeColor is used at the glyph site and that dash rendering is wired up in Task 8. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
The toolbar redesign uses these to switch into the text-editing state and bind shared text formatting controls to the docs editor that owns the active text box. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Aligns storage with currentSlideListeners and insertModeListeners which both use Set<() => void>. Avoids the array-reallocation on unsubscribe and removes a copy-paste inconsistency for future readers.
Slides toolbar redesign needs the shared text-formatting controls to drive both the docs main editor and the slides text-box editor. Promotes applyStyle/toggleList/indent/etc onto TextBoxEditorAPI by delegating to the internal TextEditor; mirrors the surface on SlidesTextBoxEditor. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Refactors the docs formatting toolbar to use three new shared components (TextStyleGroup, TextFormatGroup, TextParagraphGroup) under packages/frontend/src/components/text-formatting/, driven by the TextFormattingEditor interface. This makes the controls reusable for the upcoming slides text-box toolbar without duplicating logic. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Restores the IconChevronDown after the alignment icon in the header/footer toolbar — the original AlignmentDropdown rendered it and the refactor accidentally dropped it. Adds an optional allowedBlockTypes prop to TextStyleGroup so Task 11's slides toolbar can hide Title/Subtitle options that silently no-op inside text boxes.
Drops methods from TextFormattingEditor that no shared component calls. Imports STYLE_OPTIONS in the mobile overflow menu instead of re-inlining the block-type list. Extracts isMac/modKey to a shared platform module so the four copy-pasted definitions collapse to one. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
New slides/toolbar/ directory holds the redesigned toolbar shell. Old slides-formatting-toolbar.tsx remains mounted; the new toolbar is built up in isolation across the next several commits, then swapped in at the end. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Splits 'connector' out of the 'shape' selectionType so Task 8's Fill button doesn't need to re-discriminate. Adds optional onChange to the SlidesStore interface so toolbar subscribers no longer cast to a structural type at every call site. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Centralises the upload→insert path so the toolbar button, drag-drop, and clipboard paste can all funnel through one helper, preventing divergence flagged in the spec. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Aligns its disabled predicate with the rest of the insert group so the button reflects "no editor connected" the same way Select / Text / Shape / Line do.
When no element is selected and not text-editing, the toolbar's contextual middle shows the InsertGroup plus a Slide background themed color picker. Wires through SlidesToolbar's onImagePick prop (consumed by Tasks 5+ already). Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Replaces the eight always-on align/distribute buttons. Adds rotateBy and z-order convenience methods to SlidesEditor where missing. The component is unused for now; Task 8 wires it into the object-selected toolbar state.
Adds a brief comment to bringForward/sendBackward explaining why the descending/ascending sort keeps stored indices safe (only elements at unaffected sides shift). Adds parameterized tests covering the text-editing no-op guard for all five new methods.
Shape and connector selections route to ShapeControls (Fill + Border). Connector hides the Fill button. Image/text-element/mixed sub-states leave the contextual format zone empty for now — Tasks 9 and 10 fill them in. Arrange menu appears at the end of every object sub-state. Adds updateConnectorStroke to SlidesStore interface, MemSlidesStore, and YorkieSlidesStore since ConnectorElement.stroke is top-level (not inside a .data sub-object) and updateElementData throws for connectors.
Drops the dependency on applyShapeFill's internal batch wrapper. The multi-select fill loop now uses store.updateElementData directly so the helper isn't relied on for re-entrancy it wasn't designed for.
Replace and Alt write via store.updateElementData; Reset crop clears the crop field. Crop is a disabled placeholder until the full crop UI lands in its own spec. Both the Insert and Replace paths use an inline hidden file input internally and take an `upload` callback from the parent — unified pattern; `onImagePick` remains for the idle/object insert path since changing it would exceed the scope of this task. Also fixes MemSlidesStore.updateElementData to delete keys whose patch value is `undefined` (JSON.stringify strips them, so the previous spread-and-clone approach silently ignored explicit clears). Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
When a text element is selected as an object (not editing inside), the toolbar offers Background fill / Border / Font family / Font size that apply to the whole box. Per-run text editing is State 3 (Task 11). The font picker writes fontFamily / fontSize across every inline run of every selected text box. Also adds optional `fill?: ThemeColor` to TextElement.data to store the box background color (migration-safe optional field).
When state.kind === 'text-edit', the toolbar's contextual middle swaps to the shared TextStyleGroup / TextFormatGroup / TextParagraphGroup from components/text-formatting/, bound to the active SlidesTextBoxEditor. The right-globals zone gains a Done button that exits text editing (Esc-equivalent). InsertGroup is naturally hidden because the text-edit branch doesn't render it. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Mounts SlidesToolbar from slides/toolbar/ and removes the old slides-formatting-toolbar.tsx. Wires the workspace image upload pipeline (reusing uploadImageFile from spreadsheet) for both Insert and Replace image paths. Present button is now rendered inside RightGlobals, so the separate mount in the SiteHeader is removed. Updates the harness visual scenario to use SlidesToolbar too. Manual smoke pending: open a slides document in `pnpm dev`, exercise Idle / Shape selected / Image selected / Text element selected / Text editing / Multi-select states; verify Undo/Redo, Image insert, Background, Border, Replace, Done all work. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Six new slides toolbar scenarios cover idle / shape / image / text-element / text-editing / multi-select states for snapshot regression. Lessons file captures the architecture findings (TextBoxEditorAPI shim, ConnectorElement.stroke, undefined patch strip) and follow-ups (Crop UI, Flip, stroke lifting). docs/design/README.md updated to link the new spec.
IconCropOff doesn't exist in @tabler/icons-react 3.31. The undo-arrow glyph reads as "reset/revert the crop" and is consistent with the project's icon set.
Slide background is a document-level control; it now lives in RightGlobals alongside the Theme toggle so it stays reachable from every toolbar state (idle / object / text-edit), not only when nothing is selected. Adds separators at every group boundary that was previously butted up against the next group: - contextual middle ↔ right globals - Done ↔ slide-style group (text-edit state) - right globals ↔ Present split-button - shape-controls: Fill ↔ Border - text-element-controls: Border ↔ Font - image-controls: Reset crop ↔ Alt text IdleSection collapses to just InsertGroup since the Background button moved out. The background-handler test is renamed to reflect its new home in RightGlobals (logic is unchanged).
- Move PresentButton back to the SiteHeader area (its location before Task 12 folded it into the toolbar). The toolbar's RightGlobals now hosts only Done / Background / Theme. - ToolbarSeparator: switch to bg-zinc-300 / dark:bg-zinc-700 (with !h-5 to defeat the data-orientation:h-full default). The --border default was too faint to read as a divider.
- YorkieSlidesStore.updateElementData now applies the patch
key-by-key with an explicit `delete` for `undefined` values, so
clears like Reset Crop (`updateElementData(id, { crop: undefined })`)
actually clear the field. MemSlidesStore was patched alongside
Task 9; the Yorkie path was missed and silently no-op'd in real
collaborative sessions.
- Strip "Task N" / "wired in Task N" / "populated by Tasks 5-11"
references from source code. Task numbering belongs in the PR
description and rots as the codebase evolves; the comments now
describe what the code does and why, not which task introduced it.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (60)
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR implements a complete redesign of the Slides editor toolbar from a fixed button layout to a morphing, context-driven UI. It extracts reusable text-formatting components from Docs, expands both Docs and Slides text-editing APIs, adds image insertion/replacement workflows, refactors the old toolbar, and delivers comprehensive test and documentation coverage. ChangesSlides Toolbar Redesign
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
Verification: verify:selfResult: ✅ PASS in 160.0s
Verification: verify:integrationResult: ✅ PASS |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Adds the missing scenario IDs (slides-toolbar-{idle,shape-selected,
image-selected,text-element-selected,text-editing,multi-select}) to
verify-visual-browser.mjs's hardcoded list so the harness captures
them as separate baselines. Establishes 24 new baseline PNGs across
the four profiles (desktop / desktop-dark / mobile / mobile-dark)
for the six new scenarios.
Also refreshes pre-existing baselines that drifted: the existing
slides-toolbar / slides-theme-panel / harness-root images shift
because the toolbar layout, height, and right-globals composition
changed; mobile-dark variants of slide-canvas / shapes scenarios
were already drifted before this branch (the spec reviewer noted
~168 pre-existing non-docs mismatches) and are now aligned.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (2)
packages/frontend/tests/app/slides/toolbar/shape-controls.test.ts (1)
24-48: ⚡ Quick winAvoid duplicating production handler logic inside the test.
This local mirror can drift from
shape-controls.tsxand create false confidence. Extract/import a shared pure handler (or helper) and assert against that symbol directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/frontend/tests/app/slides/toolbar/shape-controls.test.ts` around lines 24 - 48, The test duplicates the production on-stroke handler logic via makeOnStrokeChange; instead extract the pure handler from the production module (shape-controls.tsx) or create a shared helper and import it into both the component and this test, then replace the local makeOnStrokeChange with an import of that symbol. Ensure the shared handler preserves the same signature (store, slideId, elements, ids) and uses store.batch plus store.updateElementData and store.updateConnectorStroke for 'shape' vs 'connector' so the test asserts the real implementation rather than a mirrored copy.packages/slides/src/view/editor/editor.ts (1)
597-605: ⚡ Quick winAvoid per-iteration full-store reads in z-order loops.
Lines 602-604 and 627-629 call
store.read()for every selected element. Sinceread()clones the document, this scales poorly on larger decks. You can reorder by id with fixed target indices and remove the repeated reads entirely.Refactor sketch
this.options.store.batch(() => { - for (const id of orderedIds) { - const live = this.options.store.read().slides.find((s) => s.id === slideId); - if (!live) continue; - this.options.store.reorderElement(slideId, id, live.elements.length - 1); - } + const frontIndex = slide.elements.length - 1; + for (const id of orderedIds) { + this.options.store.reorderElement(slideId, id, frontIndex); + } }); this.options.store.batch(() => { - for (const id of [...orderedIds].reverse()) { - const live = this.options.store.read().slides.find((s) => s.id === slideId); - if (!live) continue; - this.options.store.reorderElement(slideId, id, 0); - } + for (const id of [...orderedIds].reverse()) { + this.options.store.reorderElement(slideId, id, 0); + } });Also applies to: 623-629
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/slides/src/view/editor/editor.ts` around lines 597 - 605, The loop repeatedly calls this.options.store.read() and clones the whole document per iteration; instead read the slide once (const live = this.options.store.read().slides.find(s => s.id === slideId)) before the loop, build the orderedId -> fixed targetIndex mapping (e.g., start at live.elements.length and increment for each id or compute desired insertion indices ahead of time) and then call this.options.store.reorderElement(slideId, id, targetIndex) for each id without further store.read() calls, using the existing orderedIds, slideId and reorderElement symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/slides/slides-toolbar-redesign.md`:
- Line 69: Several fenced code blocks in slides-toolbar-redesign.md are missing
language tags and trigger markdownlint MD040; locate the bare triple-backtick
code fences ("```") used throughout this document and add an appropriate
language identifier (e.g., `ts`, `text`, `bash`, etc.) after the opening fence
so each block becomes "```ts" or similar; ensure each changed fence matches the
code content and re-run the linter to confirm MD040 is resolved.
In `@packages/docs/src/view/text-box-editor.ts`:
- Around line 279-281: The callback registered in cursorMoveCallback (via
onCursorMove) is currently invoked every render (including blink-driven
renders); change renderNow so it tracks the last-emitted cursor/selection (e.g.,
lastCursorPos and lastSelection) and only calls cursorMoveCallback(pos,
selection) when either blockId or offset of the cursor or any anchor/focus
values in selection actually differ from those last-emitted values; ensure you
initialize those last-* variables near where cursorMoveCallback is declared and
update them only when you call cursorMoveCallback so subscribers are only
notified on real changes.
In `@packages/frontend/src/app/slides/slides-detail.tsx`:
- Around line 200-224: The file input handler can throw when
workspaceId/upload/insert fail; update handleImagePick to guard readiness and
handle errors: check workspaceId (or uploadFn readiness) before
creating/clicking the input, and wrap the onchange async logic (the call to
insertImageOnSlide which uses uploadFn/uploadImageFile) in a try/catch that logs
the error and surface a user-friendly message (e.g., via console.error or an
existing toast/error handler) instead of letting the promise reject silently;
reference functions/vars to change: handleImagePick, uploadFn, workspaceId,
insertImageOnSlide, uploadImageFile, store, editor.
In `@packages/frontend/src/app/slides/toolbar/image-controls.tsx`:
- Line 1: The alt-text draft state (draft) is only initialized from value once,
so when the selected image or value changes the UI can show or save stale alt
text; update the component in
packages/frontend/src/app/slides/toolbar/image-controls.tsx to synchronize draft
with value by adding an effect that runs when value (or the selected image
identifier) changes and sets draft to the new value (or empty string if
null/undefined). Locate the draft state and its initializer alongside the value
prop/variable and add a useEffect that calls the setter (setDraft) whenever
value (or the selected image id) changes to keep the draft in sync.
- Around line 54-69: The onReplace handler currently calls await
replaceImageOnSlide(...) inside the input.onchange async callback without error
handling, which can produce unhandled promise rejections; wrap the await
replaceImageOnSlide(...) call in a try/catch inside the input.onchange handler,
and on error call a meaningful error reporter (e.g., console.error or the app's
toast/logger) including context like store, slideId and elementId (firstId) and
the original error message so failures are explicit and debuggable.
In `@packages/frontend/src/app/slides/toolbar/shape-controls.tsx`:
- Around line 101-123: The Fill menu trigger (the button inside
DropdownMenuTrigger/TooltipTrigger) is currently only disabled when !store ||
!slideId but the color picker is only rendered when theme exists, allowing an
empty menu to open; update the trigger's disabled condition to also include
!theme (e.g., disabled={!store || !slideId || !theme}) so
DropdownMenuTrigger/TooltipTrigger cannot open when theme is absent and the
ThemedColorPicker won't be accessible.
In `@packages/frontend/src/app/slides/yorkie-slides-store.ts`:
- Around line 799-815: The Yorkie text reconstruction paths (e.g., in
addElement, replaceRoot, the text read mapping and withTextElement write-back)
are overwriting el.data with a { blocks }-only object and dropping extra
persisted keys like box-level fill/stroke; update those sites to use the same
key-by-key preservation/merge used here (the logic around source, merged, clone,
and deleting keys on undefined) so that when rebuilding a text element you first
copy existing e.data, iterate source entries to delete when value === undefined
or clone/assign otherwise, and then assign the merged object back to e.data;
apply this pattern in addElement, replaceRoot, and any text backfill functions
that currently set el.data = { blocks } to ensure Yorkie reads/writes keep
non-block fields.
In `@packages/frontend/src/components/text-formatting/text-format-group.tsx`:
- Around line 38-54: The toggleBold/toggleItalic/toggleUnderline handlers
correctly flip styles on click but the button "pressed" state isn't driven from
the editor selection, causing UI drift; update TextFormatGroup to derive each
button's pressed prop from editor.getSelectionStyle() (e.g., compute current =
editor?.getSelectionStyle() and pass pressed={!!current?.bold} for the Bold
button, pressed={!!current?.italic} for Italic and
pressed={!!current?.underline} for Underline) and apply the same pattern for the
other format buttons rendered later in the component so the visual pressed state
always reflects the live selection style.
- Around line 2-5: The toolbar claims Strikethrough support but the UI never
renders it; update the TextFormatGroup component to render a Strikethrough
control alongside Bold/Italic/Underline by adding the missing control button in
the same control list (where Bold/Italic/Underline are created) and wire it to
the existing mark toggle handler used by the other controls (e.g., the
onToggleMark/onToggleFormat or editor.toggleMark equivalent) using the same
pattern for active state, tooltip, and icon so it behaves consistently with the
other formatting buttons.
In `@packages/frontend/src/components/text-formatting/text-style-options.ts`:
- Around line 70-78: getBlockLabel currently treats a "heading" type with a
missing headingLevel as "Normal text"; update getBlockLabel (and its type
signature) to explicitly handle this invalid state: either make headingLevel
required when type is "heading" by tightening the function signature (use a
discriminated union for BlockType/HeadingLevel), or add explicit runtime
handling inside getBlockLabel to use a default level (e.g., 1) or throw a
descriptive error when type === "heading" and headingLevel is undefined; modify
callers or types accordingly so errors are surfaced at compile time if you
choose the discriminated union approach.
In `@packages/slides/src/view/editor/editor.test.ts`:
- Around line 794-799: The test is asserting rotation on e1 while the no-op
rotateBy call was made with selection [e2], so update the assertions to verify
the selected element's rotation (e2) is unchanged and still 0 and optionally
keep/assert that e1 remains unchanged too; locate the test block referencing
rotateBy, selection [e2], and the variables e1/e2 in editor.test.ts and change
the el1 lookup/assert to target e2 (or add a separate el2 lookup/assert) so the
test actually checks the element that was attempted to be rotated.
---
Nitpick comments:
In `@packages/frontend/tests/app/slides/toolbar/shape-controls.test.ts`:
- Around line 24-48: The test duplicates the production on-stroke handler logic
via makeOnStrokeChange; instead extract the pure handler from the production
module (shape-controls.tsx) or create a shared helper and import it into both
the component and this test, then replace the local makeOnStrokeChange with an
import of that symbol. Ensure the shared handler preserves the same signature
(store, slideId, elements, ids) and uses store.batch plus
store.updateElementData and store.updateConnectorStroke for 'shape' vs
'connector' so the test asserts the real implementation rather than a mirrored
copy.
In `@packages/slides/src/view/editor/editor.ts`:
- Around line 597-605: The loop repeatedly calls this.options.store.read() and
clones the whole document per iteration; instead read the slide once (const live
= this.options.store.read().slides.find(s => s.id === slideId)) before the loop,
build the orderedId -> fixed targetIndex mapping (e.g., start at
live.elements.length and increment for each id or compute desired insertion
indices ahead of time) and then call this.options.store.reorderElement(slideId,
id, targetIndex) for each id without further store.read() calls, using the
existing orderedIds, slideId and reorderElement symbols.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e5c7dbb7-b73d-472e-ba71-9fb8b224f54e
⛔ Files ignored due to path filters (78)
packages/frontend/tests/visual/baselines/harness-visual.browser.desktop.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.mobile.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.mobile.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.shapes-action-buttons.mobile.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.shapes-action-buttons.mobile.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.shapes-adjustments-p3b-arrows.desktop.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.shapes-adjustments-p3b-arrows.mobile.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.shapes-adjustments-p3b-arrows.mobile.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.shapes-adjustments-p3b-arrows.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.shapes-adjustments-p3b-basics.desktop.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.shapes-adjustments-p3b-basics.mobile.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.shapes-adjustments-p3b-basics.mobile.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.shapes-adjustments-p3b-basics.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.shapes-adjustments-pilot.mobile.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.shapes-adjustments-pilot.mobile.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.shapes-adjustments-sweep.mobile.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.shapes-adjustments-sweep.mobile.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-canvas-callout-tail.mobile.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-canvas-callout-tail.mobile.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-canvas-default-dark.mobile.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-canvas-default-dark.mobile.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-canvas-default-light.mobile.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-canvas-default-light.mobile.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-canvas-donut-evenodd.mobile.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-canvas-donut-evenodd.mobile.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-canvas-focus.mobile.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-canvas-focus.mobile.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-canvas-layout-big-number.mobile.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-canvas-layout-big-number.mobile.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-canvas-layout-section-header.mobile.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-canvas-layout-section-header.mobile.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-canvas-layout-title-body.mobile.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-canvas-layout-title-body.mobile.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-canvas-material.mobile.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-canvas-material.mobile.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-canvas-shapes-catalog-dark.mobile.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-canvas-shapes-catalog-dark.mobile.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-canvas-shapes-catalog-light.mobile.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-canvas-shapes-catalog-light.mobile.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-canvas-shapes-catalog-material.mobile.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-canvas-shapes-catalog-material.mobile.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-canvas-streamline.mobile.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-canvas-streamline.mobile.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-pickers.mobile.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-pickers.mobile.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-theme-panel.desktop.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-theme-panel.mobile.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-theme-panel.mobile.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-theme-panel.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-toolbar-idle.desktop.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-toolbar-idle.mobile.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-toolbar-idle.mobile.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-toolbar-idle.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-toolbar-image-selected.desktop.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-toolbar-image-selected.mobile.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-toolbar-image-selected.mobile.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-toolbar-image-selected.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-toolbar-multi-select.desktop.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-toolbar-multi-select.mobile.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-toolbar-multi-select.mobile.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-toolbar-multi-select.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-toolbar-shape-selected.desktop.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-toolbar-shape-selected.mobile.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-toolbar-shape-selected.mobile.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-toolbar-shape-selected.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-toolbar-text-editing.desktop.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-toolbar-text-editing.mobile.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-toolbar-text-editing.mobile.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-toolbar-text-editing.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-toolbar-text-element-selected.desktop.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-toolbar-text-element-selected.mobile.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-toolbar-text-element-selected.mobile.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-toolbar-text-element-selected.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-toolbar.desktop.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-toolbar.mobile.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-toolbar.mobile.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.slides-toolbar.pngis excluded by!**/*.png
📒 Files selected for processing (62)
docs/design/README.mddocs/design/slides/slides-toolbar-redesign.mddocs/tasks/active/20260515-slides-toolbar-redesign-lessons.mddocs/tasks/active/20260515-slides-toolbar-redesign-todo.mdpackages/docs/src/view/text-box-editor.tspackages/docs/test/view/text-box-editor.test.tspackages/frontend/scripts/verify-visual-browser.mjspackages/frontend/src/app/docs/docs-formatting-toolbar.tsxpackages/frontend/src/app/harness/visual/slides-scenarios.tsxpackages/frontend/src/app/slides/insert-image.tspackages/frontend/src/app/slides/replace-image.tspackages/frontend/src/app/slides/slides-detail.tsxpackages/frontend/src/app/slides/slides-formatting-toolbar.tsxpackages/frontend/src/app/slides/toolbar/arrange-menu.tsxpackages/frontend/src/app/slides/toolbar/border-picker.tsxpackages/frontend/src/app/slides/toolbar/global-controls.tsxpackages/frontend/src/app/slides/toolbar/idle-section.tsxpackages/frontend/src/app/slides/toolbar/image-controls.tsxpackages/frontend/src/app/slides/toolbar/index.tsxpackages/frontend/src/app/slides/toolbar/insert-group.tsxpackages/frontend/src/app/slides/toolbar/object-section.tsxpackages/frontend/src/app/slides/toolbar/shape-controls.tsxpackages/frontend/src/app/slides/toolbar/slide-group.tsxpackages/frontend/src/app/slides/toolbar/state.tspackages/frontend/src/app/slides/toolbar/text-edit-section.tsxpackages/frontend/src/app/slides/toolbar/text-element-controls.tsxpackages/frontend/src/app/slides/yorkie-slides-store.tspackages/frontend/src/components/text-formatting/index.tspackages/frontend/src/components/text-formatting/platform.tspackages/frontend/src/components/text-formatting/text-format-group.tsxpackages/frontend/src/components/text-formatting/text-paragraph-group.tsxpackages/frontend/src/components/text-formatting/text-style-group.tsxpackages/frontend/src/components/text-formatting/text-style-options.tspackages/frontend/src/components/text-formatting/types.tspackages/frontend/src/components/ui/toolbar.tsxpackages/frontend/tests/app/docs/text-style-group.test.tspackages/frontend/tests/app/slides/insert-image.test.tspackages/frontend/tests/app/slides/replace-image.test.tspackages/frontend/tests/app/slides/toolbar/arrange-menu.test.tspackages/frontend/tests/app/slides/toolbar/idle-section.test.tspackages/frontend/tests/app/slides/toolbar/image-controls.test.tspackages/frontend/tests/app/slides/toolbar/insert-group.test.tspackages/frontend/tests/app/slides/toolbar/object-section.test.tspackages/frontend/tests/app/slides/toolbar/shape-controls.test.tspackages/frontend/tests/app/slides/toolbar/state.test.tspackages/frontend/tests/app/slides/toolbar/text-edit-section.test.tspackages/frontend/tests/app/slides/toolbar/text-element-controls.test.tspackages/slides/src/index.tspackages/slides/src/model/element.test.tspackages/slides/src/model/element.tspackages/slides/src/node.tspackages/slides/src/store/memory.tspackages/slides/src/store/store.tspackages/slides/src/view/canvas/connector-renderer.tspackages/slides/src/view/canvas/render-context.test.tspackages/slides/src/view/canvas/render-context.tspackages/slides/src/view/canvas/shape-renderer.tspackages/slides/src/view/canvas/shape-special.tspackages/slides/src/view/editor/editor.test.tspackages/slides/src/view/editor/editor.tspackages/slides/src/view/editor/text-box-editor.test.tspackages/slides/src/view/editor/text-box-editor.ts
💤 Files with no reviewable changes (1)
- packages/frontend/src/app/slides/slides-formatting-toolbar.tsx
|
|
||
| The toolbar is one row organized into four zones: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Add language tags to fenced code blocks to satisfy markdownlint (MD040).
These fences should be annotated (for example ts or text) so lint stays clean and renderers can format consistently.
Also applies to: 118-118, 143-143, 159-159, 174-174, 190-190, 199-199, 227-227, 248-248, 270-270
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 69-69: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/slides/slides-toolbar-redesign.md` at line 69, Several fenced
code blocks in slides-toolbar-redesign.md are missing language tags and trigger
markdownlint MD040; locate the bare triple-backtick code fences ("```") used
throughout this document and add an appropriate language identifier (e.g., `ts`,
`text`, `bash`, etc.) after the opening fence so each block becomes "```ts" or
similar; ensure each changed fence matches the code content and re-run the
linter to confirm MD040 is resolved.
Collaborator
Author
There was a problem hiding this comment.
Acknowledged but skipping in this PR. The fenced blocks are inside a long architectural spec; adding language tags is a doc-wide cleanup that belongs in its own commit (other Slides specs in docs/design/slides/ have the same untagged fences). Will batch-fix when next touching the docs.
There was a problem hiding this comment.
@hackerwins, understood — batching the language-tag cleanup across all the Slides design docs makes sense. Would you like me to open a GitHub issue to track the batch MD040 fix so it doesn't slip through the cracks?
Comment on lines
+70
to
+78
| export function getBlockLabel( | ||
| type: BlockType, | ||
| headingLevel?: HeadingLevel | ||
| ): string { | ||
| if (type === "title") return "Title"; | ||
| if (type === "subtitle") return "Subtitle"; | ||
| if (type === "heading" && headingLevel) return `Heading ${headingLevel}`; | ||
| return "Normal text"; | ||
| } |
There was a problem hiding this comment.
Handle heading blocks without a level explicitly.
If type is "heading" but headingLevel is undefined or falsy, the function falls through to return "Normal text", which seems incorrect for a heading block type.
Consider either:
- Making
headingLevelrequired (non-optional) whentype === "heading"via a more precise type signature, or - Providing an explicit default heading level, or
- Validating and throwing an error for this invalid state
Proposed fix: explicit handling
export function getBlockLabel(
type: BlockType,
headingLevel?: HeadingLevel
): string {
if (type === "title") return "Title";
if (type === "subtitle") return "Subtitle";
- if (type === "heading" && headingLevel) return `Heading ${headingLevel}`;
+ if (type === "heading") {
+ return headingLevel ? `Heading ${headingLevel}` : "Heading 1";
+ }
return "Normal text";
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function getBlockLabel( | |
| type: BlockType, | |
| headingLevel?: HeadingLevel | |
| ): string { | |
| if (type === "title") return "Title"; | |
| if (type === "subtitle") return "Subtitle"; | |
| if (type === "heading" && headingLevel) return `Heading ${headingLevel}`; | |
| return "Normal text"; | |
| } | |
| export function getBlockLabel( | |
| type: BlockType, | |
| headingLevel?: HeadingLevel | |
| ): string { | |
| if (type === "title") return "Title"; | |
| if (type === "subtitle") return "Subtitle"; | |
| if (type === "heading") { | |
| return headingLevel ? `Heading ${headingLevel}` : "Heading 1"; | |
| } | |
| return "Normal text"; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/frontend/src/components/text-formatting/text-style-options.ts`
around lines 70 - 78, getBlockLabel currently treats a "heading" type with a
missing headingLevel as "Normal text"; update getBlockLabel (and its type
signature) to explicitly handle this invalid state: either make headingLevel
required when type is "heading" by tightening the function signature (use a
discriminated union for BlockType/HeadingLevel), or add explicit runtime
handling inside getBlockLabel to use a default level (e.g., 1) or throw a
descriptive error when type === "heading" and headingLevel is undefined; modify
callers or types accordingly so errors are surfaced at compile time if you
choose the discriminated union approach.
Collaborator
Author
There was a problem hiding this comment.
Acknowledged but not changing in this PR. The getBlockLabel helper was lifted from the existing docs toolbar verbatim during the extraction; the "heading without level" path is unreachable today since setBlockType('heading', ...) always supplies a level (see docs-formatting-toolbar.tsx). Tightening to a discriminated-union type === 'heading' & headingLevel belongs in a follow-up that touches the docs editor's BlockType too.
There was a problem hiding this comment.
@hackerwins, understood — that context makes sense. Since the extraction was verbatim and setBlockType('heading', ...) always supplies a level today, the path is indeed unreachable at runtime.
Would you like me to open a follow-up GitHub issue to track the discriminated-union tightening for getBlockLabel / BlockType so it doesn't get lost?
✏️ Learnings added
Learnt from: hackerwins
Repo: wafflebase/wafflebase PR: 244
File: packages/frontend/src/components/text-formatting/text-style-options.ts:70-78
Timestamp: 2026-05-16T01:01:30.064Z
Learning: In `packages/frontend/src/components/text-formatting/text-style-options.ts`, the `getBlockLabel` function's `heading` branch without a `headingLevel` is intentionally unreachable: `setBlockType('heading', ...)` always supplies a level in the current codebase. Tightening the signature to a discriminated union (covering `type === 'heading'` requiring `headingLevel`) is deferred to a follow-up PR that will also touch the docs editor's `BlockType` definition. Do not flag this as a bug in PR reviews.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Major: - yorkie-slides-store: text-element read path now preserves all data fields (fill, stroke, …) alongside the CRDT-backed `blocks`. Without this, the box-level fill/border landed earlier in this PR would silently disappear after any read or undo. - text-format-group: add the missing Strikethrough toggle (the JSDoc already promised it) and bind Bold/Italic/Underline/Strike `pressed` state to the live editor.getSelectionStyle() so toggles reflect the caret's actual formatting after every cursor move. - text-box-editor (docs): onCursorMove fires only when the cursor or selection key actually changed, so cursor-blink re-renders no longer thrash subscribers. - image-controls: AltText draft re-syncs on `value` change so switching to a different image doesn't show stale alt text. Wraps Replace upload in try/catch + toast.error so an upload failure no longer bubbles as an unhandled rejection. - slides-detail handleImagePick: guards on workspaceId, wraps the insert call in try/catch + toast.error. Minor: - shape-controls Fill button now also disables when no theme is loaded (previously rendered an empty picker). - editor.test.ts text-editing-guard test: assert rotation on the actually-selected element (e2), not e1.
text-format-group gained the Strikethrough toggle and now binds B/I/U/Strike pressed state to the live selection style, so the slides-toolbar-text-editing scenario shifts (extra button + active toggle highlights). Other baselines refresh from the already-flagged pre-existing mobile-dark drift.
The shared text-formatting extraction (Bold/Italic/Color/Align/List groups) and the per-section split of slides/toolbar/* (insert, arrange, object, image, text-element, text-edit) cause vite to emit extra dynamically-imported chunks. Measured count is 87; +3 headroom.
hackerwins
added a commit
that referenced
this pull request
May 16, 2026
Moves the toolbar redesign todo + lessons to docs/tasks/archive/ and refreshes the active / archive READMEs.
14 tasks
This was referenced May 29, 2026
This was referenced Jul 11, 2026
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
1 participant
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.
Summary
SlidesFormattingToolbarwith a single morphing toolbar driven by editor selection state: Idle / Object-selected / Text-editing.packages/frontend/src/components/text-formatting/consumed by both the docs toolbar and the slides text-editing state.SlidesEditor(isTextEditing,getActiveTextEditor,onTextEditingChange,enterTextEditing,exitTextEditing) and extendsTextBoxEditorAPIwith the 15 formatting methods the shared components need.ShapeElement.data.stroke.dash,TextElement.data.stroke,TextElement.data.fill. New store methodupdateConnectorStroketo handle the connector's top-levelstrokefield.MemSlidesStore.updateElementData(and nowYorkieSlidesStore.updateElementData) preserve explicitundefinedpatches so clears like Reset Crop actually clear.Spec:
docs/design/slides/slides-toolbar-redesign.md· Plan + lessons:docs/tasks/active/20260515-slides-toolbar-redesign-todo.mdand-lessons.md.Test plan
pnpm verify:fast— green (773 tests)pnpm verify:browser:docker— 6 new slides scenarios (idle / shape-selected / image-selected / text-element-selected / text-editing / multi-select) need first-run baselines; existing docs/slides baselines should not diffKnown limitations (follow-ups in lessons file)
frame.flipH/flipVmodel fields).bringForward/sendBackwardmulti-select has an edge case when adjacent selected elements sit at the top of the z-stack (relative order can shift by 1); flagged for follow-up.ShapeStrokedeprecated alias still re-exported forconnector.ts; full migration toStrokeis a follow-up.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation