Skip to content

fix(desktop): center compact appearance choices - #3564

Merged
Astro-Han merged 1 commit into
apache:mainfrom
ARE404:codex/fix-appearance-choice-alignment
Aug 23, 2026
Merged

fix(desktop): center compact appearance choices#3564
Astro-Han merged 1 commit into
apache:mainfrom
ARE404:codex/fix-appearance-choice-alignment

Conversation

@ARE404

@ARE404 ARE404 commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Summary

  • vertically center compact palette and app-icon choices when their grid row stretches to fit wrapped content
  • keep media-led theme cards top-aligned and leave the shared SelectableCard primitive unchanged
  • add geometry-based Electron coverage for the palette and app-icon regressions

Root cause

Astryx Grid stretches cards to the tallest item in each row, while the compact choice HStack kept its natural height. Shorter content therefore remained pinned to the card padding-top instead of sharing the extra vertical space.

Before / after

Before After
Before: compact appearance choices align to the top of stretched cards After: compact appearance choices are vertically centered

Testing

  • npm run build:renderer (desktop workspace)
  • npm run typecheck
  • npx playwright test e2e/settings.spec.ts --config e2e/playwright.config.ts --workers=1 (4 passed)
  • spacing detector scan (clean)

@ARE404
ARE404 marked this pull request as ready for review August 23, 2026 07:31

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed exact head 0af6e0371736e299b0106d45ba745691f2dc6cc6 — required test is completed / success bound to that SHA. No P0–P3.

The production change is two lines: height="100%" on the HStack inside the palette and app-icon SelectableCards. That is the correct fix rather than a nudge — the cards sit in a grid whose rows stretch to the tallest sibling, so the content box was keeping its natural height and sitting at the top of a taller row. align="center" had nothing to center against; giving the box the row's full height is what makes the existing alignment take effect.

The e2e test is the part worth calling out. It asserts cardHeight > contentHeight before asserting that the top and bottom gaps match within a pixel. That ordering is what makes it a real regression test: without the first assertion, a card whose height happens to equal its content height would satisfy the gap check trivially, and reverting height="100%" would leave the test green. Checking that the row is genuinely stretched first means the test can only pass when the condition it cares about actually exists. It also drives the real surface — sets the locale, reloads, sizes the viewport to a width where the grid actually stretches — instead of asserting against a prop.

Two small notes, neither a finding: getByRole('checkbox', { name }).locator('..') couples the test to the card's DOM nesting, so a wrapper element added inside SelectableCard would break it in a way that reads as a layout failure; and the geometry helper picks the first non-INPUT child, which is fine today but is the same coupling stated twice. Both are reasonable trade-offs for a layout assertion that has no test id to aim at.

Worth noting for whoever picks up the follow-up: these cards are still exposed with role="checkbox" for what is a single-choice control. That is the shared SelectableCard issue already tracked from #3431, not something this PR should take on.


AI-assisted review. The stretch behaviour and the test's ordering were verified against the source at this exact head. Under CONTRIBUTING.md §Review this does not count as the required independent human review — merge still needs a human committer other than the author to give LGTM and to decide.

@Astro-Han
Astro-Han merged commit 729839e into apache:main Aug 23, 2026
1 check passed
@ARE404
ARE404 deleted the codex/fix-appearance-choice-alignment branch August 23, 2026 07:52
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