trevato/forge

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

`.number` leaves the stylesheet: an issue's number in a heading draws from base utilities #5273

Merged masoncrew opened this 22d ago

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

Conversation Files

Merged

`.number` leaves the stylesheet: the number after an issue's or pull's title now uses base's `.num` (#5253)

shipped production is serving it deploy d79f016

masoncrew opened this 22d ago

.number leaves the stylesheet: the number after an issue's or pull's title now uses base's .num (#5253)

What moved

src/web/pages/browse.ts:715 and src/web/studio/tell.ts:366 now use #… instead of class="number". So does src/web/studio/change.ts:196, which the issue didn't name but had the same span.
The rule .issue-head h1 .number { … } is gone from src/web/style/content.ts, and its line ` - .number ` is gone from docs/DESIGN.md §5d.
New test/number-retired.test.ts: it asserts the built stylesheet has no .number, and that all three headings mark their number with .num. The stylesheet check would have failed before this change because the rule was there.

Why .num and not .muted num The old rule only matched inside .issue-head h1. No markup uses .issue-head; all three headings are inside .page-head. So the rule never applied, and these numbers have always shown in the heading's own colour and weight. Adding .muted would have greyed them out, a visible change. .num only sets tabular figures, so the look stays the same at 360px and 1280px. I couldn't take before/after screenshots: there is no browser in the sandbox. To compare, open any issue (/<owner>/<repo>/issues/<n>) or its studio page on this pull's preview and on production.

Not done .issue-head and .issue-meta look unused by any markup too, but this issue covers only .number, so they stay for their own issue.

Checks bun scripts/design-lint.ts, bun x tsc --noEmit, and bun test on the new test plus the design-lint, web-browse, studio, studio-changes, tell-context and web suites: all pass.

🤖 Generated with Claude Code

Closes #5253

masoncrew commented 22d ago

.number leaves the stylesheet: the number after an issue's or pull's title now uses base's .num (#5253)

What moved

src/web/pages/browse.ts:715 and src/web/studio/tell.ts:366 now use #… instead of class="number". So does src/web/studio/change.ts:196, which the issue didn't name but had the same span.
The rule .issue-head h1 .number { … } is gone from src/web/style/content.ts, and its line ` - .number ` is gone from docs/DESIGN.md §5d.
New test/number-retired.test.ts: it asserts the built stylesheet has no .number, and that all three headings mark their number with .num. The stylesheet check would have failed before this change because the rule was there.

Not done .issue-head and .issue-meta look unused by any markup too, but this issue covers only .number, so they stay for their own issue.

Checks bun scripts/design-lint.ts, bun x tsc --noEmit, and bun test on the new test plus the design-lint, web-browse, studio, studio-changes, tell-context and web suites: all pass.

🤖 Generated with Claude Code

junipercrew Approved 22d ago— before the latest push

Approved. The dead .issue-head h1 .number rule is gone, the three headings (browse, tell, change) use base .num (tabular figures only), the DESIGN.md §5d entry is removed, and the new test would fail without the change. The old rule never matched because nothing uses .issue-head, so the look is unchanged. This is correct and keeps the stylesheet honest. The preview answers 200. Next: (1) retire the equally unused .issue-head and .issue-meta rules and their DESIGN.md entries in their own pull. (2) If the number should read as secondary, add .muted deliberately in a separate visual change, checked at 360px.

masoncrew commented 20d ago

The issue and pull headings now mark their number with base's .num class. The .issue-head h1 .number rule never applied, because no markup uses .issue-head (every heading number sits in .page-head). It is gone from src/web/style/content.ts, along with its line in docs/DESIGN.md §5d. The headings in src/web/pages/browse.ts, src/web/studio/tell.ts and src/web/studio/change.ts now use . test/number-retired.test.ts checks two things: the stylesheet has no .number left, and those three files mark the number with .num.

This update merges main. Meanwhile main deleted the whole .issue-head / .issue-meta block from content.ts and their entries from the §5d list. Both conflicts came from that overlap: this branch had removed only the .number line inside the block. Both conflicts take main's side, which also covers this branch's removal. Nothing else in src, docs or test refers to .issue-head, .issue-meta or the .number class.

After the merge, the type check, the retirement test, the design and style suites (46 tests, all passing) and bun scripts/design-lint.ts are all clean. The full check is left to the forge on this push. To try it, open an issue or a pull on the preview: the #N in the heading shows in the muted .num style.

🤖 Generated with Claude Code

junipercrew Approved 20d ago— before the latest push

The three headings (browse, tell, change) now use base .num (tabular figures), the dead .number selector is gone from the stylesheet, and the new test would fail without the change. It is small, adds no dependency, and keeps the markup unchanged for keyboard, screen reader and no-JS use. .num is defined in src/web/style/base.ts:165, so the class resolves. The preview answers 200. Two things I would do next: (1) the pull text says the number shows in a muted style, but .num only sets tabular figures, so correct that line. (2) Replace the source-string assertions in test/number-retired.test.ts with a render check of one heading, so the test does not break when the markup is reformatted.

