Repository navigation
docs(List): a story draws each compact row's touch target - #9894
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:design-review
PR Analysis Report🔎 Preview ·
|
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested.
Compared with main, this pull request also carries the touch hit layer from #8085, so its diff is 2,470 authored lines. Blocking:
- The diff is 2,470 authored lines, 2,239 of them the separable feature from #8085, and neither body declares the stack: add one identical
Stack: #8085 → #9894line to both bodies, or rebase ontomainafter #8085 merges so only the story commit remains. - In
useClickableContainer.ts, the marking Effect now depends onmarksContainerbut never removesdata-pressable-containerwhen it turns false, so a row that stops being a delegate keeps the marker and a mouse click in its padding no longer reaches a clickable card around it; render the marker on Item's root for delegate rows instead and leave the public hook's Effect as it is onmain.
The story and the touch hit layer match the List touch-target requirement, and the unit tests of the changed files pass. A few added comments in Core narrate the code; trimming them is optional.
[Automated review]
| el.setAttribute(PRESSABLE_CONTAINER_ATTRIBUTE, 'true'); | ||
| } | ||
| }, [containerRef]); | ||
| }, [containerRef, marksContainer]); |
There was a problem hiding this comment.
When marksContainer turns false on the same element this Effect re-runs and does nothing, so data-pressable-container stays and a clickable container around the row keeps treating it as a control. Rendering the marker on Item's root when isDelegate lets React add and remove it; the public hook's Effect can then stay as it is on main.
There was a problem hiding this comment.
bb8c1af to
a03e703
Compare
There was a problem hiding this comment.
#8085: changes requested.
- Miss:
InfoTipkeeps its own watch and::afterhit layer (InfoTip/InfoTip.tsxlines 109-123 and 153-161) beside the shareduseTouchHitAreaandtouchHitAreaStyles.layerthis pull request adds, and the two already differ, so move InfoTip onto the shared hook and layer within this stack, either by adding #9916 to the stack declaration in every body or by folding its change into #8085.
#9894: approved on its own, pending CI. The story changes no Core code and draws exactly the layer #8085 adds.
Whole stack: not yet. A pull request lands only when it and every pull request below it are approved, so #9894 waits on #8085; CI has not run on this head (a draft from a fork), so a maintainer adds ci:run once it is ready for review.
[Automated review]
| * <li ref={hitAreaRef} {...stylex.props(isInteractive && touchHitAreaStyles.layer)} /> | ||
| * ``` | ||
| */ | ||
| export function useTouchHitArea(enabled: boolean): RefCallback<HTMLElement> { |
There was a problem hiding this comment.
Miss: InfoTip still runs its own copy of this hook and its layer: a callback ref over watchHitArea (InfoTip/InfoTip.tsx lines 153-161) and an ::after reading the same inset properties (lines 109-123). That copy neither clears its insets on stop nor waits for nearby motion, as this hook does. Moving InfoTip onto useTouchHitArea(true) and touchHitAreaStyles.layer within this stack (adding #9916 to the stack declaration, or folding it in here) leaves one path.
There was a problem hiding this comment.
#9916 moves InfoTip onto the shared hook and layer within this stack, and every body in the stack now declares Stack: #8085 → #9916, #9894.
| // Under a coarse primary pointer, a List row's whole touch target | ||
| // follows its link. That covers a tap in the row's padding, outside the | ||
| // anchor, and a tap in the band past its box. The click goes to the | ||
| // row's anchor with its modifier keys, so the link component and the | ||
| // browser pick the destination. The primary pointer decides, as it | ||
| // decides the band, not the pointer that made the click. With a fine | ||
| // primary pointer a click in the padding does nothing. A click with no | ||
| // pointer behind it does nothing either. |
There was a problem hiding this comment.
Optional (comments): these lines narrate the branch below. Keeping only the reason reads: "The primary pointer decides, as it decides the band. WebKit reports a finger's click as a mouse click."
| // Under a finger, an interactive row's hit target grows toward the touch | ||
| // target where its neighbors leave room. Nothing moves or repaints. | ||
| // useTouchHitArea keeps the row's insets live, and its invisible layer reads | ||
| // them. |
There was a problem hiding this comment.
Optional (comments): this restates the next three lines. Suggested: "Under a finger, the row's hit target grows where its neighbors leave room."
| // Under a coarse primary pointer, a host's hit target grows toward the touch | ||
| // target where its neighbors leave room. Nothing moves and nothing repaints. | ||
| // utils/hitArea.ts measures how far on each side. It stops at half the gap to | ||
| // the nearest neighboring target, and at 0 where they touch. | ||
| // | ||
| // The layer is the host's `::after`, grown past the host's edges by those | ||
| // insets. The insets are 0 until the first measurement, so first paint and | ||
| // server rendering never overlap a neighbor. The host is the layer's | ||
| // containing block, so a host that is not positioned sets `position: | ||
| // relative`. The insets start at the host's border box, but the layer's | ||
| // offsets start at its padding box. So a host that draws a border declares | ||
| // its width on that side (`--_hit-border-{top,right,bottom,left}`, 0 by | ||
| // default). A divided list row declares its divider's width. | ||
| // | ||
| // A clip path cuts the host's own box out of the layer. So the host's content | ||
| // and controls keep their taps, and a tap in the band goes to the host. The | ||
| // layer has no z-index of its own. A later sibling that is positioned with no | ||
| // z-index paints and hit-tests over it. Inside a List, which isolates its | ||
| // rows, the layer takes `z-index: 1`, so a later row cannot cover it. A host | ||
| // outside such a container raises nothing, so its band never passes a | ||
| // page-level sticky header. The host itself keeps no isolation and no | ||
| // z-index, so at rest it is no stacking context. A swipeable row is a | ||
| // stacking context, so it raises itself. | ||
| // | ||
| // Under a finger the host's `::after` is this layer, so decorate the host | ||
| // with a child. The insets are measured in the viewport, so the inline edges | ||
| // are physical: a left inset grows the left edge in either direction. StyleX | ||
| // evaluates property names at build time, so they are spelled out, and | ||
| // touchHitArea.test.tsx checks them against HIT_INSET_PROPERTIES. |
There was a problem hiding this comment.
Optional (comments): a 29-line block before one style. Keeping the constraints only: "The host must be positioned. Its insets start at the border box and the layer's offsets at the padding box, so a host with a border declares its width in --_hit-border-*. Under a finger the host's ::after is this layer."
There was a problem hiding this comment.
Left as is: the block states the layer's constraints in one place, beside the style that depends on them.
There was a problem hiding this comment.
Fair point — keeping the layer's constraints together beside the style that depends on them is a reasonable reason to leave the block as is. The suggestion was optional, and the next full review will re-evaluate it.
a03e703 to
039925e
Compare
039925e to
42d2a63
Compare
There was a problem hiding this comment.
#8085: approved, pending CI. Under a finger, interactive List rows now reach toward a 44 px target through an invisible band that stops halfway to a neighbour, as the List touch-target requirement asks, and InfoTip now uses the same hook and layer, so the shared-path blocker from the last round is fixed. No blocking findings.
- Optional: the size report has per-component lines but none for the package stylesheet itself; one line for
astryx.css(base against head, gzip) completes it.
#9894: approved, pending CI. The story changes no package code and draws exactly the layer #8085 adds; its readout names the tapped row, or "none".
Whole stack: passes. A pull request lands only when it and every pull request below it are approved. CI has not run on either head yet: both are drafts from a fork, so once they are marked ready for review a maintainer adds the ci:run label so CI runs.
[Automated review]
The Compact touch targets story draws each row's touch target under a finger as a striped band past the row's box, read from the row's own hit layer. A readout names the row a tap fires, or "none". Story only: no product change.
42d2a63 to
971e97c
Compare
github-actions
Bot
removed
the
needs:design-review
There was a problem hiding this comment.
Approved pending CI. The new List › Compact Touch Targets story changes no package code and draws exactly the touch band interactive List rows already have: under touch in Chromium the bands measure 5.5, 4, 4 and 6 px, matching each row's hit insets, and the readout names the tapped row or "none". No blocking findings.
- Optional: Testing step 3 expects "Row 2" and "Row 3" for taps in the gap between the lists, but in Chromium both halves read "none", as your Evidence section explains; naming the engine in that step keeps the test plan reproducible.
CI has not run on this head yet: it is a draft from a fork, so once it is marked ready for review a maintainer adds the ci:run label so CI runs.
[Automated review]
cixzhang
added
the
ci:run
Reader impact
Under a finger, a List row's touch target reaches past the row's box where nothing is near (#8085), but nothing on screen shows how far. Someone checking that behavior on a phone can't tell whether a tap landed inside the target or past it. This story draws each compact row's target and names the row a tap fires.
Source of truth
The rows' own touch hit layer: each interactive row's
::after, grown by the insets the shared hit area measures (utils/touchHitArea.stylex.ts,utils/hitArea.ts). The story reads the layer as the browser lays it out and hard-codes nothing.Correction
A new Storybook story, List › Compact Touch Targets: two compact lists, Row 1–2 and Row 3–4, 8 px apart. Each row's touch target past its box is a striped band, with its reach in px under the lists. "Last tapped:" names the row a tap fires, or "none". A tap that fires no row reads "none", including one outside the lists and one the shared hit area drops. With a mouse there are no bands, and the story says so.
Evidence
On a phone-sized touch page, in Chromium and WebKit, the bands equal the rows' hit insets: Row 1 5.5 px above, Row 2 4 px below, Row 3 4 px above, Row 4 6 px below. Each band's center hit-tests to its own row. In WebKit a tap in each band fires its row. In Chromium, the browser's touch adjustment can move a small tap in the 8 px gap onto the other list's row. The shared hit area then drops it, so it fires nothing (never the wrong row) and the readout says "none". A finger-sized touch fires its row.
Scope
Testing
What was run at this head (on main 7b71d71): review:pre and its checklist, check:repo, changeset coverage (nothing ships), strict lint (0 errors), Prettier, the root and Storybook builds, Storybook's type check (0 errors), and the accessibility audit scoped to this story (no new violations). test:ui (832 files, 20,820 tests) and test:node (913 files) pass with no unhandled errors. In Chromium and WebKit under a phone profile, the story draws four painted bands (5.5, 4, 4 and 6 px); a tap on Row 2 reads "Row 2", a tap on the band above Row 1 reads "Row 1", and a tap far below the lists reads "none".