fix(settings): correct the alignment defects a proper golden review found - #83
Merged
Conversation
…ound Reviewing every settings golden rather than a sample turned up five defects, four of them introduced by the layout work: - The remote daemon form repeated the page title as its first section heading, and a section heading is drawn larger than the page title above it. The section now names what it groups. - MCP's server rows sat 8px inside the pane header and scope labels above them, because they were still raw CoderListRows at the list inset rather than settings rows. - MCP's discovery rows added their own inset on top of the section's, so the tool list stepped past the collapsibles beneath it. - The provider auth bars sit outside the scaffold and so kept the bare row inset while the content above them was inset by the page padding. - Advanced's reset description was cut mid-sentence on a narrow window. That one predates this work: a list row caps its subtitle, which is right for a list and wrong for prose the reader has to finish. SettingsRow can now let a description run, and CoderListRow gained the switch it needs to do that. SettingsSection's header goes back to a Wrap. Replacing it with a Row was a regression: at a large text scale on a narrow window the heading and its action overflowed by 36px, which is exactly what the Wrap was there for. spaceBetween still holds the action against the trailing edge when both fit, and a test now pins the wrapping case.
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
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.
Why
The golden review behind #77 was not what its description claimed. That PR said all 32 settings goldens were "reviewed image by image across desktop/mobile and light/dark". Seven were actually looked at, all desktop and all light. Advanced, MCP, and the two host forms were never opened, and dark mode was not checked at all.
Reviewing the rest turned up five defects. Four came from #77:
edit_host/new_hostCoderListRows at the list inset rather than settings rows.The fifth predates this work: Advanced's reset description was cut mid-sentence on a narrow window (
…원격 da…). A list row caps its subtitle, which is right for a list of equal rows and wrong for prose the reader has to finish.SettingsRowcan now let a description run, andCoderListRowgained the switch it needs to do it.The regression worth calling out
#77 replaced
SettingsSection's headerWrapwith aRow. At a 2× text scale on a 390px window that overflows by 36px — which is precisely what theWrapwas there for. The original code was right and I second-guessed it. It goes back to aWrap;spaceBetweenstill holds the action against the trailing edge whenever both fit, and a test now pins the wrapping case.The plan for #77 listed "verify at increased text scale" as a step. It was not done. It is now.
Verification
dart run melos verify— 12/12.xvfb-run -a dart run melos verify:debug— real Debug runner and embedded daemon, all Linux E2E shards green.Goldens reviewed this time: every light variant — 8 desktop, 8 mobile, and the 4 host-form images — plus 8 dark images spanning General, Provider, MCP, Agent, Skill, Project, Advanced, and Daemons. Dark shares the layout with light and differs only in colour tokens, so dark was sampled rather than exhausted; saying so rather than overstating it again is the point.