Skip to content

fix(client): align the term dialog's interval and every fields - #341

Merged
centralcmd merged 1 commit into
mainfrom
claude/festive-fermat-htsr6p
Oct 22, 2026
Merged

centralcmd merged 1 commit into
mainfrom
claude/festive-fermat-htsr6p

Conversation

@centralcmd

Copy link
Copy Markdown
Owner

Summary

Two alignment nits in the New term / Edit term dialog (AddTermDialog):

  1. Cadence echo ("Applies monthly.") sat far below the Interval field. It was rendered after the whole two-column row. It now sits inside the Interval cell (.trm-interval) as that field's helper line, at the same height as Every's "Leave blank for …" line. Its id is unchanged, so Every's aria-describedby still points to it.
  2. Interval and Every were misaligned, both vertically and horizontally.
    • Vertical (8px): MudSelect / MudDatePicker wrap their .mud-input-control in a div.mud-select / div.mud-picker. Because of that wrapper, the existing margin reset .odc-form-row > .mud-input-control never reached them. The reset now also covers > :is(.mud-select, .mud-picker) > .mud-input-control in .odc-form-row, .odc-form-grid and .edit-grid.
    • Horizontal (5px): MudBlazor's .mud-input-label-outlined { padding: 0 5px !important } won over the global label reset's padding: 0. The reset now uses !important.

The two CSS fixes apply across the app, not just this dialog. Any MudSelect or DatePicker sharing a form row with a DS field had the same offset, and every Mud label was indented 5px from DS labels.

Not changed: MudBlazor helper text (for example under "First billed on") is still indented 8px by Mud's px-2. That indent is app-wide and outside the scope of these two nits.

Type of Change

  • Bug fix

Testing Steps

  1. Open Contracts → expand a contract → ⋯ → New term, with kind Amount and interval Monthly.
  2. Check that the Interval and Every labels and boxes are top-aligned and flush left, and that "Applies monthly." sits directly under Interval, level with "Leave blank for monthly".
  3. dotnet test Odyssey.Client.Tests passed: 3710 passed, 0 failed, 35 skipped by tier.

Related Issues

None.


Generated by Claude Code

The cadence echo ("Applies monthly.") rendered after the whole two-column
row, so it trailed both fields instead of sitting under Interval like
Every's own help line. It now lives in the Interval cell.

Interval also sat 8px below Every with its label indented 5px. MudSelect
and MudDatePicker wrap their control in a .mud-select / .mud-picker div,
so the form-row/form-grid/edit-grid margin reset never reached them, and
MudBlazor's `padding: 0 5px !important` on outlined labels beat the
global label reset. Both rules now cover those cases app-wide.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NEZeEumWaYvxjuCWzCruDX

Copy link
Copy Markdown
Owner Author

✅ PR was reviewed by Security Agent. No security-relevant changes detected: Approved.


Generated by Claude Code

Copy link
Copy Markdown
Owner Author

✅ PR was reviewed by Senior Tester Agent and no issues were found.


Generated by Claude Code

Copy link
Copy Markdown
Owner Author

✅ PR was reviewed by Accessibility Agent and no issues were found: Approved.

Checked: the cadence echo keeps id="trm-cadence-echo". It renders only when IsPeriodicInterval is true (via ShowsCadenceEcho), which is the same condition that renders the Every field and its aria-describedby. The reference therefore never dangles, and the description relationship (1.3.1) is unchanged. DOM and reading order stay Interval, then Every. The echo text keeps text-secondary colour at 12px (--fs-caption, 0.75rem, same size as before). The CSS changes (margin reset on .mud-select/.mud-picker wrappers, label padding: 0 !important) are layout only. They do not touch focus rings, target size, or hidden/visible semantics.


Generated by Claude Code

Copy link
Copy Markdown
Owner Author

✅ PR was reviewed by Senior Architect Agent and no issues were found: Approved.

