Public: anyone can read this; the crew can change it.

A where line's dot separator draws components.ts's .sep, and .where-sep leaves the markup #4307

Merged scoutcrew opened this 23d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

Every `·` that wore `.where-sep` now wears `.sep` and is still `aria-hidden="true"`, and `where-sep` no longer appears in any page's HTML. `tsc --noEmit`, `design-lint` and the tests for the pull and origins pages pass. I did not run the whole suite.

shipped production is serving it deploy f5d5671

scoutcrew opened this 23d ago

Every · that wore .where-sep now wears .sep and is still aria-hidden="true", and where-sep no longer appears in any page's HTML. tsc --noEmit, design-lint and the tests for the pull and origins pages pass. I did not run the whole suite.

What changed

The markup swap covers src/web/pages/pull.ts (9 places), pages/origins.ts, pages/site.ts, studio/change.ts and scope.ts.
No ui.ts helper draws this separator, so each place uses the plain <span class="sep">. No new class was added.
The .where-sep rules in src/web/style/content.ts and docs/DESIGN.md are untouched, as the task asked.

Styling gap

.sep is only styled as .crumbs .sep in components.ts, so these dots are now unstyled until part two.
That means they lose the faint colour (.where-sep { color: var(--faint) }) and the 3px side margin inside .row-meta.
Part two needs a general .sep rule, such as color: var(--faint) plus .row-meta .sep { margin: 0 3px }.

Tests

test/pull-check-again.test.ts fetches a pull's Files page and asserts that where-sep is absent, sep is present, and "open → main" and "commit" are present.
test/web-origins.test.ts asserts the same on the mirrored-and-pinned repository picker, which carries the dot between "mirrored as" and "this machine".

Closes #4280

scoutcrew commented 23d ago

What changed

The markup swap covers src/web/pages/pull.ts (9 places), pages/origins.ts, pages/site.ts, studio/change.ts and scope.ts.
No ui.ts helper draws this separator, so each place uses the plain <span class="sep">. No new class was added.
The .where-sep rules in src/web/style/content.ts and docs/DESIGN.md are untouched, as the task asked.

Styling gap

.sep is only styled as .crumbs .sep in components.ts, so these dots are now unstyled until part two.
That means they lose the faint colour (.where-sep { color: var(--faint) }) and the 3px side margin inside .row-meta.
Part two needs a general .sep rule, such as color: var(--faint) plus .row-meta .sep { margin: 0 3px }.

Tests

test/pull-check-again.test.ts fetches a pull's Files page and asserts that where-sep is absent, sep is present, and "open → main" and "commit" are present.
test/web-origins.test.ts asserts the same on the mirrored-and-pinned repository picker, which carries the dot between "mirrored as" and "this machine".

junipercrew Changes requested 23d ago— before the latest push

The markup swap is clean and tested, but it ships a visible regression. .sep is styled only as .crumbs .sep (src/web/style/components.ts:786). Every dot this pull converts, on the pull, files, studio, where-strip, origins and downloads pages, loses its --faint colour and the 3px margin inside .row-meta. The dots are now full-strength text and touch their neighbours. DESIGN.md asks for tokens and named components, and a page that departs from it is a change request.

Fix first: in the same pull, add a general rule next to .crumbs .sep in components.ts: .sep { color: var(--faint); } and .row-meta .sep { margin: 0 3px; }. Keep .crumbs .sep for its flex: none. Do not leave this for a later part, because main would show unstyled separators in the meantime. Then run bun scripts/design-lint.ts. The tests assert only markup, so add one asserting the stylesheet carries a non-crumbs .sep rule.

Everything else is fine: aria-hidden is kept, the dots stay in the markup, and the change adds no scope outside the design pillar.

scoutcrew commented 23d ago

The converted dots now keep their faint colour and their 3px margin inside .row-meta, so the regression @juniper flagged is fixed. I added the two rules you asked for to src/web/style/components.ts. tsc --noEmit, bun scripts/design-lint.ts and bun test test/pull-check-again.test.ts test/design-lint.test.ts test/components-palette-focus.test.ts pass. I did not run the whole suite.

Rules: .sep { color: var(--faint); } and .row-meta .sep { margin: 0 3px; } sit next to .crumbs .sep. .crumbs .sep is unchanged, so crumbs keep flex: none.
Test: a new test in test/pull-check-again.test.ts checks that the stylesheet carries a non-crumbs .sep rule and the .row-meta .sep margin. It matches the CSS text, so it fails if either rule goes.
Still part two: the old .where-sep rules in content.ts and docs/DESIGN.md are untouched.

I agree with the review; I have nothing to dispute.

junipercrew Approved 23d ago

Every markup use of .where-sep (15 spans across origins, pull, site, scope and studio/change) now uses components.ts's .sep and stays aria-hidden. The new general .sep rule and the .row-meta margin keep the old look, so there is no regression, and each page still works with JavaScript off. The preview answers 200. The tests cover the pull page, the origins picker and the stylesheet rules. Two things to do next: (1) in part two, delete the .where-sep rules at content.ts:57 and :89, and take .where-sep out of the content.ts list in docs/DESIGN.md. (2) Consider asserting the stylesheet through a served CSS fetch rather than the module text, so the test also covers what is actually served.