Conversation
ogzhanolguncu
requested review from
MichaelUnkey,
chronark,
mcstepp and
perkinsjr
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
📝 WalkthroughWalkthroughThis pull request introduces a new, fully documented Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FormInput
participant InputElement
User->>FormInput: Provide input and configuration (label, description, etc.)
FormInput->>InputElement: Render input with appropriate props and ARIA attributes
alt Error State
InputElement-->>FormInput: Trigger validation error
FormInput->>User: Display error message with associated icon
else Valid Input
InputElement-->>FormInput: Accept user input
FormInput->>User: Continue with normal/success display
end
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![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
🧹 Nitpick comments (8)
apps/engineering/content/design/components/form-input.mdx (2)
89-89: Fix punctuation in props table.Remove the trailing semicolon after the variant type definition for consistency.
- variant?: 'default' | 'success' | 'warning' | 'error'; + variant?: 'default' | 'success' | 'warning' | 'error'🧰 Tools
🪛 LanguageTool
[uncategorized] ~89-~89: Loose punctuation mark.
Context: ...t' | 'success' | 'warning' | 'error'; }`} /> ## Accessibility FormInput is built...(UNLIKELY_OPENING_PUNCTUATION)
103-103: Improve grammar in best practices section.Change "Keep error messages" to "To keep error messages" for grammatical correctness.
-- Keep error messages specific and actionable ++ To keep error messages specific and actionable🧰 Tools
🪛 LanguageTool
[grammar] ~103-~103: The verb “Keep” needs to be in the to-infinitive form.
Context: ...text to provide additional context when needed - Keep error messages specific and actionable ...(MISSING_TO_BEFORE_A_VERB)
internal/ui/src/components/form/form-input.tsx (3)
6-11: Add JSDoc comments for props.Consider adding detailed JSDoc comments for better IDE integration and documentation.
export interface FormInputProps extends InputProps { + /** Label text displayed above the input */ label?: string; + /** Additional description or help text */ description?: string; + /** Whether the field is mandatory */ required?: boolean; + /** Error message displayed when validation fails */ error?: string; }
51-54: Enhance error message accessibility.Consider adding
aria-live="polite"to improve screen reader announcements of error messages.- <div id={errorId} role="alert" className="text-error-11 flex gap-2 items-center"> + <div id={errorId} role="alert" aria-live="polite" className="text-error-11 flex gap-2 items-center">
48-76: Optimize conditional rendering.Consider extracting the description/error section into a separate component to improve readability and maintainability.
+const FormInputMessage = ({ error, description, errorId, descriptionId, variant }) => { + if (!description && !error) return null; + + if (error) { + return ( + <div id={errorId} role="alert" aria-live="polite" className="text-error-11 flex gap-2 items-center"> + <TriangleWarning2 aria-hidden="true" /> + {error} + </div> + ); + } + + return ( + <output + id={descriptionId} + className={cn( + "text-gray-9 flex gap-2 items-center", + variant === "success" ? "text-success-11" : variant === "warning" ? "text-warning-11" : "", + )} + > + {variant === "warning" ? ( + <TriangleWarning2 size="md-regular" aria-hidden="true" /> + ) : ( + <CircleInfo size="md-regular" aria-hidden="true" /> + )} + <span>{description}</span> + </output> + ); +};Then use it in the component:
- {(description || error) && ( - <div className="text-[13px] leading-5"> - {error ? ( - <div id={errorId} role="alert" className="text-error-11 flex gap-2 items-center"> - <TriangleWarning2 aria-hidden="true" /> - {error} - </div> - ) : description ? ( - <output - id={descriptionId} - className={cn( - "text-gray-9 flex gap-2 items-center", - variant === "success" - ? "text-success-11" - : variant === "warning" - ? "text-warning-11" - : "", - )} - > - {variant === "warning" ? ( - <TriangleWarning2 size="md-regular" aria-hidden="true" /> - ) : ( - <CircleInfo size="md-regular" aria-hidden="true" /> - )} - <span>{description}</span> - </output> - ) : null} - </div> - )} + <div className="text-[13px] leading-5"> + <FormInputMessage + error={error} + description={description} + errorId={errorId} + descriptionId={descriptionId} + variant={variant} + /> + </div>apps/engineering/content/design/components/form/form-input.variants.tsx (3)
4-14: Add TypeScript type safety to the component.Consider adding explicit return type and making the component type-safe.
-export const DefaultFormInputVariant = () => { +export const DefaultFormInputVariant = (): JSX.Element => {
17-131: Apply consistent type safety across all variant components.All variant components should have explicit return types for consistency and type safety.
-export const RequiredFormInputVariant = () => { +export const RequiredFormInputVariant = (): JSX.Element => { -export const SuccessFormInputVariant = () => { +export const SuccessFormInputVariant = (): JSX.Element => { // Apply similar changes to all remaining variants
53-54: Enhance accessibility for specific input types.Consider adding appropriate ARIA attributes and input-specific attributes for better accessibility:
// For password input type="password" + aria-label="Password input" + autoComplete="current-password" placeholder="Enter your password" // For URL input placeholder="https://api.yourdomain.com/webhooks" className="max-w-lg" id="webhook-url-input" + type="url" + autoComplete="url" + pattern="https://.*"Also applies to: 125-128
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/engineering/content/design/components/form-input.mdx(1 hunks)apps/engineering/content/design/components/form/form-input.variants.tsx(1 hunks)apps/engineering/content/design/components/input.mdx(0 hunks)internal/ui/src/components/form/form-input.tsx(1 hunks)internal/ui/src/components/form/index.tsx(1 hunks)internal/ui/src/components/input.tsx(1 hunks)internal/ui/src/index.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/engineering/content/design/components/input.mdx
✅ Files skipped from review due to trivial changes (1)
- internal/ui/src/components/form/index.tsx
🧰 Additional context used
🪛 LanguageTool
apps/engineering/content/design/components/form-input.mdx
[uncategorized] ~89-~89: Loose punctuation mark.
Context: ...t' | 'success' | 'warning' | 'error'; }`} /> ## Accessibility FormInput is built...
(UNLIKELY_OPENING_PUNCTUATION)
[grammar] ~103-~103: The verb “Keep” needs to be in the to-infinitive form.
Context: ...text to provide additional context when needed - Keep error messages specific and actionable ...
(MISSING_TO_BEFORE_A_VERB)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./internal/id
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: Test Go API Local / test_agent_local
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Build / Build
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
internal/ui/src/index.ts (1)
7-7: LGTM!The new export statement follows the established pattern and correctly exposes the form components.
internal/ui/src/components/input.tsx (1)
16-18:Details
❓ Verification inconclusive
LGTM! Verify color contrast ratios for accessibility.
The changes improve visual feedback by using higher contrast borders and subtler focus rings. However, let's verify the color contrast ratios meet WCAG guidelines.
Also applies to: 21-23, 26-28
🏁 Script executed:
#!/bin/bash # Description: Check color contrast ratios using WCAG guidelines # Note: This script requires the contrast-cli package # Install contrast-cli npm install -g contrast-cli # Check contrast ratios for success state contrast-cli check --background "#ffffff" --foreground "var(--success-9)" contrast-cli check --background "#ffffff" --foreground "var(--success-10)" # Check contrast ratios for warning state contrast-cli check --background "#ffffff" --foreground "var(--warning-9)" contrast-cli check --background "#ffffff" --foreground "var(--warning-10)" # Check contrast ratios for error state contrast-cli check --background "#ffffff" --foreground "var(--error-9)" contrast-cli check --background "#ffffff" --foreground "var(--error-10)"Length of output: 1516
Accessibility Verification Needed: Manual Check for Color Contrast Ratios
The updated styles in
internal/ui/src/components/input.tsx(lines 16–18, and similarly applied at lines 21–23 and 26–28) improve the visual feedback by using higher contrast borders and subtler focus rings. However, the automated check using thecontrast-clitool failed (the package wasn’t found), so we couldn’t verify the WCAG compliance automatically.Please manually verify the contrast ratios using a trusted accessibility tool or your local setup to ensure they meet WCAG guidelines.
apps/engineering/content/design/components/form/form-input.variants.tsx (1)
1-2: LGTM! Clean imports following good practices.The imports are well-organized, with a clear separation between local and package imports.
2 participants
What does this PR do?
Fixes # (issue)
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit