feat(thumbnail): add showRemoveOn prop#4300
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR Analysis Report📚 Storybook PreviewView Storybook for this PR 🧪 Sandbox PreviewView Sandbox for this PR Modified ComponentsThumbnail · View in Storybook
Bundle Size Summary
Accessibility AuditStatus: 1 accessibility violation(s) found — 1 serious. Thumbnail - 1 issue(s)
Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
dcfa9ac to
98171d2
Compare
cixzhang
left a comment
There was a problem hiding this comment.
Thanks, Kenton — dropping APCA image-sampling for a fixed treatment is the right direction, and reusing Overlay's showOn vocabulary for showRemoveOn keeps the API coherent. The ::after tint matching ClickableCard (the currentColor derivation + @media (hover: hover) guard) is spot on, and the z-index layering checks out. Three blocking items before this lands.
🔴 1. showRemoveOn="hover" breaks the remove path on touch (accessibility). The reveal fires only on :hover (hover-capable), :focus-within, or @media (hover: none) and (pointer: coarse). Keyboard, screen reader, mouse, and phone/tablet are all fine. But a hybrid device (touchscreen laptop, Surface — reports hover: hover + pointer: fine) matches none of the three, so touch gets no reveal:
- Interactive thumbnails degrade (tap focuses the inner button →
:focus-withinreveals remove — removal needs a second aimed tap). - Non-interactive thumbnails (
onRemove, noonClick) are the sharp edge: the remove button is the only focusable element,opacity: 0but still mounted and clickable. A touch user must tap an invisible destructive control — and that same tap both reveals and firesonRemove. Silent delete with no visual affordance.
Since touch is an operable/accessibility modality, this is blocking. A clean signal is @media (any-pointer: coarse) — if the device can be touched at all (including hybrid laptops with both a mouse and a touchscreen), just keep the button visible rather than gating it behind hover. Alternatively, treat "no onClick" as always-visible. Your call on the exact approach.
🔴 2. Remove-button backing — use --color-overlay. backgroundColor: '#00000066' is raw hex on the button's xstyle (top of the cascade → un-themeable), and the eslint-disable documents the bypass rather than fixing it. The semantic scrim token is --color-overlay, already used for this file's uploadOverlay — a one-token swap that keeps the darkening intent and stays themeable. (The --color-on-dark X and the secondary-variant hover/press overlay are already tokenized — no change there.)
🔴 3. Inset hairline — --color-border (or derive from a token). light-dark(#00000010, #FFFFFF40) is raw hex inside light-dark(), slipping past no-hardcoded-styles via the const indirection. Colors must be a var or derived from vars — no raw hex, including as light-dark() / color-mix() inputs. Please revert to --color-border; if a neutral-over-media edge is truly required, derive it from --color-on-light / --color-on-dark rather than raw hex.
Confirmed clean: the elevation removal is safe — it's not in the published CHANGELOG, and you've correctly edited the still-pending elevation changeset to drop Thumbnail.
Recommendation: request changes. All three have concrete targets — happy to pair on the touch-reveal approach.
|
@cixzhang to use |
|
The rule is no hardcoded colors to keep components themeable. Perhaps apply a color mix over one of the other tokens but we should avoid hardcoded hex |
5e0fd27 to
325a2a9
Compare
325a2a9 to
b41f77c
Compare
Adds `showRemoveOn?: 'always' | 'hover'` (default 'hover'). - 'hover' — the remove button is hidden at rest and revealed on hover or keyboard focus, and stays visible on any touch-capable device. The reveal is CSS-only (opacity) so the button stays mounted and remains reachable by keyboard and assistive tech; a scoped marker keeps the reveal responsive to this thumbnail only. - 'always' — the button is always shown at rest. Only applies when onRemove is set.
b41f77c to
e7bf76c
Compare
Summary
Adds a
showRemoveOnprop toThumbnailto control when the overlaid remove button is visible. This PR is scoped to only that prop — no changes to the remove-button colors, hover styling, or image hairline.showRemoveOnprop — defaults to'hover'Adds
showRemoveOn?: 'always' | 'hover'(default'hover'). Only has an effect whenonRemoveis set. The naming follows the existingOverlayshowOnvocabulary for consistency.'hover'— (default) the button is hidden at rest and revealed on hover or keyboard focus, and stays visible on any touch-capable device.'always'— the button is always shown at rest.Accessibility
The reveal is CSS-only (opacity) — the button stays mounted, so it remains keyboard- and screen-reader-reachable. Tabbing into the thumbnail triggers a
:focus-withinreveal. A scopeddefineMarker+stylex.when.ancestor(':hover' / ':focus-within')keeps the reveal responsive to this thumbnail rather than any outer hoverable container. An@media (any-pointer: coarse)branch keeps the button visible on any touch-capable device — including hybrid touchscreen laptops (which reporthover: hover+pointer: fineand would otherwise get no reveal on touch). Motion respectsprefers-reduced-motion.Implementation
The remove button is wrapped in a positioned "slot"
<div>that carries the opacity reveal and the scope marker (ancestor-marker styles can't ride on a child component'sxstyle). The existingMediaTheme/useImageModecontrast handling and the secondary-Button treatment are unchanged — the slot simply fades the whole button in/out.Testing
pnpm -F @astryxdesign/core build✅ (verified indist/astryx.css: the@media (any-pointer: coarse)reveal branch; no changes to remove-button colors or the--color-borderhairline)pnpm -F @astryxdesign/core typecheck✅showRemoveOn)generate+ 282 data-extraction tests passingsync:exports✅, token-docs ✅