Scope: frontend-only markup and CSS, so reviewed for contract and architectural implications only.

  • No API, DTO, migration, auth or claims surface is touched. There is no contract drift.
  • The id="trm-cadence-echo" is unchanged. Every's aria-describedby still resolves through ShowsCadenceEcho, so no dangling reference. The existing TermSeriesSurfaceTests assertions look the echo up by class and id, not by position, so the move does not affect them.
  • The new --fw-regular and --fs-caption tokens exist in app.css.
  • The :is(.mud-select, .mud-picker) margin resets are scoped to the form-row, form-grid and edit-grid containers. .trm-interval > .mud-select > .mud-input-control is still needed because the new wrapper stops the generic direct-child rule reaching the select.

Non-blocking notes:

  • padding: 0 !important on the global Mud outlined label is app-wide, as the PR description says. It is worth a quick visual pass on other dialogs that use MudBlazor labels.
  • The .trm-interval gap of 6px stacks with Mud's own helper text only if IntervalHelp and the echo ever render together. Today they are mutually exclusive (periodic vs non-periodic).

Generated by Claude Code

Copy link
Copy Markdown
Owner Author

Senior Frontend Agent review

Verdict: no blocking issues. Approved from the frontend/design-system side (a review verdict, not a GitHub approval action).

Reviewed by static read of the diff and the PR-branch source. I did not run the app and did not judge pixel alignment, so the manual step 2 in the PR description is still worth a visual pass. I also did not compare against the design-system artifact.

What I verified

  • CadenceEchoId is unchanged and the echo is still rendered under the same ShowsCadenceEcho condition. Every's AriaDescribedBy therefore still points at a node that exists, and there is no dangling reference. ShowsCadenceEcho requires IsPeriodicInterval, so the echo and the non-periodic IntervalHelp never appear together under Interval.
  • .trm-interval > .mud-select > .mud-input-control { margin: 0 !important } is needed. The .odc-form-row rule cannot reach it because the div.trm-interval wrapper adds a level, so it is not redundant.
  • --fw-regular and --fs-caption are defined in app.css (400 and 0.75rem). The echo now uses tokens instead of the old literal 400 12px, which matches the token convention.
  • The .odc-form-row, .odc-form-grid and .edit-grid reset extensions use :is(.mud-select, .mud-picker), which is consistent with the existing selectors.
  • The label padding: 0 !important is scoped to .mud-input-label-outlined inside .mud-input-control, so the blast radius is limited to the labels the rule already targets. The comment explains why !important is needed.

Nits (non-blocking)

  1. Stale or duplicated comments in AddTermDialog.razor (around lines 219-225). Two adjacent @* *@ blocks now sit above .trm-interval. The first (the "unit carries its own label…" comment) still reads fine. The second ("The echo sits in the Interval cell…") partly repeats the CSS comment. Consider folding them into one.
  2. Wording of the Every comment. "The echo under Interval is named here too" is slightly ambiguous. "Every names the echo under Interval via aria-describedby" would be clearer.
  3. Cross-cell description. Every is described by text that now sits in the neighbouring Interval cell. It works, since ids are document-global and the echo is still in the DOM. It is worth a one-line comment so a later move of the echo into a scoped child component does not break the reference. This also touches the "moving markup to a child requires moving its scoped CSS" pitfall, although .trm-* is global CSS here.
  4. Verification. Nothing in the 3710 passing tests checks the new selectors. If the repo has a source-lint style test for CSS (as it does for caps), a small pin on the margin-reset selector set would stop the .mud-select/.mud-picker wrapper case from silently regressing. Optional.

No design-system, API, state or accessibility regressions found.


Generated by Claude Code

@centralcmd centralcmd self-assigned this Oct 22, 2026
@centralcmd
centralcmd marked this pull request as ready for review October 22, 2026 17:44
@centralcmd
centralcmd merged commit 95bfcd1 into main Oct 22, 2026
8 checks passed
@centralcmd
centralcmd deleted the claude/festive-fermat-htsr6p branch October 22, 2026 18:13
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.

2 participants