Repository navigation
refactor(web): share a page's status line in one global class - #187
Merged
Merged
Conversation
The PRs, Time, Nudge and Todos pages each drew the line that takes their sections' place from a copy of the same rules, and the GitHub page would have made a fifth. They now use .page-status from styles/page.css. On phones, the Nudge page's line leaves a section's 48 px below it, since its copy lacked the phone rule (decision 206).
Each page that draws the shared status line shows it muted, with a section's 64 px below it, or 48 px on phones. The Home's spec shares its --ink-muted probe through e2e/ink.ts.
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.
Brief
Goal: the line that takes a page's sections' place, such as "Bitbucket isn't set up.", comes from one global class, so the GitHub page's "GitHub isn't set up." doesn't make a fifth copy of its rules (decision 206, as decisions 112 and 205 did for the form styles and the polled sections).
Scope
apps/web/src/styles/page.css:.page-status, loaded bymain.tsafter the other global stylesheets.Out of scope: the GitHub page (
github-page); the polled sections' muted lines and the Notes pages' failure lines, which sit in a block that gives them their space; the Time page's own smaller space under its failure line when its sections follow.Acceptance criteria
.page-status.main, but for the class names,overflow-wrap: anywhereon the lines that lacked it, and the Nudge page's line on phones, which now leaves a section's 48 px below it rather than 64 px.mise run checkis green.What changed
.page-status: no margin, the page's padding at the sides,--ink-muted, 64 px below (48 on phones), and long words break rather than widen the page. Before, only the Time page's copy broke long words, and the Nudge page's copy had no phone rule.page-statusplus its ownfailureclass when the sections follow, whose scoped rule keeps the smaller space below it.TimePage.test.tschecks the new class names.github-widgetsnow has nine steps; this is the seventh.QA
main's HTML and every element's tag, box and computed styles for 7 scenarios: the PRs page not set up; the Time page not set up, failed, with a failure before any search succeeded and with a failure over its sections, both with long unbroken texts; the Nudge page failed; the Todos page failed. It ran in desktop Chromium and phone WebKit, light and dark: 28 captures onb7f499cand on this branch. With the class names mapped, the HTML is the same in all 28. The Time page's 16 captures differ in nothing else, the failure line's 30 px and 24 px included. The only other differences:overflow-wrapturnsanywhereon the PRs, Nudge and Todos lines, with no box moving, and on phones the Nudge line's padding goes from 64 px to 48 px, which shortensmainby 16 px.e2e/page-status.spec.tschecks each page's line is muted, with 64 px below it, or 48 px on phones. On the base it fails for the Nudge page on phones alone (64 px); with the class taken off the PRs page's line, it fails on both projects. The Home's spec shares its--ink-mutedprobe with it throughe2e/ink.ts. The Time page's existing "a long failure wraps on a narrow screen" now guards the sharedoverflow-wrap.TimePage.svelte's one uncovered branch is the compiler'sfailure ?? ''inside{#if failure}, as onmain.Review
No blocking findings. Non-blocking:
.failureoutranks the global class, as Svelte's scoped styles always do; the QA captures confirm 30 px and 24 px.Gates
mise run check, green locally withBAKHYARD_E2E_PORT=8280: Go fmt, gen, lint, actions and compose lint, tests, coverage, govulncheck; web install, Biome, svelte-check and tsc, Vitest with coverage, knip, pnpm audit, Playwright end-to-end tests in both browsers (394 passed, 8 skipped).