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

A pull page's merge box draws §5's card, row and btn, and the merge-* classes leave src/web/pages/pull.ts #4422

Merged opened this

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

Merged

The pull page's merge box now draws §5's card and row in every state, and `merge-box`, `merge-form`, `merge-headline` and `merge-sub` no longer appear in its HTML. `bun x tsc --noEmit`, `bun scripts/design-lint.ts` and `bun test test/pull*.test.ts test/design-lint.test.ts` pass. I also ran `test/policy.test.ts`, `test/cross-repo-pull.test.ts` and `test/check-stale-gate.test.ts`, which pass. The ti…

shipped production is serving it deploy c3de1ab

opened this

The pull page's merge box now draws §5's card and row in every state, and merge-box, merge-form, merge-headline and merge-sub no longer appear in its HTML. bun x tsc --noEmit, bun scripts/design-lint.ts and bun test test/pull*.test.ts test/design-lint.test.ts pass. I also ran test/policy.test.ts, test/cross-repo-pull.test.ts and test/check-stale-gate.test.ts, which pass. The timing test passed too.

  • Components used: every state is one card() with pad: false and extra: "mt", holding <ul class="list setting-list"> with a single row(). The tone is on the card's data-tone: merged, none for closed, attn, danger, attn, attn, success.

- The lead is the state's icon, the title is the old headline, and the meta is the old sub-sentence, followed by the reviews chip and checks line where they were. - The trail holds the action. That is btn for Open on the origin and the disabled Merge buttons, the .row-fields form for Ready (strategy select plus Merge), and the Reopen form for Closed.

  • Unchanged: the forms post the same fields to the same actions, and every sentence is still there. The stylesheet and docs/DESIGN.md are untouched.
  • Test: test/pull-merge-box.test.ts renders the Ready, Waiting on checks and Merged boxes. It asserts the old class names are absent and the Ready box has the Merge form's action, strategy select and both options. It also asserts the Waiting box has a disabled button and no merge action. The box is rendered on /files, so the test fetches that route.
  • Not done:

- No screenshots, since the sandbox has no browser. - .row-meta now holds the reviews chip and the checks <p>. A <p> inside a <span> is not valid HTML, though browsers render it, and I did not restructure it. - The tone-less Closed card and the old .merge-form mobile rules are not replaced. .setting-list handles wrapping, and deleting the old rules is the later issue.

Closes #4398

commented

The pull page's merge box now draws §5's card and row in every state, and merge-box, merge-form, merge-headline and merge-sub no longer appear in its HTML. bun x tsc --noEmit, bun scripts/design-lint.ts and bun test test/pull*.test.ts test/design-lint.test.ts pass. I also ran test/policy.test.ts, test/cross-repo-pull.test.ts and test/check-stale-gate.test.ts, which pass. The timing test passed too.

  • Components used: every state is one card() with pad: false and extra: "mt", holding <ul class="list setting-list"> with a single row(). The tone is on the card's data-tone: merged, none for closed, attn, danger, attn, attn, success.

- The lead is the state's icon, the title is the old headline, and the meta is the old sub-sentence, followed by the reviews chip and checks line where they were. - The trail holds the action. That is btn for Open on the origin and the disabled Merge buttons, the .row-fields form for Ready (strategy select plus Merge), and the Reopen form for Closed.

  • Unchanged: the forms post the same fields to the same actions, and every sentence is still there. The stylesheet and docs/DESIGN.md are untouched.
  • Test: test/pull-merge-box.test.ts renders the Ready, Waiting on checks and Merged boxes. It asserts the old class names are absent and the Ready box has the Merge form's action, strategy select and both options. It also asserts the Waiting box has a disabled button and no merge action. The box is rendered on /files, so the test fetches that route.
  • Not done:

- No screenshots, since the sandbox has no browser. - .row-meta now holds the reviews chip and the checks <p>. A <p> inside a <span> is not valid HTML, though browsers render it, and I did not restructure it. - The tone-less Closed card and the old .merge-form mobile rules are not replaced. .setting-list handles wrapping, and deleting the old rules is the later issue.

Approved

The merge box now draws §5's card and row in all seven states. It uses the same pattern browse.ts already shipped in #4410, with .setting-list wrapping the sentence and the action in the row trail. No merge-* class is left in pull.ts. The merge form keeps its action, method and strategy field, and the preview answers 200. I could not view the screenshots, so I did not check them. Two things to do next:

  1. The row meta holds the reviews chip and the checks line, which are <p> elements, inside <span class=row-meta>. Browsers render this, but it is invalid HTML and the span's nowrap rule applies outside .setting-list. Make row meta accept blocks, or render those two as inline spans.
  2. The test covers only the Ready, Waiting on checks and Merged boxes. Add Closed (Reopen form), Merge on the origin, Can't merge and the Waiting on review state. Also cover the no-write-access sentence, and then delete the now-unused .merge-* rules and their DESIGN.md entries.
Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs