fix(ui): deep-review round over the plan-reminder sheet - #2007
Merged
Conversation
Adversarial review of #2002/#2004 (correctness + design lenses, every finding verified before fixing). Confirmed regressions, fixed: - Autofocus was broken: raw `autoFocus` fires at React commit, before showModal() makes the dialog visible, so focus silently landed on the first button. The title input now wears `data-autofocus`, the hook Astryx Dialog actually queries after opening (this is also what the plan-reminders e2e asserts). - The raw title input carried none of the a11y wiring TextInput used to provide — aria-required / aria-invalid / aria-describedby now point at Field's generated status id, and Field gets isRequired back, so the validation message is announced again. - The `::backdrop` override derived its color from --foreground, which inverts in dark mode (a whitening veil over a dark UI). It derives from --background now — light wash over light, dark scrim over dark — and the sheet gains a hairline inline-start border so dark mode separates by ring before shadow. - The form's own `overflow: auto` was a no-op inside LayoutContent's `overflow: clip`; tall variants (cron + bot delivery) clipped the Chat ID row and Create with no way to scroll. LayoutContent is the single scroll region now (isScrollable), between the pinned header and footer Layout slots. Design-review completions: - 时间 row: hand-typed `YYYY-MM-DDTHH:mm` TextInput → Astryx DateTimeInput (compact, 24h, 5-min stepping, full-row width); the sheet widens 440 → 480 so the date+time pair fits its min-content with proper insets. - The inset group cards gain a real fill (--muted) — a border on the same surface color read as a bare outlined rectangle, not an inset. - The borderless title gets a :focus-visible ring (it was the one control with no focus indicator); its placeholder moves to the platform text-secondary token (the hand mix was below 4.5:1). - Group labels step up to the label type role so the kicker/label ladder reads as two tiers; presets keep role="group"; the sheet slides in from the right (Astryx's center scale-pop detached a 100dvh element from three window edges mid-animation). - Dead copy keys pruned (eyebrow, cancel, field.delivery, templatesAriaLabel); two classNames that landed on the menu popover instead of the trigger removed. Verified: build, typecheck, check-dead-css, check-a11y/copy/console, plan-reminder unit tests (9/9), Storybook smoke (74 renders x 3 viewports), rendered screenshot of the open sheet re-checked.
WAWQAQ round 3: the sheet was pinned to the WINDOW edge while the page content is width-clamped, leaving a dead band between the list and the form — the reference has none: its create panel sits flush against the list pane, one divider between, non-modal. - The form drops the <dialog> overlay entirely. It is an in-page <aside> now: when open, .maka-plan-panel becomes a two-column grid (list keeps flowing in column one; the panel takes minmax(380px, 460px) flush right) with a single hairline seam. Non-modal role="dialog" + aria-labelledby keeps the accessible name the e2e locates; Escape closes through the same guarded path via an onKeyDown at the panel root; the title autofocuses from a mount effect (the component still remounts per form session). - The aside is position: sticky with its own pinned header/footer and the form as the single scroll region between them, so the list can scroll independently — matching the reference's split behavior. - The sheet-era CSS (window-pinned dialog, custom ::backdrop) is retired; the slide-in animation survives on the aside. Verified: build, typecheck, check-dead-css, check-a11y/copy/console, plan-reminder unit tests 9/9, Storybook smoke (74 renders x 3 viewports), rendered screenshot re-checked against the reference.
jackwener
force-pushed
the
fix/plan-form-deep-review
branch
from
August 3, 2026 12:12
fd637b6 to
ebc63c3
Compare
The Astryx Dialog used to capture the focused element at open and restore it on close; the split-view aside dropped that contract, so Escape from the edit panel left focus on <body> — caught by the e2e that expects the row's menu trigger to regain focus. The open effect now captures the opener (the panel has already re-focused the triggering row control by then) and its cleanup hands focus back when the panel closes. Verified locally: both plan-reminders e2e tests pass.
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.
Adversarial deep review of #2002/#2004 (independent correctness + design lenses; every finding verified against sources before fixing).
Confirmed regressions, fixed
autoFocusfires at React commit — beforeshowModal()makes the dialog visible — so focus actually landed on the first button (template menu / X). The title input now wearsdata-autofocus, the hook AstryxDialogqueries after opening. (This is also exactly whatplan-reminders.spec.tsasserts.)Fieldnow carriesaria-required/aria-invalid/aria-describedby→ Field's generated status id, andFieldgetsisRequiredback — screen readers hear the validation again (FieldStatusdeliberately has no live region; it relies on describedby).--foreground(near-white in dark mode → a whitening veil). Now derives from--background, and the sheet gains a hairline inline-start border (ring before shadow, per dark-restraint).overflow:autowas a no-op insideLayoutContent'soverflow:clip— tall variants (cron + bot delivery) clipped Chat ID and Create unreachably.LayoutContent isScrollableis the single scroll region now, between pinned header/footer Layout slots.Design completions
YYYY-MM-DDTHH:mm→ AstryxDateTimeInput(compact, 24h, 5-min step, full-row width); sheet widens 440 → 480 so date+time fit min-content with clean insets.--muted) — border-on-same-surface read as an outline, not an inset.:focus-visiblering (it was the only chrome-free control with no focus cue); placeholder moves to the platform text-secondary token (hand mix was <4.5:1).role="group"; the sheet slides in from the right (Astryx's center scale-pop detached a 100dvh element from three edges mid-animation).eyebrow,cancel,field.delivery,templatesAriaLabel); two classNames that landed on the menu popover instead of the trigger removed.Verification
typecheck ✅ · check-dead-css ✅ · check-a11y/copy/console ✅ · plan-reminder unit tests 9/9 ✅ · Storybook smoke 74 renders × 3 viewports ✅ · re-rendered screenshot of the open sheet posted in the work thread