Fix theming, accessibility, and code quality across 18 components#5
Fix theming, accessibility, and code quality across 18 components#5
Conversation
Theming: - Replace hardcoded gray/white/zinc Tailwind classes with CSS variable references in Search, Separator, Collapsible, Card, Tabs, Navigation - Replace 30+ hardcoded red-*/slate-* classes in ThemeToggle with --fever-primary/--fever-primary-dark/--fever-primary-light variables - Replace hardcoded hex colors in PasswordStrength with CSS variables - Replace hardcoded rgba(239,68,68) inline box-shadows with color-mix() using --fever-primary for proper theming Accessibility: - Add aria-describedby linking helper/error text to Input and Textarea - Add aria-invalid attribute for error state in Input and Textarea Type safety: - Replace 'as any' casts with properly typed React.ReactElement in Dialog (DialogTrigger, DialogClose), Drawer (DrawerClose), and Collapsible (CollapsibleTrigger) Code quality: - Remove dead duplicate 'if (!open) return null' in Select - Fix Rules of Hooks violation in Dropdown (useDropdownContext called inline in JSX instead of at component top level) - Replace deprecated substr() with slice() in FileUpload, Checkbox, Input, PasswordInput, Textarea - Remove console.error from CodeBlock clipboard error handler
Summary of ChangesHello @el-mehdi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on improving the theming, accessibility, and code quality of several components. It replaces hardcoded color classes with CSS variables for better theming, fixes accessibility issues in the Input and Textarea components, and resolves various code quality concerns across multiple files. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This is an excellent pull request that makes significant improvements across 18 components. The move to CSS variables for theming is a huge win for maintainability. The accessibility fixes for Input and Textarea are crucial, and the type-safety improvements by removing as any are great. The code quality fixes, such as resolving the Rules of Hooks violation and removing dead code, are also well-executed. I've identified a few areas for improvement, primarily related to using Math.random() for generating unique IDs, which can be problematic in React. I've also found a bug in the DialogClose component's asChild implementation that overwrites the child's onClick handler. Overall, this is a high-quality contribution.
| return React.cloneElement( | ||
| children as React.ReactElement<{ onClick?: React.MouseEventHandler }>, | ||
| { | ||
| onClick: handleClick, | ||
| } | ||
| ); |
There was a problem hiding this comment.
When using the asChild pattern, it's important to merge the component's event handlers with the child's existing handlers, rather than overwriting them. Here, the child's onClick handler is being replaced. This is a bug that will prevent any onClick on the child component from firing.
You should chain the onClick handlers, similar to how it's done in the DialogTrigger and DrawerClose components.
return React.cloneElement(
children as React.ReactElement<{ onClick?: React.MouseEventHandler<HTMLButtonElement> }>,
{
onClick: (e: React.MouseEvent<HTMLButtonElement>) => {
handleClick(e);
(children as React.ReactElement<{ onClick?: React.MouseEventHandler<HTMLButtonElement> }>).props.onClick?.(e);
},
}
);
| ...props | ||
| }, ref) => { | ||
| const checkboxId = id || `checkbox-${Math.random().toString(36).substr(2, 9)}`; | ||
| const checkboxId = id || `checkbox-${Math.random().toString(36).slice(2, 11)}`; |
There was a problem hiding this comment.
Using Math.random() to generate component IDs is an anti-pattern in React. It can lead to hydration mismatches in server-rendered applications and cause issues with accessibility and state. Please use the React.useId() hook instead for generating stable, unique IDs.
| const checkboxId = id || `checkbox-${Math.random().toString(36).slice(2, 11)}`; | |
| const checkboxId = id || React.useId(); |
|
|
||
| // Helper function to generate unique ID | ||
| const generateId = (): string => Math.random().toString(36).substr(2, 9); | ||
| const generateId = (): string => Math.random().toString(36).slice(2, 11); |
There was a problem hiding this comment.
Using Math.random() for generating keys is not recommended as it doesn't guarantee uniqueness, especially if many files are uploaded quickly. A more robust approach would be to use a simple counter to ensure unique IDs within the component's lifecycle. Since this function is at the module level, a module-level counter would be appropriate.
let fileIdCounter = 0;
const generateId = (): string => `file-${fileIdCounter++}`;This ensures unique IDs for each file added during the session.
| ...props | ||
| }, ref) => { | ||
| const inputId = id || `input-${Math.random().toString(36).substr(2, 9)}`; | ||
| const inputId = id || `input-${Math.random().toString(36).slice(2, 11)}`; |
There was a problem hiding this comment.
Using Math.random() to generate component IDs is an anti-pattern in React. It can lead to hydration mismatches in server-rendered applications and cause issues with accessibility and state. Please use the React.useId() hook instead for generating stable, unique IDs.
| const inputId = id || `input-${Math.random().toString(36).slice(2, 11)}`; | |
| const inputId = id || React.useId(); |
| }, ref) => { | ||
| const [showPassword, setShowPassword] = useState(false); | ||
| const inputId = id || `password-${Math.random().toString(36).substr(2, 9)}`; | ||
| const inputId = id || `password-${Math.random().toString(36).slice(2, 11)}`; |
There was a problem hiding this comment.
Using Math.random() to generate component IDs is an anti-pattern in React. It can lead to hydration mismatches in server-rendered applications and cause issues with accessibility and state. Please use the React.useId() hook instead for generating stable, unique IDs.
| const inputId = id || `password-${Math.random().toString(36).slice(2, 11)}`; | |
| const inputId = id || React.useId(); |
| ...props | ||
| }, ref) => { | ||
| const textareaId = id || `textarea-${Math.random().toString(36).substr(2, 9)}`; | ||
| const textareaId = id || `textarea-${Math.random().toString(36).slice(2, 11)}`; |
There was a problem hiding this comment.
Using Math.random() to generate component IDs is an anti-pattern in React. It can lead to hydration mismatches in server-rendered applications and cause issues with accessibility and state. Please use the React.useId() hook instead for generating stable, unique IDs.
| const textareaId = id || `textarea-${Math.random().toString(36).slice(2, 11)}`; | |
| const textareaId = id || React.useId(); |
- DialogClose: chain child's onClick with handleClick instead of overwriting it (matches DialogTrigger/DrawerClose/CollapsibleTrigger) - Replace Math.random() ID generation with React.useId() in Checkbox, Input, PasswordInput, Textarea for stable IDs across re-renders and SSR hydration compatibility - Replace Math.random() with counter-based ID in FileUpload for guaranteed uniqueness
Summary
aria-describedbyandaria-invalidto Input/Textarea for screen reader supportsubstr()in 5 files,as anycasts in Dialog/Drawer/CollapsibleChanges by Category
Theming (12 components)
red-*/slate-*classes →--fever-primaryCSS variables; 3 inlinergba(239,68,68)box-shadows →color-mix()bg-white,focus:bg-white→bg-[var(--fever-background)];hover:bg-gray-100→ CSS varbg-gray-200→bg-[var(--fever-border)]border-gray-200,bg-gray-50,hover:bg-gray-100→ CSS variablestext-gray-600→text-[var(--fever-text-secondary)]bg-gray-100,border-gray-200, inactive text/hover grays → CSS variablestext-gray-400/hover:text-white/hover:bg-zinc-800→ CSS variablesAccessibility (2 components)
aria-describedbylinking helper text;aria-invalidfor error statearia-describedbylinking helper text;aria-invalidfor error stateType Safety (3 components)
as any→React.ReactElement<{ onClick?: React.MouseEventHandler }>as any→ properly typedReact.ReactElementas any→ properly typedReact.ReactElementCode Quality (5 components)
if (!open) return nulluseDropdownContext()called inline in JSX)console.errorfrom clipboard error handlersubstr()→slice()(deprecated API)Test Plan
pnpm build)