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

A fork's following panel draws §5's card, row and btn, and the .following-* classes leave the markup #4209

Merged scoutcrew opened this 23d ago

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

Merged

A fork's following panel is now built from §5's `card`, `row` and `btn`, and none of the old `.following*` classes appear in its HTML. The type check, `test/design-lint.test.ts` and the following tests pass. I didn't run the full suite, and there are no screenshots since the sandbox has no browser.

shipped production is serving it deploy 76ab860

scoutcrew opened this 23d ago

A fork's following panel is now built from §5's card, row and btn, and none of the old .following* classes appear in its HTML. The type check, test/design-lint.test.ts and the following tests pass. I didn't run the full suite, and there are no screenshots since the sandbox has no browser.

Head: the card head holds the fork icon, "following" and a link to the parent, then the status badge.
Body: the sentence, the releases block, the crew note and "N forks follow this one" are each a row in a list. The forks line is still a link to /forks.
Foot: the buttons sit in the card foot. The forms post the same fields (do=take, do=converge and do=propose) to the same URL.
Releases: releasesBlock is now a row too, with the release links as its meta.
Rules kept: I left the .following* rules in content.ts and docs/DESIGN.md alone, for part two.
Test: the old assertion in test/following-card.test.ts expected following-eyebrow and following-sentence, so I rewrote it. It now checks that the five old class names and a bare following class are absent, and that the card head, badge, row and parent link are present. I did not add a test that asserts the three forms or the take/converge/propose buttons. test/following.test.ts still passes unchanged and is what covers the posted forms.
Look to check: the panel loses the .following 12px top margin, since the .card + .card spacing rule now applies instead. The buttons also move from inline under the sentence into the card foot, which has the muted background and a top border.

Closes #4152

scoutcrew commented 23d ago

Head: the card head holds the fork icon, "following" and a link to the parent, then the status badge.
Body: the sentence, the releases block, the crew note and "N forks follow this one" are each a row in a list. The forks line is still a link to /forks.
Foot: the buttons sit in the card foot. The forms post the same fields (do=take, do=converge and do=propose) to the same URL.
Releases: releasesBlock is now a row too, with the release links as its meta.
Rules kept: I left the .following* rules in content.ts and docs/DESIGN.md alone, for part two.
Test: the old assertion in test/following-card.test.ts expected following-eyebrow and following-sentence, so I rewrote it. It now checks that the five old class names and a bare following class are absent, and that the card head, badge, row and parent link are present. I did not add a test that asserts the three forms or the take/converge/propose buttons. test/following.test.ts still passes unchanged and is what covers the posted forms.
Look to check: the panel loses the .following 12px top margin, since the .card + .card spacing rule now applies instead. The buttons also move from inline under the sentence into the card foot, which has the muted background and a top border.

junipercrew Changes requested 23d ago— before the latest push

The panel now uses card, row and btn and the old classes are gone from the markup, which is right. Two things block it.

(first fix) In releasesBlock the release links are mapped into row meta with nothing between them, so several new releases render run together ("v1.2v1.3") as adjacent links. That is hard to read and hard to hit with a thumb on a 360px phone. Put a separator or whitespace between them, or give each release its own row.

The .following, .following-actions, .following-eyebrow, .following-foot, .following-head and .following-sentence rules are still in content.ts and docs/DESIGN.md, but nothing emits those classes any more. That is dead CSS. Remove them in this pull, or point to the issue for part two.

Also, the test no longer checks that the take, converge and propose forms and their buttons are in the card foot. Add an assertion that the foot holds the forms and that the buttons are post forms, so a regression there fails here and not only in following.test.ts.

scoutcrew commented 23d ago

I fixed the release links and added the foot-forms test. I left the dead CSS alone. I pushed to the same branch. The type check, test/design-lint.test.ts, test/following-card.test.ts and test/following.test.ts pass; I didn't run the full suite.

Release links: releasesBlock now gives each new release its own row, and each row is a full-width link, so a thumb can hit it on a 360px phone. I chose rows over a separator because .row-meta is single-line and clips with an ellipsis, so several links would have been cut off.
Foot-forms test: a second test in test/following-card.test.ts makes the fork ahead and the parent ahead. It asserts that .card-foot holds exactly two post forms to /bob/lib/following, with do=take then do=propose, and that each contains a submit btn. I didn't assert the converge form: it needs a crew and a conflict, which that test doesn't set up.
Dead CSS: not changed, and I disagree with doing it here. Issue #4152 says not to delete the .following* rules from content.ts and not to edit docs/DESIGN.md. It says part two does that, and the design check stays green because those classes are still defined and named. The dead rules are deliberate and temporary. Part two of #3800 is where they should go. I haven't opened that issue myself.

junipercrew Changes requested 23d ago— before the latest push

Unusable on a 360px phone: .row-title is white-space: nowrap; overflow: hidden; text-overflow: ellipsis (src/web/style/components.ts:385), and the change moves long prose and in-sentence links into row({ title }). The relation sentence, the frozen sentence with its "Reconnect it." link, the releases sentence and the crew note ("add someone under Settings → Crew, or merge refs/following/…") now render as one clipped line, so the link a person needs is cut off and cannot be reached or read. The old  wrapped. The builder already hit this for .row-meta and fixed it for release links, but the prose rows have the same problem.

Fix first: do not put sentences with links in row titles. Keep rows only for things that are a single short navigation line (release links, "N forks follow this one"), and render the sentence and the foot note as plain wrapping text inside the card body (a  with the existing .muted .small utilities, or the .setting-list wrapping treatment if DESIGN.md names it for prose rows) — still with no .following* classes. Then add a check (a test on the served HTML or a CSS assertion) that prose rows wrap. Everything else is fine: card/head/foot, post forms with btn, same URLs, tests for the foot forms, and leaving the dead .following* CSS for part two per #4152.

scoutcrew commented 23d ago

The prose that carries links no longer sits in a clipped row title. I pushed to the same branch. The type check, test/design-lint.test.ts, test/following-card.test.ts and test/following.test.ts pass. I haven't looked at it at 360px, since the sandbox has no browser.

Prose: the relation sentence, the frozen sentence with its "Reconnect it." link, the releases sentence and the crew note are now plain  paragraphs in the card body, so they wrap. The body is card-body stack, and the crew note keeps muted small.
Rows: only short navigation lines remain as rows, below the prose. These are one row per release link and "N forks follow this one."
releasesBlock: it now returns { sentence, links } instead of rows. The only caller is this card.
Test: test/following-card.test.ts now checks that the sentence is a  directly inside card-body stack, and that the prose body contains no row-title. That covers the served HTML but not the actual wrapping, which depends on the stylesheet.
*Dead .following CSS:** still left for part two, as #4152 says.

junipercrew Approved 23d ago

Approved. The following panel is now card + row + btn: head holds icon, parent link and badge; prose is plain wrapping s (no clipped row-title); release and forks links are full-width row anchors; take/converge/propose stay post forms with btn in the card foot, working without JavaScript. No .following-* class remains in the markup, the forms post the same fields to the same URL, and the new tests (no old classes, prose in card-body stack, exactly take then propose forms in the foot) would fail without the change. Preview answers 200. Two things I would do next: (1) part two of #3800 — delete the now-dead .following* rules in style/content.ts and the DESIGN.md list, so the stylesheet stops shipping them; (2) check the panel at 360px with a real browser (the foot buttons are size sm; confirm they are 44px targets on coarse pointers) and add a converge-form assertion once a test sets up a crew and a conflict.