chore(ui,shared,localizations): UX improvements for <ConfigureSSO />#8601
chore(ui,shared,localizations): UX improvements for <ConfigureSSO />#8601LauraBeatris wants to merge 14 commits into
<ConfigureSSO />#8601Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 6ae19e6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 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 |
ba679b7 to
9774083
Compare
1279e85 to
23d3243
Compare
dc5d30e to
f1c419e
Compare
<ConfigureSSO />
<ConfigureSSO /><ConfigureSSO />
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@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/vue
commit: |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR updates Configure SSO end-to-end: many locale files now reference Single Sign-On (SSO) and simplify the select-provider localization shape; shared localization types were adjusted (selectProviderStep, testUrl action label, testResults.empty, provider-specific saml attributeMapping); UI changes include rehydrating provider selection, removing enterprise-connection creation from verify-domain, conditional mounting of select-provider, changing the permission key to org:sys_entconns:manage, refactoring ConfigureStep to provider-driven attribute mapping, adding an Open test URL flow and test-results empty state, instrumenting UI with elementDescriptor/elementId keys, and adding a primary-email-domain footer in the navbar. Tests were updated to reflect copy and permission-key changes. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/src/components/ConfigureSSO/steps/TestConfigurationStep.tsx (1)
372-381:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClickable table rows are mouse-only; keyboard users can’t open test-run details.
The row action is bound only to
onClickon<Tr>, so the drawer cannot be triggered via keyboard navigation. Please make the row action keyboard-accessible (focusable + Enter/Space activation) or use a semantic interactive element inside cells.Suggested fix
<Tr key={row.id} elementDescriptor={descriptors.configureSSOTestResultsRow} + role='button' + tabIndex={0} onClick={() => setSelectedTestRun(row)} + onKeyDown={e => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + setSelectedTestRun(row); + } + }} sx={t => ({ cursor: 'pointer', '&:hover > td': { backgroundColor: t.colors.$neutralAlpha50, }, })} >As per coding guidelines: "Implement proper focus management for keyboard navigation in React components" and "Implement proper tab order in React components".
🤖 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/components/ConfigureSSO/steps/TestConfigurationStep.tsx` around lines 372 - 381, The table row <Tr> with key={row.id} and elementDescriptor={descriptors.configureSSOTestResultsRow} only handles onClick so keyboard users cannot open a test run; make it keyboard-accessible by adding tabIndex={0} (or wrapping an interactive element inside the cells), add a onKeyDown handler that calls setSelectedTestRun(row) when Enter or Space is pressed, and include an appropriate role (e.g., role="button") and aria-label/aria-describedby as needed to convey purpose; ensure styling and focus outline are preserved so keyboard focus is visible.
🤖 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.
Outside diff comments:
In `@packages/ui/src/components/ConfigureSSO/steps/TestConfigurationStep.tsx`:
- Around line 372-381: The table row <Tr> with key={row.id} and
elementDescriptor={descriptors.configureSSOTestResultsRow} only handles onClick
so keyboard users cannot open a test run; make it keyboard-accessible by adding
tabIndex={0} (or wrapping an interactive element inside the cells), add a
onKeyDown handler that calls setSelectedTestRun(row) when Enter or Space is
pressed, and include an appropriate role (e.g., role="button") and
aria-label/aria-describedby as needed to convey purpose; ensure styling and
focus outline are preserved so keyboard focus is visible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: bfc6c94d-6fa9-4cf7-9b36-95d99428637c
📒 Files selected for processing (1)
packages/ui/src/components/ConfigureSSO/steps/TestConfigurationStep.tsx
| title={localizationKeys('configureSSO.navbar.title')} | ||
| routes={[]} | ||
| contentRef={contentRef} | ||
| footer={<PrimaryEmailDomainFooter />} |
There was a problem hiding this comment.
we now have three slots that all render in the same spot. Do we need to introduce another? footer also feels like the wrong name here, as this floats towards the top of the sidebar
{header}
{items}
{footer}There was a problem hiding this comment.
I'll check with Tu and Jeremy again to see if we can find out another option to tackle this domain UX issue, but I'll rever the changes from the sidebar for the time being
52be397 to
5bf8241
Compare
Description
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change