Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/components/shop/ProductCard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@
className="
group flex flex-col min-w-[340px] max-w-[400px] w-full rounded-xl
border border-transparent bg-transparent
hover:bg-shop-bg-2 hover:border-shop-line-2
hover:bg-[#EFEFE3] dark:hover:bg-shop-bg-2 hover:border-shop-line-2
transition-[border-color,background-color] duration-200
px-[22px] pt-7 pb-5
"
Expand Down Expand Up @@ -241,7 +241,7 @@
if (onQuickView) {
return (
<div
role="button"

Check warning on line 244 in src/components/shop/ProductCard.tsx

View workflow job for this annotation

GitHub Actions / PR

eslint-plugin-jsx-a11y(prefer-tag-over-role)

Prefer `button` over `role` attribute `button`.
tabIndex={0}
onClick={() => onQuickView(product.handle)}
onKeyDown={(e) => {
Expand Down
39 changes: 29 additions & 10 deletions src/components/shop/ProductDrawer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@
espresso: '#3c1f0f',
}

function contrastColor(hex: string): string {

Check warning on line 68 in src/components/shop/ProductDrawer.tsx

View workflow job for this annotation

GitHub Actions / PR

eslint(no-unused-vars)

Function 'contrastColor' is declared but never used.
const h = hex.replace('#', '')
const r = parseInt(h.slice(0, 2), 16)
const g = parseInt(h.slice(2, 4), 16)
Expand Down Expand Up @@ -145,18 +145,37 @@
const isOpen = !!productHandle

// Keep the last-known handle alive through the exit animation so the drawer
// slides out with content visible (not empty). Cleared after 400ms — just
// past the 380ms transition — so the body unmounts cleanly after exit.
// slides out with content visible (not empty). Uses the derived-state pattern
// so displayHandle is updated synchronously on open (no empty-frame flash).
const [displayHandle, setDisplayHandle] = React.useState<string | null>(null)
const [prevProductHandle, setPrevProductHandle] = React.useState<
string | null
>(null)
if (productHandle !== prevProductHandle) {
setPrevProductHandle(productHandle)
if (productHandle) setDisplayHandle(productHandle)
}

// Clear displayHandle after exit animation completes
React.useEffect(() => {
if (productHandle) {
setDisplayHandle(productHandle)
} else {
if (!productHandle) {
const t = setTimeout(() => setDisplayHandle(null), 400)
return () => clearTimeout(t)
}
}, [productHandle])

// Pre-fetch product data so the drawer only animates open once content is ready,
// preventing the skeleton flash. Same query key as DrawerBody so cache is shared.
const { data: prefetchedProduct } = useQuery({
queryKey: ['shopify', 'product', displayHandle ?? ''],
queryFn: () => getProduct({ data: { handle: displayHandle! } }),
enabled: !!displayHandle,
staleTime: 5 * 60 * 1000,
})

// Delay the open animation until data is in cache. Close animates immediately.
const isAnimatedOpen = isOpen && !!prefetchedProduct
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoff

Scrim gated on isAnimatedOpen leaves users with zero click feedback on slow connections.

isAnimatedOpen = isOpen && !!prefetchedProduct means that while data is fetching (cold cache), both the scrim and the drawer stay fully invisible. A user clicking a product card sees nothing happen for the entire round-trip duration — a regression compared to the previous skeleton approach.

The scrim dimming the background is itself a cheap, immediate affordance that doesn't require product data. Only the drawer slide needs to wait for isAnimatedOpen. A simple split:

💡 Proposed fix — show scrim immediately, gate only the drawer on `isAnimatedOpen`
-        tabIndex={isAnimatedOpen ? 0 : -1}
+        tabIndex={isOpen ? 0 : -1}
         onClick={onClose}
         className={twMerge(
           'fixed inset-0 z-[60] bg-black/50 backdrop-blur-sm transition-opacity duration-300',
-          isAnimatedOpen
+          isOpen
             ? 'opacity-100 pointer-events-auto'
             : 'opacity-0 pointer-events-none',
         )}

The drawer's translate-x-fulltranslate-x-0 transition still only fires when isAnimatedOpen is true, so the drawer slides in with content already rendered and no skeleton flash.

Also applies to: 241-251

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/components/shop/ProductDrawer.tsx` at line 177, The scrim is currently
tied to isAnimatedOpen (const isAnimatedOpen = isOpen && !!prefetchedProduct)
causing no immediate click feedback when prefetchedProduct is null; change the
rendering so the scrim uses isOpen (show scrim immediately when isOpen is true)
while keeping the drawer slide transition gated by isAnimatedOpen (only apply
the translate-x-full → translate-x-0 transition/class when isAnimatedOpen is
true). Update any conditional rendering/className logic that references
isAnimatedOpen for the scrim to use isOpen instead and keep isAnimatedOpen for
the drawer element (the element that toggles translate-x-full/translate-x-0).


const effectiveWidth = width

const navigateStep = React.useCallback(
Expand Down Expand Up @@ -222,11 +241,11 @@
<button
type="button"
aria-label="Close product drawer"
tabIndex={isOpen ? 0 : -1}
tabIndex={isAnimatedOpen ? 0 : -1}
onClick={onClose}
className={twMerge(
'fixed inset-0 z-[60] bg-black/50 backdrop-blur-sm transition-opacity duration-300',
isOpen
isAnimatedOpen
? 'opacity-100 pointer-events-auto'
: 'opacity-0 pointer-events-none',
)}
Expand All @@ -236,8 +255,8 @@
<aside
ref={drawerRef}
aria-label="Product detail"
aria-hidden={!isOpen}
style={{ width: isOpen ? effectiveWidth : undefined }}
aria-hidden={!isAnimatedOpen}
style={{ width: effectiveWidth }}
className={twMerge(
'fixed top-[48px] right-0 bottom-0 z-[70]',
'border-l border-shop-line flex flex-col',
Expand All @@ -246,7 +265,7 @@
isDragging
? 'transition-none select-none'
: 'transition-transform duration-[380ms] ease-[cubic-bezier(0.2,0.8,0.2,1)]',
isOpen ? 'translate-x-0' : 'translate-x-full',
isAnimatedOpen ? 'translate-x-0' : 'translate-x-full',
)}
>
{/* Splitter handle */}
Expand Down Expand Up @@ -367,9 +386,9 @@

function DrawerContent({
product,
allHandles,

Check warning on line 389 in src/components/shop/ProductDrawer.tsx

View workflow job for this annotation

GitHub Actions / PR

eslint(no-unused-vars)

Parameter 'allHandles' is declared but never used. Unused parameters should match /(^_)|(^__+$)|(^e$)|(^error$)/.
onNavigate,

Check warning on line 390 in src/components/shop/ProductDrawer.tsx

View workflow job for this annotation

GitHub Actions / PR

eslint(no-unused-vars)

Parameter 'onNavigate' is declared but never used. Unused parameters should match /(^_)|(^__+$)|(^e$)|(^error$)/.
onClose,

Check warning on line 391 in src/components/shop/ProductDrawer.tsx

View workflow job for this annotation

GitHub Actions / PR

eslint(no-unused-vars)

Parameter 'onClose' is declared but never used. Unused parameters should match /(^_)|(^__+$)|(^e$)|(^error$)/.
}: {
product: ProductDetail
allHandles: string[]
Expand Down Expand Up @@ -400,7 +419,7 @@
// updates immediately on color pick even before size is chosen.
const variantForImage = findMatchingVariant(variants, selected)
React.useEffect(() => {
if (variantForImage?.image) setHeroOverride(variantForImage.image)

Check warning on line 422 in src/components/shop/ProductDrawer.tsx

View workflow job for this annotation

GitHub Actions / PR

eslint-plugin-react-hooks(exhaustive-deps)

React Hook useEffect has a missing dependency: 'variantForImage.image'
else setHeroOverride(null)
}, [variantForImage?.id, variantForImage?.image?.url])

Expand Down
194 changes: 173 additions & 21 deletions src/components/shop/ui/Select.tsx
Original file line number Diff line number Diff line change
@@ -1,27 +1,179 @@
import * as React from 'react'
import { twMerge } from 'tailwind-merge'

type Props = React.SelectHTMLAttributes<HTMLSelectElement>

/** Styled native <select> with a carved-in chevron. */
export const ShopSelect = React.forwardRef<HTMLSelectElement, Props>(
function ShopSelect({ className, children, ...rest }, ref) {
return (
<select
ref={ref}
{...rest}
type OptionData = { value: string; label: string }

type Props = {
value?: string
onChange?: (e: { target: { value: string } }) => void
className?: string
triggerClassName?: string
children?: React.ReactNode
}

function extractOptions(children: React.ReactNode): Array<OptionData> {
const options: Array<OptionData> = []
React.Children.forEach(children, (child) => {
if (!React.isValidElement(child) || child.type !== 'option') return
const el = child as React.ReactElement<{
value?: string
children?: React.ReactNode
}>
const value = String(el.props.value ?? '')
const label =
typeof el.props.children === 'string'
? el.props.children
: String(el.props.value ?? '')
options.push({ value, label })
})
return options
}

/** Custom themed dropdown. Keeps the same onChange API as a native <select>
* so all call-sites stay unchanged. */
export function ShopSelect({
value,
onChange,
className,
triggerClassName,
children,
}: Props) {
const [open, setOpen] = React.useState(false)
const [focused, setFocused] = React.useState<string | null>(null)
const triggerRef = React.useRef<HTMLButtonElement>(null)
const menuRef = React.useRef<HTMLDivElement>(null)

const options = extractOptions(children)
const selected = options.find((o) => o.value === value)

// Close on outside click
React.useEffect(() => {
if (!open) return
const handler = (e: MouseEvent) => {
if (
!triggerRef.current?.contains(e.target as Node) &&
!menuRef.current?.contains(e.target as Node)
) {
setOpen(false)
}
}
document.addEventListener('mousedown', handler)
return () => document.removeEventListener('mousedown', handler)
}, [open])

// Keyboard navigation
const onKeyDown = (e: React.KeyboardEvent) => {
if (!open) {
if (e.key === 'Enter' || e.key === ' ' || e.key === 'ArrowDown') {
e.preventDefault()
setOpen(true)
setFocused(value ?? options[0]?.value ?? null)
}
return
}
const idx = options.findIndex((o) => o.value === focused)
if (e.key === 'ArrowDown') {
e.preventDefault()
setFocused(options[Math.min(idx + 1, options.length - 1)]?.value ?? null)
} else if (e.key === 'ArrowUp') {
e.preventDefault()
setFocused(options[Math.max(idx - 1, 0)]?.value ?? null)
} else if (e.key === 'Enter' || e.key === ' ') {
e.preventDefault()
if (focused) select(focused)
} else if (e.key === 'Escape') {
setOpen(false)
triggerRef.current?.focus()
}
}

const select = (optValue: string) => {
onChange?.({ target: { value: optValue } })
setOpen(false)
triggerRef.current?.focus()
}

return (
<div className={twMerge('relative', className)} onKeyDown={onKeyDown}>

Check failure on line 97 in src/components/shop/ui/Select.tsx

View workflow job for this annotation

GitHub Actions / PR

eslint-plugin-jsx-a11y(no-static-element-interactions)

Static HTML elements with event handlers require a role.
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Move keyboard handler off the static wrapper to fix a11y/lint blocker.

At Line 97, onKeyDown is attached to a non-interactive <div>, which triggers jsx-a11y/no-static-element-interactions and can block CI. Bind the handler to interactive elements (trigger/menu) instead.

Suggested fix
-    <div className={twMerge('relative', className)} onKeyDown={onKeyDown}>
+    <div className={twMerge('relative', className)}>
       <button
         ref={triggerRef}
         type="button"
+        onKeyDown={onKeyDown}
         aria-haspopup="listbox"
         aria-expanded={open}
@@
       {open ? (
         <div
           ref={menuRef}
           role="listbox"
+          onKeyDown={onKeyDown}
           className={twMerge(

Also applies to: 138-140

🧰 Tools
🪛 GitHub Check: PR

[failure] 97-97: eslint-plugin-jsx-a11y(no-static-element-interactions)
Static HTML elements with event handlers require a role.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/components/shop/ui/Select.tsx` at line 97, The onKeyDown handler is
currently attached to a non-interactive wrapper div in the Select component (the
element using twMerge('relative', className)), causing an a11y/lint error; move
the onKeyDown binding off that static wrapper and attach it to the interactive
elements instead (the trigger/button element that opens the dropdown and/or the
dropdown/menu listbox element used by this Select) so keyboard events are
handled on interactive controls; update references to the onKeyDown prop/handler
used in this component (and the similar occurrences around the menu/trigger
logic later in the file) to ensure the same behavior is preserved while
satisfying jsx-a11y/no-static-element-interactions.

<button
ref={triggerRef}
type="button"
aria-haspopup="listbox"
aria-expanded={open}
onClick={() => {
setOpen((o) => !o)
if (!open) setFocused(value ?? options[0]?.value ?? null)
}}
className={twMerge(
'appearance-none border border-shop-line-2 rounded-xl',
'py-2.5 pl-4 pr-8 bg-transparent text-shop-text-2 font-shop-mono text-shop-ui',
'bg-no-repeat bg-[right_12px_center]',
// inline SVG chevron, colored with the muted text token
"bg-[url(\"data:image/svg+xml;utf8,<svg xmlns='http://www.w3.org/2000/svg' width='10' height='10' viewBox='0 0 10 10'><path d='M2 4l3 3 3-3' stroke='%23a8a8b0' fill='none' stroke-width='1.4'/></svg>\")]",
'cursor-pointer transition-colors hover:border-shop-muted hover:text-shop-text',
className,
'flex items-center gap-2 border border-shop-line-2 rounded-xl',
'py-2.5 pl-4 pr-3 bg-transparent text-shop-text-2 font-shop-mono text-shop-ui',
'cursor-pointer transition-colors select-none whitespace-nowrap',
'hover:border-shop-muted hover:text-shop-text',
open && 'border-shop-muted text-shop-text',
triggerClassName,
)}
>
{children}
</select>
)
},
)
{selected?.label ?? '—'}
<svg
width="10"
height="10"
viewBox="0 0 10 10"
aria-hidden
className={twMerge(
'transition-transform duration-150 shrink-0',
open && 'rotate-180',
)}
>
<path
d="M2 4l3 3 3-3"
stroke="currentColor"
fill="none"
strokeWidth="1.4"
strokeLinecap="round"
/>
</svg>
</button>

{open ? (
<div
ref={menuRef}
role="listbox"
className={twMerge(
'absolute right-0 top-[calc(100%+6px)] z-[200] min-w-full',
'bg-shop-panel border border-shop-line rounded-xl overflow-hidden',
'shadow-[0_8px_24px_-4px_rgba(0,0,0,0.18),0_2px_8px_-2px_rgba(0,0,0,0.12)]',
)}
>
{options.map((opt) => {
const isSelected = opt.value === value
const isFocused = opt.value === focused
return (
<button
key={opt.value}
type="button"
role="option"
aria-selected={isSelected}
onMouseEnter={() => setFocused(opt.value)}
onClick={() => select(opt.value)}
className={twMerge(
'w-full text-left px-4 py-2.5 font-shop-mono text-shop-ui whitespace-nowrap',
'transition-colors duration-75 flex items-center gap-2',
isSelected ? 'text-shop-accent' : 'text-shop-text-2',
isFocused && 'bg-shop-surface text-shop-text',
)}
>
<span
className={twMerge(
'w-1.5 h-1.5 rounded-full shrink-0 transition-opacity',
isSelected ? 'bg-shop-accent opacity-100' : 'opacity-0',
)}
/>
{opt.label}
</button>
)
})}
</div>
) : null}
</div>
)
}
15 changes: 8 additions & 7 deletions src/components/shop/ui/Tab.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import * as React from 'react'
import { twMerge } from 'tailwind-merge'

type Props = React.ButtonHTMLAttributes<HTMLButtonElement> & {
isActive?: boolean
Expand All @@ -11,18 +12,18 @@ export const ShopTab = React.forwardRef<HTMLButtonElement, Props>(
{ isActive, count: _count, children, className, ...rest },
ref,
) {
const base =
'inline-flex items-center px-4 py-2.5 rounded-xl font-shop-mono text-shop-ui transition-colors cursor-pointer'
const state = isActive
? 'border-2 border-shop-muted font-medium text-shop-text'
: 'border border-shop-line-2 font-normal text-shop-text-2 hover:text-shop-text hover:border-shop-muted'

return (
<button
ref={ref}
type="button"
{...rest}
className={`${base} ${state}${className ? ` ${className}` : ''}`}
className={twMerge(
'inline-flex items-center px-4 py-2.5 rounded-xl font-shop-mono text-shop-ui transition-colors cursor-pointer',
isActive
? 'border-2 border-shop-muted font-medium text-shop-text'
: 'border border-shop-line-2 font-normal text-shop-text-2 hover:text-shop-text hover:border-shop-muted',
className,
)}
>
{children}
</button>
Expand Down
Loading
Loading