Uh oh!
There was an error while loading. Please reload this page.
fix(ui): Ensure tab focus ring is visible for keyboard interactions - #8998
Conversation
🦋 Changeset detectedLatest commit: 939d5b0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughUpdates ChangesUI tab focus ring
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/ui/src/elements/Tabs.tsx (1)
175-178: 📐 Maintainability & Code Quality | 🔵 TrivialFlatten the focus-ring selectors here.
common.focusRing(t)already emits:focus,:focus:not(:focus-visible), and:focus-visible. Spreading it inside&:focus-visiblenests those selectors unnecessarily and makes the intent harder to follow. Keep:hoverseparate and apply the helper at the top level; it already suppresses the ring for non-keyboard focus.Suggested change
- '&:hover, &:focus-visible': { backgroundColor: t.colors.$transparent, boxShadow: 'none' },- '&:focus-visible': {- ...common.focusRing(t),- },+ '&:hover': { backgroundColor: t.colors.$transparent, boxShadow: 'none' },+ ...common.focusRing(t),🤖 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 `@packages/ui/src/elements/Tabs.tsx` around lines 175 - 178, The focus-ring styles in Tabs are nested too deeply because common.focusRing(t) already provides the needed focus selectors. Update the style block in Tabs to keep the &:hover rule separate and apply common.focusRing(t) at the top level instead of spreading it inside &:focus-visible, so the selector structure stays flat and easier to read.
🤖 Prompt for all review comments with 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.
Nitpick comments:
In `@packages/ui/src/elements/Tabs.tsx`:
- Around line 175-178: The focus-ring styles in Tabs are nested too deeply
because common.focusRing(t) already provides the needed focus selectors. Update
the style block in Tabs to keep the &:hover rule separate and apply
common.focusRing(t) at the top level instead of spreading it inside
&:focus-visible, so the selector structure stays flat and easier to read.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 31c08c4a-0eaf-4917-a757-b9c500180659
📒 Files selected for processing (2)
.changeset/ui-tab-focus-ring.mdpackages/ui/src/elements/Tabs.tsx
API Changes Report
Summary
No API Changes DetectedAll packages have stable APIs with no detected changes. Report generated by Break Check Last ran on |
@clerk/astro@clerk/backend@clerk/chrome-extension@clerk/clerk-js@clerk/electron@clerk/electron-passkeys@clerk/eslint-plugin@clerk/expo@clerk/expo-passkeys@clerk/express@clerk/fastify@clerk/hono@clerk/localizations@clerk/nextjs@clerk/nuxt@clerk/react@clerk/react-router@clerk/shared@clerk/tanstack-react-start@clerk/testing@clerk/ui@clerk/upgrade@clerk/vuecommit: |
Description
BEFORE
before.mov
AFTER
after.mov
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit