Phase 9: Editor UX Polish (UX-03/04/07) + v1.2 scaffolds#35
Merged
Conversation
…privileged-tier backlog item
…08, BUG-06, BUG-07)
Write failing node:test spec for modeStatusLabel(state, strings) before implementation exists. Covers idle→''/saving/saved/error/unknown states. Function is not yet on maestro-logic.js api — all 5 assertions fail with TypeError as expected.
Add modeStatusLabel(state, strings) to assets/maestro-logic.js api object. Maps 'saving'/'saved'/'error' to the corresponding i18n string; returns '' for 'idle' and any unknown state so the save-status element stays empty. The persistent 'Edit Mode' mode label is DOM-built (Plan 02), not here. All 5 mode-status.test.mjs assertions pass.
Write failing node:test specs for firstRunSeen(storage) and placeholderVisible(value) before implementations exist. firstRunSeen covers null/getItem='1'/getItem='0'/throws-→-true; placeholderVisible covers empty/whitespace-only/non-empty. All 7 assertions fail with TypeError as expected.
Add firstRunSeen(storage) and placeholderVisible(value) to the maestro-logic.js api object. firstRunSeen reads 'maestroFirstRunDone' from an injected storage stub; returns true (suppress cue) when storage throws, guarding against unavailable localStorage. placeholderVisible mirrors commitRename's trim()==='' rule — whitespace-only counts as empty. All 7 assertions pass; npm run test:js is 56/56 green including Phase 6 suites unregressed.
…placeholderVisible Record execution results: 3 new test files, maestro-logic.js extended with 3 pure helpers, 56/56 npm run test:js green. Update STATE.md, ROADMAP.md, and mark UX-03/UX-04 requirements complete.
- idle string changed from verbose 'Editor active — click an item to edit.' to 'Edit Mode' - new modeLabel key added to i18n payload (persistent mode label text) - LocalizationTest::expected_i18n_keys() updated with modeLabel in the same commit - idle key retained (value changed; key preserved for LocalizationTest)
- buildToolbar: persistent .maestro-mode-label (aria-hidden dashicons-edit + "Edit Mode") sits beside a separate transient .maestro-status (aria-live, empty at idle) - setStatus: routes through window.maestroLogic.modeStatusLabel() — returns '' at idle so save-status element self-empties; wp.a11y.speak() retained for saved/error - CSS: .maestro-mode-label (green flex child, dashicon aligned) replaces old comment block; .maestro-status::before still handles saving/saved/error glyphs (BUG-05 intact) - Idle dashicon is a real DOM span (aria-hidden); not a ::before; avoids BUG-04
- new UX-03 describe block: .maestro-mode-label visible with .dashicons child and "Edit Mode" text; save-status is a separate element empty at idle - idle .maestro-status::before content:none guard preserved (Phase 7 guard unchanged; also re-stated explicitly in UX-03 block for clarity) - three new tests cover all plan done-criteria for the DOM split
…-03) - Swap buildFirstRunCue gate to window.maestroLogic.firstRunSeen() (Plan-01 seam) - Add maestro-firstrun-pulse class to first #adminmenu > li.menu-top.maestro-item - Register one-shot animationend handler to remove the class (motion path) - Extend dismiss() to ALSO remove the class (reduced-motion / early-dismiss path) - Add @Keyframes maestro-pulse-item (1.5s, iteration-count 1, outline-based, zero reflow) - Add prefers-reduced-motion fallback: animation:none + static outline (no stuck class)
- Add UX-03 describe block: pulse class present on first editable item after cue shows - Assert pulse class absent after dismiss() (the reduced-motion cleanup path) - Reuses the same localStorage-clear setup as the Phase 7 first-run gate test
- SUMMARY: one-shot pulse + dual-cleanup pattern documented - STATE: progress 75%, metrics recorded, two decisions added - ROADMAP: Phase 9 progress updated (3/6 plans complete)
- Add renamePlaceholder key ('Menu label') to class-assets.php i18n array
- Retain rename key (now used as SR-only accessible label text, not visible node)
- Add renamePlaceholder to LocalizationTest::expected_i18n_keys() in same commit
so integration never goes red between commits (Pitfall 5)
- phpcbf auto-aligned all i18n array double-arrows to WPCS standard
…X-04)
- Replace implicit label wrapper renameField with explicit SR-only label:
- <label class='screen-reader-text' for='maestro-rename-field'> carries I.rename
- input#maestro-rename-field gets placeholder = I.renamePlaceholder ('Menu label')
- Visible 'Rename ' text node (createTextNode) removed
- Keydown (Enter/Escape) and blur->commitRename handlers unchanged
- panel.rename reference unchanged; populatePanel/commitRename untouched
- Panel append: p.appendChild(renameLabel); p.appendChild(rename) — BUG-02 order kept
- Add ::placeholder rule with color:#8c8f94 (WP muted-text token) + opacity:1
to fix Firefox's default 0.54 opacity and meet AA non-text contrast
- Assert no .maestro-panel-field element (the visible renameField wrapper is gone)
- Assert input#maestro-rename-field has placeholder='Menu label'
- Assert page.getByLabel('Rename') resolves the rename input (for/id accessible name)
- Assert rename input is pre-filled with selected item title (populatePanel unchanged)
- Placeholder shows only when value is empty (browser-native behavior, field pre-filled)
- 09-04-SUMMARY.md: 3 tasks, 5 files, 18min - STATE.md: progress updated to 81% (13/16 plans), decisions + session recorded - ROADMAP.md: Phase 9 updated to 4/6 summaries
…2px (UX-07) - Add gap/padding density rules to .maestro-toolbar at <=782px - Add padding 4px 8px, font-size 12px, min-height 44px to .maestro-toolbar .button - Add padding 0 6px, font-size 12px, min-height 44px to .maestro-rename-input - Preserve existing left:0 and icon-picker touch-target rules - No layout restructure (restructure decision gated on 700px screenshot checkpoint)
- Add UX-07 describe block asserting every .maestro-toolbar .button >= 44px tall at 700px - Assert .maestro-rename-input boundingBox height >= 44 at 700px with panel open - Existing 700px no-overflow and no-overlap guards preserved (no modifications to those tests)
…tatus split) + 700px density screenshot The Phase 7 no-overflow test asserted .maestro-status visible, but 09-02 deliberately made that element empty/hidden at idle (persistent indicator is now .maestro-mode-label). Updated the assertion; both 700px/1200px toolbar tests pass. Captured the 700px density screenshot for the UX-07 checkpoint. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-run banner alignment) from Phase 9 screenshot review
Found by running the full Playwright suite (sandbox-disabled) mid-phase — the per-task executors could not run e2e (Docker blocked), so these slipped through: - UX-07 (09-05): rename input rendered 40px, not the 44px tap-target floor — WP core's own <=782px input rule (input[type=text], spec 0,1,1) overrode .maestro-rename-input (0,1,0). Scoped under .maestro-toolbar (0,2,0), matching the .button rule, so the 44px floor wins. - UX-03 (09-02) + Phase 7 status tests: asserted .maestro-status toBeVisible at idle, but 09-02 deliberately makes that element empty/zero-size at idle. Changed the precondition to toHaveCount(1); the real ::before content:none assertion is preserved (sibling test 704 already used this pattern). Full suite green: JS, PHP unit 44/44, integration 29/29, e2e 24/24. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…, no restructure - 09-05-SUMMARY.md: records density CSS (3012677), e2e tap-target assertion (2e58803), 700px screenshot checkpoint (38323c4), and regression fixes (927b682) - STATE.md: position advanced to 09-06 next; four 09-05 decisions added - ROADMAP.md: Phase 9 plan checkboxes 01–05 marked done (5/6); milestone column filled - REQUIREMENTS.md: UX-07 marked complete Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Deeper review of the Phase 9 diff surfaced two unused leftovers from the
status-split / rename refactor:
- 'idle' i18n key was orphaned — no JS reads it after the split (save-status is
empty at idle; modeStatusLabel never reads I.idle), and it duplicated 'modeLabel'
('Edit Mode') verbatim, giving translators a redundant unused string. Removed
from class-assets.php and LocalizationTest::expected_i18n_keys() (same commit).
- placeholderVisible() was exported + unit-tested but never consumed — the native
HTML placeholder attribute handles empty-state display. Removed the helper, its
export, and tests/js/placeholder.test.mjs.
Green: JS 53/0, PHP unit 44/44, integration 29/29, e2e 24/24, phpcs clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…mplete - Full suite green: JS logic 53/53, PHP unit 44/44, integration 29/29, e2e 24/24, phpcs clean, Plugin Check 0 errors on shippable source - UX-03/UX-04/UX-07 confirmed Complete in REQUIREMENTS.md v1.2 traceability - ROADMAP: 09-06 plan ticked; Phase 9 row → 6/6 Complete (2026-06-20); Edit Mode reconciliation note recorded in Phase 9 success criteria - STATE.md: position → Phase 11 (next on 1.2.0 release path); Phase 9 decisions + metrics added; session updated - 09-06-SUMMARY.md: gate results, regression story, code-review cleanup, and Phase 9 sign-off narrative
… scaffold Phase 10 dir (health clean)
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da29029579
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Owner
Author
|
@codex review for security regressions, missing tests, accessibility regressions, WordPress coding standards issues, and risky behavior changes. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da29029579
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- maestro.js: guard window.localStorage access at the firstRunSeen call site. In blocked/partitioned-storage browsers the localStorage GETTER throws a SecurityError when evaluated as the argument, before firstRunSeen's own try/catch — escaping uncaught and aborting edit-mode init. Wrapped the call so a throwing getter is treated as 'seen' (cue skipped), restoring pre-refactor safety while keeping the testable seam. - maestro.js: the first-run cue now measures the toolbar's offsetHeight and sets its bottom offset, instead of a fixed 53px. UX-07's 44px tap targets made the <=782px toolbar taller (and it wraps), so the cue was covered by the toolbar. - editor.spec.ts: new e2e guard asserts the cue's bottom edge stays above the toolbar's top edge at 700px (no overlap). e2e 25/25, JS 53/0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Owner
Author
|
Both Codex P2 findings fixed in 9ae3a87:
Both verified locally (e2e 25/25); CI re-running. |
…rast, test flake) - maestro.js: WCAG 2.5.3 (Label in Name) — the rename input's accessible name was 'Rename' but the visible placeholder is 'Menu label', so speech-control users saying 'Menu label' couldn't target it. Aligned the visually-hidden <label> to 'Menu label' (the visible text). The now-orphaned 'rename' i18n key is removed (class-assets.php + LocalizationTest, same commit); e2e getByLabel updated to 'Menu label'. - maestro.css: rename placeholder color #8c8f94 (~3.3:1) failed WCAG 1.4.3 text contrast (placeholder is the only visible field hint) → #50575e (~7:1). - editor.spec.ts: the first-run pulse test raced the 1.5s one-shot animation on slow CI; run it under emulated reduced-motion so the class persists until dismiss (deterministic, and exercises the reduced-motion cleanup path). Note: Codex's 'cached idle key' P2 is handled by the release version bump — assets enqueue with MAESTRO_VERSION, so 1.2.0 busts the cache. No code change. Full suite green: JS 53/0, PHP unit 44/44, integration 29/29, e2e 25/25, phpcs clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Owner
Author
|
Update — all 6 Codex findings now addressed (my earlier 'both fixed' note was premature; Codex ran a second pass with 4 more). Fixes in
Full suite green locally (JS 53, PHP 44/44, integration 29/29, e2e 25/25, phpcs clean). Thanks @codex — the Label-in-Name catch in particular was a real one my review missed. |
|
To use Codex here, create an environment for this repo. |
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.
Phase 9: Editor UX Polish — v1.2
Delivers the v1.2 editor UX polish (UX-03, UX-04, UX-07) plus the v1.2 planning scaffolds. Goal-verified 7/7 must-haves.
What's in it (code)
.maestro-mode-label("Edit Mode" + aria-hidden dashicon — WCAG 1.4.1, not colour alone), split from a separate transient save-status (Saving…/Saved/Save failed, empty at idle, routed through the testedmodeStatusLabelseam). One-shot ~1.5s pulse on the first editable menu item,firstRunSeen-gated, with aprefers-reduced-motionstatic fallback and dual class cleanup (animationend and dismiss —animationendnever fires under reduced motion).<label for>keeps the accessible name (a placeholder is not an accessible name); field stays pre-filled..maestro-toolbarto beat WP core's 40px input rule on specificity).Tests / quality (full suite, run locally with Docker)
modeStatusLabel,firstRunSeen).Notable: regressions caught + review cleanups
A full-suite gate run (the per-task sandbox can't run Docker-backed tests) caught 3 real e2e regressions — fixed in
927b682/38323c4: rename-input 44px losing to WP core on CSS specificity, and two stale.maestro-statustoBeVisibleassertions after the status split. A follow-up code review removed 2 dead-surface items (1ef7fae): an orphanedidlei18n key and an unusedplaceholderVisiblehelper+test.Also included (planning docs)
Release path: 9 → 11 → 12, then cut 1.2.0. Phase 10 runs independently.
🤖 Generated with Claude Code