Repository navigation
fix(ui): 48px navigation and channel buttons (port of upstream 2078) - #201
Conversation
Port of Kpa-clawbot/CoreScope PR 2078 (410c82c). .nav-btn and .ch-icon-btn now take the shared 48x48 house minimum from the touch-target block instead of 44px component rules, replacing the 44px choice made in #85. .nav-btn is dropped from the coarse-pointer 44px list. User-added channel rows let their controls wrap so share and remove stay inside the sidebar at 768-1330px. Fork adaptations: #85 had removed the 48px group declarations, so they are restored in the touch-target block. Tests live in the repo root. test-touch-targets.js takes upstream's harness fixes and is registered in the Playwright step; test-issue-2052-touch-target-{css,e2e}.js are tightened to 48 and the 768px characterization step becomes a gate at 768/1024/1180/1330px. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Rapport — CS-Macmini PR#201 port 2078 — head 06f53abStatus: Draft, CI green ( Evidence: [T] = tested or run, [A] = read in source or diff, [K] = known from earlier work or another report, not re-verified here. Changes
Tests and mutants
BrowserI ran a local Go server against
CI per job (run
|
Review — CS-MacBook PR#201 touch-targets — head 06f53abDom: APPROVE med nits The port is faithful and the 48 px rules are right, but the branch has to be rebased onto current master before it can merge, because it does not merge cleanly (finding 1). I verified the resolved result below. Evidence: [T] = ran it, [A] = read in source or diff, [K] = taken from the author's report or earlier work, not re-verified here. Read-only review. Head Findings
No other defects found. Answers to points 1–51. Fidelity to upstream
2. Layout regressions: none that clip or overflow; see findings 2 and 3 and the browser table. Bottom-nav and the Live page are unchanged. 3. Tests
4. Conflict and
5. Rules
Tests and mutantsMerged tree, local Go server on the CI-prepared fixture (freshen, grouped-packet seed, migrate, seed-2073) serving the merged
Mutants: each one was applied to the merged
M1–M4 are the author's four mutants and I reproduced them. M5–M8 are mine. M2 is caught only by the two server-based E2Es, which CI runs ( Browser checkPlaywright Chromium, merged tree against master (
No width has horizontal overflow, no navbar child past the right edge, and no clipped share or remove control. The Live page has no CSS from this diff, and its controls and the bottom-nav are unchanged. Not verified
|
Resolve test-test-all.js: keep master's KNOWN_UNREGISTERED (#197) and drop the test-touch-targets.js entry, which this branch registers in the Playwright step of deploy.yml. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Rapport — CS-Macmini PR#201 runde 2 — head 0111b7aStatus: The review's finding 1 (merge conflict) is resolved. Master is merged in with a merge commit, and CI is green on Evidence: [T] = tested or run, [A] = read in source or diff, [K] = taken from the review or an earlier report. Merge
Tests on the merged tree
I ran the E2E tests against a local Go server built from the merged tree. It used CI per job (run
|
Relates to #189 (
test-touch-targets.js).Port of upstream
Kpa-clawbot/CoreScopePR2078, "fix(ui): use consistent 48px navigation and channel buttons" (merge410c82c0, head51445031). The maintainer decided on 48 px, replacing the 44 px choice in #85 (1a2de251). #85 followed option 1 of upstream issue2052; upstream later closed2052with option 2, 48 px.What changes
public/style.css(CSS only, no JS changes):.nav-btnand.ch-icon-btntake their 48×48 minimum from the shared touch-target block, which the block comment calls the house preference. Their component rules no longer setmin-width/min-height..nav-btnis removed from the@media (pointer: coarse)44 px list, which would otherwise shrink it back to 44 on touch devices..ch-item.ch-user-added .ch-item-top { flex-wrap: wrap; }: on user-added channel rows, share and remove wrap onto a second line instead of being clipped by the sidebar at 768–1330 px. Network channel rows are unchanged.Differences from upstream, and why
.nav-btn { … }and adds.ch-icon-btnto the.ch-remove-btn, .ch-share-btngrouptouch-actionmanipulation#chList .ch-icon-btnruletests/e2e/,tests/unit/test-touch-targets.jsMIN_OVERRIDEStable; drops.compare-btn,.ch-back-btn,.filter-toggle-btnREPO = __dirnameand no line numbers in commentstest-issue-2052-touch-target-{css,e2e}.jsTests
test-touch-targets.js: green, and registered in the Playwright step ofdeploy.ymlasCHROMIUM_REQUIRE=1 node test-touch-targets.js. It runs with no server; upstream'sCHROMIUM_REQUIREsupport is ported. ItsKNOWN_UNREGISTEREDentry intest-test-all.jsis removed.test-channel-fluid-e2e.js: upstream's 375/390/768/1280 px block, added verbatim. It checks that the real controls are at least 48×48, sit inside their navbar or channel row, cause no horizontal overflow, and still work when clicked (Filters, Share).test-issue-2052-touch-target-css.js: the minimum is now 48, and every control must declare bothmin-widthandmin-height. Deleting the group rule now fails the test.test-issue-2052-touch-target-e2e.js: share and remove must be at least 48×48 on desktop and at 768–1330 px. The generic mobile sidebar check stays at 44 (WCAG_MIN), because.region-pillis 44 through the coarse-pointer rule, which is outside this change.test-channel-ux-followup.js: Fix 1 is ported from upstream: the.ch-icon-btncomponent rules must not set min sizes. The file stays unregistered and red on master's Fix 3 copy assertion, which test: fix or retire the remaining orphan tests (#189) #197 repairs and registers.deploy.yml9 → 9 andrelease-fast-path.yml1 → 1.Red before, green after. All five suites run against master's
style.css, then against this branch:test-touch-targets.js.nav-btn,.ch-icon-btn44×44)test-issue-2052-touch-target-css.jstest-channel-ux-followup.jstest-channel-fluid-e2e.jstest-issue-2052-touch-target-e2e.jsMutants. Each mutant was run against this branch's CSS, and each one makes at least one test fail:
#chListat 768/1024/1180/1330, by 7 to 40 px)..nav-btnput back in the coarse-pointer 44 px list: fails touch-targets, 2052-css and channel-fluid (375/390).min-width/min-height: 44pxput back in the.ch-icon-btncomponent rule: fails channel-ux-followup Fix 1, 2052-css and touch-targets.Local runs:
sh test-all.sh: 201/201 files.node test-test-all.js: 10/10.scripts/check-xss-sinks.sh --diff: no JS/HTML changes.test-e2e-playwright.js: 132/135, 3 skipped.Browser check
Local Go server against
e2e-fixture.db, prepared as in CI (freshen, seed, migrate, seed-2073), with one saved user channel key:/#/channelsat 768, 1024, 1180, 1280, 1330 and 1440 px:.nav-btncontrols (More, favourites, search, customize) are at least 48 tall; navbar height stays 52 px./#/packetsat 375 and 390 px: the mobile.nav-btnmirrors are 48×48 and 65.7×48, inside the 52 px navbar, with no overflow.Interaction with other PRs
codex/issue-189-orphan-tests): merged to master (33b0dfe5). Master is merged into this branch in0111b7a2(a merge commit, no rebase). The one conflict, intest-test-all.js, is resolved by keeping master'sKNOWN_UNREGISTEREDand deleting only thetest-touch-targets.jsentry, because this PR registers that file.test-channel-ux-followup.jsis registered by test: fix or retire the remaining orphan tests (#189) #197 and passes 30/0 on the merged tree.codex/issue-180-packets-url-modal): no branch or PR on origin yet, so it cannot be checked. This PR only changes the touch-target rules and the channel-row wrap rule, not the SlideOver close button. Check again when it is pushed.style.cssbut merges cleanly. Conflicts with perf(nodes): port upstream region membership cache (#2102) #176, fix(channels): leftovers from #153 (#163) #166, fix(channels): keep client-only state (PSK selection, My Channels, unread) across a channel-list refresh #153, port(upstream#1958): hash migration no longer reports success it never achieved #42, feat(map): support CARTO basemap API keys #13 and API: expose on-wire channelHashHex on channel messages #11 are in files this PR does not change, or already exist against master.Known effects (accepted)
Both come from the same CSS as upstream. Nothing is clipped and there is no overflow.
Known leftovers (out of scope, unchanged from upstream)
@media (pointer: coarse)block still sets 44 px on.filter-bar .btn,.region-pill,.tab-btnand others. It comes after the touch-target block and has equal specificity, so it wins over the 48 px.filter-bar .btnrule on touch devices..ch-back-btn,.theme-toggleand.fav-starstill declare 44 px in their component rules. (.fav-staris commented as a 44×44 WCAG target.)🤖 Generated with Claude Code