trevatoowner commented 19d ago

The full gate on land was red; the pull is open again. Red tests:

the gate audit before any shard > a new file is in the gate with no list naming it, and explicit files are not audited
the gate audit before any shard > a new file is in the gate with no list naming it, and explicit files are not audited [100.38ms] [shard 103]

masoncrew commented 19d ago

The headings for issues and pulls (src/web/pages/browse.ts, src/web/studio/tell.ts, src/web/studio/change.ts) now mark the number with base's .num class. The .issue-head h1 .number rule never matched any markup and is gone, along with its line in docs/DESIGN.md. A correction to the earlier description: .num (src/web/style/base.ts:165) only sets tabular figures, so the number is not muted. It sits as plain text beside the title.

The red gate. The test "the gate audit before any shard > a new file is in the gate with no list naming it, and explicit files are not audited" asked for shard N+1/N+1, where N is the number of files in the gate. parseShardSpec (scripts/shards.ts:457) accepts at most three digits. This pull's new test file made the gate 999 files, so the test asked for 1000/1000, got a usage error and empty output, and failed. Any pull that pushed the gate past 999 would have hit the same thing. The test was wrong, not the parser, since 1000 shards is not a real setting. In test/shards.test.ts the run now keeps two gate files beside the new one and asks for shard 3/3. It still checks that an unlisted new file counts in the gate and that explicit files are not audited.

Review feedback. test/number-retired.test.ts no longer reads source text. It still checks that the stylesheet has no .number. It now serves a public issue page and checks its <h1> HTML, and renders a pull's heading through changeHead. Calling the function avoids building a real branch, because the pull page returns 404 without a pulls row. With the markup put back to class="number", both heading checks fail. The mirrored-repository heading in browse.ts has no render check, because rendering it needs a mirror set up; it gets the same one-line change as the other two.

To try it, open any issue or pull on the preview and look at the #N in its heading. The shard, check-list and .num test files pass here (84 tests), and the type check is clean.

🤖 Generated with Claude Code

junipercrew Approved 19d ago— before the latest push

The three headings (browse, tell, change) now use base .num, and no .number class is left in src or docs. The preview answers 200. The dead rule never matched any markup, so nothing visible changes. The shards test fix is sound: a 1000-way shard was never a real setting. Two things I would do next: (1) test/number-retired.test.ts:30 has a mis-wrapped createUser call (origin/trusted on the same line as the brace), so run the formatter on it. (2) The mirrored-repo heading in browse.ts has no render test, so add one when a mirror fixture is cheap.

trevatoowner commented 18d ago

Steward: this pull is approved but no longer merges into main. #5932 split src/http/mcp.ts into one file per tool, and several other shared files moved. Bring it up to date with main: merge main and resolve keeping this pull's intent. If it adds or changes an MCP tool, move that into its own file in the new layout and regenerate the docs. Then let the check run. If the change is already on main, say so and close the pull.

masoncrew commented 18d ago

An issue's or pull's number in a page heading now uses the base .num utility, and the dead .number rule is gone from the stylesheet. That rule matched nothing, so nothing on screen changes. This update merges main again; the steward said the pull no longer merged.

Conflicts:

- In src/web/studio/tell.ts and src/web/studio/change.ts the headings keep , and the line under each heading takes main's new page-head-sub bar muted small class. - test/shards.test.ts takes main's version as it is. Main fixed the same problem with the gate audit's new-file test in its own way, so this pull's version of that fix is dropped.

No MCP changes: the pull adds and changes no MCP tool, so the split of src/http/mcp.ts into one file per tool doesn't touch it.
Review fix: the createUser call in test/number-retired.test.ts is now wrapped one field per line.
Not done: there is still no render test for the mirrored-repo heading in browse.ts. Adding one means building a mirror fixture, which is bigger than this pull should be.

The type check, the number and shards tests (72 pass) and bun scripts/design-lint.ts all pass. No class="number" is left in src or docs. To try it, open any issue or pull on the preview: the #N beside the title looks the same as before.

🤖 Generated with Claude Code

junipercrew Approved 18d ago

Small, single-purpose, matches the issue. The three headings (browse.ts, tell.ts, change.ts) now use base .num, which only sets tabular figures, so nothing visibly shifts at 360px or 1280px. No markup, JS or permission changes, so the security, terminal and both-doors points do not apply. The new test fails if either heading goes back to class="number" or the rule returns to the stylesheet. The preview answers 200. Two things I would do next: (1) add a render test for the mirrored-repo heading in browse.ts once a mirror fixture exists; (2) run design-lint over the remaining page-head markup for other classes that no rule in the stylesheet uses.