Fix FormControl aria-describedby attribute handling for improved accessibility#133
Fix FormControl aria-describedby attribute handling for improved accessibility#133codegen-sh[bot] wants to merge 2 commits into
Conversation
|
|
|
📝 Storybook Preview: View Storybook This preview will be updated automatically when you push new changes to this PR.
|
WalkthroughRoot package.json reformatted the workspaces array to multi-line without content changes. packages/components/package.json version bumped from 0.19.4 to 0.19.5. In form.tsx, aria-describedby computation changed to an array-based approach, conditionally applying the attribute only when IDs exist and including the message ID only when error is present. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant F as FormControl (form.tsx)
participant A as Accessibility IDs
participant D as DOM Props
Note over F: Render cycle
F->>A: Compute descriptionId
F->>A: Compute messageId (error state may apply)
A-->>F: IDs (descriptionId?, messageId?)
F->>F: Build ariaDescribedBy = []
alt descriptionId exists
F->>F: Push descriptionId
end
alt error is true and messageId exists
F->>F: Push messageId
end
alt ariaDescribedBy has entries
F->>D: Set aria-describedby = joined IDs
else
F->>D: Omit aria-describedby
end
F->>D: Set aria-invalid based on error
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/components/src/ui/form.tsx (2)
108-116: Good fix to conditionally apply aria-describedby; suggest merging user-provided tokens and tightening types.Two improvements:
- Preserve any existing aria-describedby from the consumer instead of overwriting it.
- Explicitly type the array for clarity and dedupe tokens to avoid repeated IDs.
Apply this diff:
- // Fix: Only include aria-describedby if there's an error or description - const ariaDescribedBy = []; - if (computedDescriptionId) ariaDescribedBy.push(computedDescriptionId); - if (error && computedMessageId) ariaDescribedBy.push(computedMessageId); + // Fix: Only include aria-describedby when relevant IDs exist; merge any user-provided tokens + const ariaDescribedBy: string[] = []; + const userDescribedBy = (restProps as any)['aria-describedby'] as string | undefined; + if (userDescribedBy) ariaDescribedBy.push(...userDescribedBy.trim().split(/\s+/)); + if (fromPropsDesc) ariaDescribedBy.push(computedDescriptionId); + if (error && computedMessageId) ariaDescribedBy.push(computedMessageId); @@ - ...(ariaDescribedBy.length > 0 ? { 'aria-describedby': ariaDescribedBy.join(' ') } : {}), + ...(ariaDescribedBy.length > 0 + ? { 'aria-describedby': Array.from(new Set(ariaDescribedBy)).join(' ') } + : {}),Notes:
- Using fromPropsDesc instead of computedDescriptionId avoids assuming a description exists merely because the context can supply an ID.
- De-duping prevents duplicates when users include your IDs manually.
108-116: Double-check: presence of IDs ≠ presence of DOM nodes.Even with this change, computedDescriptionId being truthy doesn’t guarantee a is rendered. If consumers don’t mount FormDescription, aria-describedby may still point to a non-existent node.
Options:
- Minimal: gate inclusion on fromPropsDesc only (as in the diff above), since it signals explicit intent from the caller.
- Robust: have FormDescription/FormMessage register their presence in context on mount/unmount (e.g., maintain a Set of described-by IDs). FormControl then derives the final list from that registry.
If you want, I can draft a lightweight presence-registration approach that doesn’t change the public API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
package.json(1 hunks)packages/components/package.json(1 hunks)packages/components/src/ui/form.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
{package.json,packages/**/package.json,apps/**/package.json}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo-organization.mdc)
Use consistent versioning across packages
Files:
packages/components/package.jsonpackage.json
{packages/**/package.json,apps/**/package.json}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo-organization.mdc)
Define peerDependencies, dependencies, and devDependencies appropriately in package.json
Files:
packages/components/package.json
packages/components/src/ui/**/*.tsx
📄 CodeRabbit Inference Engine (.cursor/rules/form-component-patterns.mdc)
packages/components/src/ui/**/*.tsx: Build on @radix-ui components as the foundation for base UI components
Follow the component composition pattern for UI components that accept form integration
Files:
packages/components/src/ui/form.tsx
packages/components/src/ui/*.{tsx,ts}
📄 CodeRabbit Inference Engine (.cursor/rules/form-component-patterns.mdc)
Base UI components should be named as ComponentName in ui/ directory
Files:
packages/components/src/ui/form.tsx
**/*.{tsx,ts}
📄 CodeRabbit Inference Engine (.cursor/rules/form-component-patterns.mdc)
**/*.{tsx,ts}: Props interfaces should be named as ComponentNameProps
Form schemas should be named formSchema or componentNameSchema
Files:
packages/components/src/ui/form.tsx
packages/components/src/{remix-hook-form,ui}/*.{tsx,ts}
📄 CodeRabbit Inference Engine (.cursor/rules/form-component-patterns.mdc)
Always export both the component and its props type
Files:
packages/components/src/ui/form.tsx
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo-organization.mdc)
{apps,packages}/**/*.{ts,tsx}: Use package name imports for published packages (e.g., import { TextField } from '@lambdacurry/forms/remix-hook-form')
Import from specific entry points (e.g., import { TextField } from '@lambdacurry/forms/remix-hook-form/text-field')
Do not use relative imports across packages (e.g., avoid import { TextField } from '../../packages/components/src/remix-hook-form/text-field')
Order imports: 1) external libraries, 2) internal package imports, 3) cross-package imports, 4) type-only imports (grouped separately)
Files:
packages/components/src/ui/form.tsx
{apps,packages}/**/src/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo-organization.mdc)
{apps,packages}/**/src/**/*.{ts,tsx}: Use relative imports within the same package (e.g., import { FormControl } from './form')
Use relative imports for sibling directories (e.g., import { Button } from '../ui/button')
Files:
packages/components/src/ui/form.tsx
packages/components/src/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo-organization.mdc)
packages/components/src/**/*.{ts,tsx}: Always export both component and props type (e.g., export { ComponentName }; export type { ComponentNameProps };)
Use named exports for components for better tree-shaking (e.g., export const ComponentName = ...; avoid default exports)
Avoid default exports for components
Use tree-shaking friendly exports
Files:
packages/components/src/ui/form.tsx
{apps,packages}/**/src/**/*.{tsx,ts}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo-organization.mdc)
{apps,packages}/**/src/**/*.{tsx,ts}: Use kebab-case for component file names (e.g., text-field.tsx, data-table.tsx)
Match component name in PascalCase to the exported component from the file (e.g., text-field.tsx exports TextField)
Files:
packages/components/src/ui/form.tsx
packages/components/src/ui/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo-organization.mdc)
Don't import from remix-hook-form package in ui components
Files:
packages/components/src/ui/form.tsx
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo-organization.mdc)
Avoid circular dependencies
Files:
packages/components/src/ui/form.tsx
🔇 Additional comments (2)
packages/components/package.json (1)
3-3: Version bump to 0.19.5 looks good and aligns with a patch release.No other changes in this file; publishing metadata remains intact.
package.json (1)
5-8: Workspace formatting change is benign and improves readability.No behavioral impact. Monorepo globs remain the same.
1 participant
Description
This PR fixes an issue with the
FormControlcomponent's handling of thearia-describedbyattribute. The previous implementation always set thearia-describedbyattribute to the description ID, even when there was no description. This could lead to invalid ARIA references.Changes
FormControlcomponent to only include thearia-describedbyattribute when there's an actual description or error messageTesting
This is a patch release (v0.19.5) that improves accessibility for all form components that use the
FormControlcomponent, including the Select component.Summary by CodeRabbit