refactor(ui): Improve SelectOption consistency#8347
refactor(ui): Improve SelectOption consistency#8347alexcarpenter wants to merge 3 commits intomainfrom
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis pull request introduces a new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ui/src/components/SignIn/SignInStart.tsx`:
- Around line 81-212: Remove the prototype demo by deleting the SelectOptionDemo
component and its invocation inside SignInStart, and also remove the now-unused
imports Select, SelectButton, SelectOption, SelectOptionList, and Text from the
top of the file; ensure SignInStart only renders the intended sign-in form and
run the linter/TS checks to clean up any remaining unused symbols or types (look
for SelectOptionDemo, its JSX usage in SignInStart, and import declarations that
mention Select/SelectButton/SelectOption/SelectOptionList/Text).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: c1935b6d-1771-4e57-a7df-6c2823758f76
📒 Files selected for processing (7)
packages/ui/src/components/APIKeys/CreateAPIKeyForm.tsxpackages/ui/src/components/Checkout/CheckoutForm.tsxpackages/ui/src/components/OAuthConsent/OrgSelect.tsxpackages/ui/src/components/OrganizationProfile/MemberListTable.tsxpackages/ui/src/components/SignIn/SignInStart.tsxpackages/ui/src/elements/PhoneInput/index.tsxpackages/ui/src/elements/Select.tsx
| // PROTOTYPE: Remove before merging | ||
| const SelectOptionDemo = () => { | ||
| const [orgValue, setOrgValue] = useState<string | null>('org_1'); | ||
| const [phoneValue, setPhoneValue] = useState<string | null>('us'); | ||
| const [roleValue, setRoleValue] = useState<string | null>('admin'); | ||
| const [defaultValue, setDefaultValue] = useState<string | null>('30d'); | ||
|
|
||
| const orgOptions = [ | ||
| { value: 'org_1', label: 'Acme Corp', logoUrl: 'https://img.clerk.com/placeholder' }, | ||
| { | ||
| value: 'org_2', | ||
| label: 'Globex Industries Globex Industries Globex Industries', | ||
| logoUrl: 'https://img.clerk.com/placeholder', | ||
| }, | ||
| { value: 'org_3', label: 'Initech', logoUrl: 'https://img.clerk.com/placeholder' }, | ||
| ]; | ||
|
|
||
| const phoneOptions = [ | ||
| { value: 'us', label: 'United States', code: '1' }, | ||
| { value: 'gb', label: 'United Kingdom', code: '44' }, | ||
| { value: 'de', label: 'Germany', code: '49' }, | ||
| { value: 'fr', label: 'France', code: '33' }, | ||
| ]; | ||
|
|
||
| const roleOptions = [ | ||
| { value: 'admin', label: 'Admin' }, | ||
| { value: 'member', label: 'Member' }, | ||
| { value: 'guest', label: 'Guest' }, | ||
| ]; | ||
|
|
||
| const expirationOptions = [ | ||
| { value: '7d', label: '7 days' }, | ||
| { value: '30d', label: '30 days' }, | ||
| { value: '90d', label: '90 days' }, | ||
| { value: 'never', label: 'Never' }, | ||
| ]; | ||
|
|
||
| return ( | ||
| <Col | ||
| gap={4} | ||
| sx={{ padding: '16px 0' }} | ||
| > | ||
| <Text variant='subtitle'>SelectOption Variants (prototype)</Text> | ||
|
|
||
| <Text | ||
| colorScheme='secondary' | ||
| sx={{ fontSize: '12px' }} | ||
| > | ||
| Image + Label (OrgSelect pattern) | ||
| </Text> | ||
| <Select | ||
| options={orgOptions} | ||
| value={orgValue} | ||
| onChange={option => setOrgValue(option.value)} | ||
| matchTriggerWidth | ||
| renderOption={(option, _index, isSelected) => ( | ||
| <SelectOption isSelected={isSelected}> | ||
| <SelectOption.Image | ||
| src={option.logoUrl} | ||
| alt={option.label} | ||
| /> | ||
| <SelectOption.Label>{option.label}</SelectOption.Label> | ||
| </SelectOption> | ||
| )} | ||
| > | ||
| <SelectButton>{orgOptions.find(o => o.value === orgValue)?.label || 'Select org'}</SelectButton> | ||
| <SelectOptionList /> | ||
| </Select> | ||
|
|
||
| <Text | ||
| colorScheme='secondary' | ||
| sx={{ fontSize: '12px' }} | ||
| > | ||
| Label + Detail (PhoneInput pattern) | ||
| </Text> | ||
| <Select | ||
| options={phoneOptions} | ||
| value={phoneValue} | ||
| onChange={option => setPhoneValue(option.value)} | ||
| matchTriggerWidth | ||
| renderOption={(option, _index, isSelected) => ( | ||
| <SelectOption isSelected={isSelected}> | ||
| <SelectOption.Label>{option.label}</SelectOption.Label> | ||
| <SelectOption.Detail>+{option.code}</SelectOption.Detail> | ||
| </SelectOption> | ||
| )} | ||
| > | ||
| <SelectButton>{phoneOptions.find(o => o.value === phoneValue)?.label || 'Select country'}</SelectButton> | ||
| <SelectOptionList /> | ||
| </Select> | ||
|
|
||
| <Text | ||
| colorScheme='secondary' | ||
| sx={{ fontSize: '12px' }} | ||
| > | ||
| Label only (RoleSelect pattern) | ||
| </Text> | ||
| <Select | ||
| options={roleOptions} | ||
| value={roleValue} | ||
| onChange={option => setRoleValue(option.value)} | ||
| matchTriggerWidth | ||
| renderOption={(option, _index, isSelected) => ( | ||
| <SelectOption isSelected={isSelected}> | ||
| <SelectOption.Label>{option.label}</SelectOption.Label> | ||
| </SelectOption> | ||
| )} | ||
| > | ||
| <SelectButton>{roleOptions.find(o => o.value === roleValue)?.label || 'Select role'}</SelectButton> | ||
| <SelectOptionList /> | ||
| </Select> | ||
|
|
||
| <Text | ||
| colorScheme='secondary' | ||
| sx={{ fontSize: '12px' }} | ||
| > | ||
| Default renderer (Checkout/APIKeys pattern) | ||
| </Text> | ||
| <Select | ||
| options={expirationOptions} | ||
| value={defaultValue} | ||
| onChange={option => setDefaultValue(option.value)} | ||
| matchTriggerWidth | ||
| > | ||
| <SelectButton> | ||
| {expirationOptions.find(o => o.value === defaultValue)?.label || 'Select expiration'} | ||
| </SelectButton> | ||
| <SelectOptionList /> | ||
| </Select> | ||
| </Col> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Remove prototype demo before merging.
SelectOptionDemo and its render site at line 708 are marked by the author as "PROTOTYPE: Remove before merging" and are unconditionally rendered inside the real SignInStart card. If this lands on main as-is, every integrator's Sign-In screen will display the demo selects above the actual form. Please strip the component, its usage, and the now-unused Select/SelectButton/SelectOption/SelectOptionList/Text imports (line 17 and Text on line 36) before marking the PR ready.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/components/SignIn/SignInStart.tsx` around lines 81 - 212,
Remove the prototype demo by deleting the SelectOptionDemo component and its
invocation inside SignInStart, and also remove the now-unused imports Select,
SelectButton, SelectOption, SelectOptionList, and Text from the top of the file;
ensure SignInStart only renders the intended sign-in form and run the linter/TS
checks to clean up any remaining unused symbols or types (look for
SelectOptionDemo, its JSX usage in SignInStart, and import declarations that
mention Select/SelectButton/SelectOption/SelectOptionList/Text).
1 participant
Description
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change