refactor: replace old Checkbox component and Added new TextInput, MaskedInput, and Select components#8214
Open
abhishek-bruno wants to merge 2 commits into
Open
Conversation
…kedInput, and Select components - Deleted the old Checkbox and StyledWrapper components from the components directory. - Introduced a new Checkbox component with enhanced features including indeterminate state and customizable icons. - Added a new StyledWrapper for the Checkbox to manage styles and layout. - Created InputWrapper and StyledWrapper components for consistent form field handling. - Added new TextInput, MaskedInput, and Select components with their respective styles for improved UI consistency.
…accessibility improvements - Added useId for unique ID generation in Checkbox and Select components, improving accessibility. - Updated Checkbox to use generated IDs for aria attributes and labels. - Refactored InputWrapper to accept and apply generated IDs for label, description, and error elements. - Modified Select component to include aria attributes for better screen reader support. - Adjusted StyledWrapper imports to reference constants for consistent styling across components.
abhishek-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno and
sid-bruno
as code owners
Contributor
WalkthroughModernizes form inputs by introducing a new UI component library with InputWrapper infrastructure, TextInput, Checkbox, MaskedInput, and Select dropdown. The legacy Checkbox in ChangesForm Components Library
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/bruno-app/src/ui/Checkbox/index.js (1)
66-71: 💤 Low valueOptional: Simplify click handling pattern.
The current implementation uses a custom click handler on the wrapper that programmatically clicks the input, while also stopping propagation on both the wrapper and input. This is more complex than needed.
Consider using a native
<label>element wrapping the input, which provides built-in click-to-focus behavior without custom handlers or stopPropagation.♻️ Simplified alternative
return ( <StyledWrapper + as="label" + htmlFor={inputId} className={className} $size={size} $disabled={disabled} $labelPosition={labelPosition} $color={color} $iconColor={iconColor} $radius={radius} - onClick={handleClick} > <div className="checkbox-box"> <input ref={inputRef} type="checkbox" className={`checkbox-input ${indeterminate ? 'checkbox-indeterminate' : ''}`} id={inputId} name={name} data-testid={testId} value={value} checked={checked} defaultChecked={defaultChecked} disabled={disabled} onChange={onChange} aria-labelledby={labelId} aria-describedby={describedBy} - onClick={(e) => e.stopPropagation()} /> <span className="checkbox-icon">{checkedIcon}</span> </div> {/* ... label content ... */} </StyledWrapper> );Also applies to: 88-88, 105-105
🤖 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/bruno-app/src/ui/Checkbox/index.js` around lines 66 - 71, The wrapper's custom click handler (handleClick) and stopPropagation logic is unnecessary—replace the clickable wrapper element with a native <label> that wraps the input (using inputRef or the input's id) so browser-built click-to-toggle behavior handles focus/clicks, remove handleClick, the manual inputRef.current.click() call, and any stopPropagation on the input/wrapper; ensure the Checkbox component still respects the disabled prop and remove the redundant handlers at the referenced locations (handleClick, inputRef usage, and the stopPropagation calls).packages/bruno-app/src/ui/Select/StyledWrapper.js (1)
13-31: 💤 Low valueConsider extracting shared wrapper styles.
The base wrapper styles (padding, font-size, border-radius, focus/error/disabled states) are duplicated across TextInput (lines 5-25), MaskedInput (lines 5-25), and Select (lines 13-31). Per coding guidelines, abstractions are recommended at 3+ occurrences—you're exactly at that threshold.
However, each component uses different class names (
.text-input-wrapper,.masked-input-wrapper,.select-trigger) and has unique child elements, so extracting a shared CSS mixin might not provide significant value. Consider this if you add a 4th input component.🤖 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/bruno-app/src/ui/Select/StyledWrapper.js` around lines 13 - 31, The shared base wrapper styles (padding, font-size, border-radius, disabled/focus states) are duplicated across TextInput, MaskedInput and Select (.text-input-wrapper, .masked-input-wrapper, .select-trigger) — extract those common rules into a single reusable style/mixin (e.g., inputWrapperStyles or a styled BaseInputWrapper) that uses INPUT_SIZES and theme lookups, then replace the duplicated blocks in StyledWrapper.js and the other input Styled files to extend or compose that mixin; keep unique class-specific children/structure in each component and only move the truly shared properties (padding, font-size, border-radius, .disabled, .select-open / focus/error border handling) into the shared style to avoid changing class names or child selectors.Source: Coding guidelines
🤖 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.
Inline comments:
In `@packages/bruno-app/src/ui/Checkbox/index.js`:
- Around line 109-114: The Checkbox component currently renders label,
description, and error inline instead of using the shared InputWrapper; update
the Checkbox render to wrap the checkbox input with InputWrapper and pass the
generated IDs (labelId, descId, errId) along with label, description, and error
props to InputWrapper so it handles those elements consistently with
TextInput/MaskedInput/Select; ensure the checkbox input remains accessible
(aria-labelledby/aria-describedby) and adjust any className/layout props or
InputWrapper usage to preserve the checkbox-specific layout.
- Around line 99-100: The Checkbox component is currently passing both checked
and defaultChecked causing mixed controlled/uncontrolled behavior; update the
render logic in the Checkbox component to only pass defaultChecked when the
checked prop is undefined (uncontrolled mode) and pass checked (and
require/expect onChange) when checked is provided (controlled mode), i.e.,
conditionally include either checked or defaultChecked in the input props based
on whether props.checked is defined; ensure prop usage in the component
(checked, defaultChecked, onChange) aligns with this conditional mode.
In `@packages/bruno-app/src/ui/MaskedInput/index.js`:
- Around line 70-100: MaskedInput is missing accessibility attributes: import
useId, generate unique description and error IDs (e.g., descriptionId, errorId)
inside the MaskedInput component, pass those IDs into InputWrapper (so it can
render aria targets), and add aria-describedby on the input combining the
descriptionId and errorId when present and aria-invalid={!!error} on the
<input>; keep using the existing id prop for htmlFor, and update the input
element in MaskedInput to include aria-describedby and aria-invalid so screen
readers can associate the field with its description and error state.
In `@packages/bruno-app/src/ui/TextInput/index.js`:
- Around line 81-100: The TextInput component's <input> is not exposing
aria-describedby and aria-invalid to connect it with InputWrapper's
description/error nodes; use React's useId inside the TextInput component to
generate stable IDs (e.g. descriptionId and errorId), pass them into
InputWrapper (the same way Select does) and add aria-describedby on the <input>
pointing to the description/error id(s) and aria-invalid={!!error} on the input;
ensure you import useId, generate IDs near the component start, pass the IDs
into InputWrapper props, and set the input attributes (aria-describedby and
aria-invalid) on the input element to wire accessibility properly.
---
Nitpick comments:
In `@packages/bruno-app/src/ui/Checkbox/index.js`:
- Around line 66-71: The wrapper's custom click handler (handleClick) and
stopPropagation logic is unnecessary—replace the clickable wrapper element with
a native <label> that wraps the input (using inputRef or the input's id) so
browser-built click-to-toggle behavior handles focus/clicks, remove handleClick,
the manual inputRef.current.click() call, and any stopPropagation on the
input/wrapper; ensure the Checkbox component still respects the disabled prop
and remove the redundant handlers at the referenced locations (handleClick,
inputRef usage, and the stopPropagation calls).
In `@packages/bruno-app/src/ui/Select/StyledWrapper.js`:
- Around line 13-31: The shared base wrapper styles (padding, font-size,
border-radius, disabled/focus states) are duplicated across TextInput,
MaskedInput and Select (.text-input-wrapper, .masked-input-wrapper,
.select-trigger) — extract those common rules into a single reusable style/mixin
(e.g., inputWrapperStyles or a styled BaseInputWrapper) that uses INPUT_SIZES
and theme lookups, then replace the duplicated blocks in StyledWrapper.js and
the other input Styled files to extend or compose that mixin; keep unique
class-specific children/structure in each component and only move the truly
shared properties (padding, font-size, border-radius, .disabled, .select-open /
focus/error border handling) into the shared style to avoid changing class names
or child selectors.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 10df57eb-d5a0-4acb-b8a4-2216d165b564
📒 Files selected for processing (14)
packages/bruno-app/src/components/Checkbox/StyledWrapper.jspackages/bruno-app/src/components/Checkbox/index.jspackages/bruno-app/src/ui/ActionIcon/StyledWrapper.jspackages/bruno-app/src/ui/Checkbox/StyledWrapper.jspackages/bruno-app/src/ui/Checkbox/index.jspackages/bruno-app/src/ui/InputWrapper/StyledWrapper.jspackages/bruno-app/src/ui/InputWrapper/constants.jspackages/bruno-app/src/ui/InputWrapper/index.jspackages/bruno-app/src/ui/MaskedInput/StyledWrapper.jspackages/bruno-app/src/ui/MaskedInput/index.jspackages/bruno-app/src/ui/Select/StyledWrapper.jspackages/bruno-app/src/ui/Select/index.jspackages/bruno-app/src/ui/TextInput/StyledWrapper.jspackages/bruno-app/src/ui/TextInput/index.js
💤 Files with no reviewable changes (2)
- packages/bruno-app/src/components/Checkbox/StyledWrapper.js
- packages/bruno-app/src/components/Checkbox/index.js
Comment on lines
+99
to
+100
| checked={checked} | ||
| defaultChecked={defaultChecked} |
Contributor
There was a problem hiding this comment.
Critical: Remove mixed controlled/uncontrolled state.
React will issue warnings when both checked and defaultChecked are provided. The component must be either fully controlled (using checked + onChange) or fully uncontrolled (using defaultChecked only).
As per coding guidelines: "Avoid mixed controlled and uncontrolled state in React components. A component is either controlled or uncontrolled."
🔧 Recommended fix
Choose controlled mode when checked is provided:
<input
ref={inputRef}
type="checkbox"
className={`checkbox-input ${indeterminate ? 'checkbox-indeterminate' : ''}`}
id={inputId}
name={name}
data-testid={testId}
value={value}
- checked={checked}
- defaultChecked={defaultChecked}
+ {...(checked !== undefined ? { checked } : { defaultChecked })}
disabled={disabled}
onChange={onChange}
aria-labelledby={labelId}
aria-describedby={describedBy}
onClick={(e) => e.stopPropagation()}
/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| checked={checked} | |
| defaultChecked={defaultChecked} | |
| {...(checked !== undefined ? { checked } : { defaultChecked })} |
🤖 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/bruno-app/src/ui/Checkbox/index.js` around lines 99 - 100, The
Checkbox component is currently passing both checked and defaultChecked causing
mixed controlled/uncontrolled behavior; update the render logic in the Checkbox
component to only pass defaultChecked when the checked prop is undefined
(uncontrolled mode) and pass checked (and require/expect onChange) when checked
is provided (controlled mode), i.e., conditionally include either checked or
defaultChecked in the input props based on whether props.checked is defined;
ensure prop usage in the component (checked, defaultChecked, onChange) aligns
with this conditional mode.
Source: Coding guidelines
Comment on lines
+109
to
+114
| {(label || description || error) && ( | ||
| <div className="checkbox-label-content"> | ||
| {label && <span id={labelId} className="checkbox-label">{label}</span>} | ||
| {description && <span id={descId} className="checkbox-description">{description}</span>} | ||
| {error && <span id={errId} className="checkbox-error">{error}</span>} | ||
| </div> |
Contributor
There was a problem hiding this comment.
Major: Use InputWrapper for consistency with other form components.
TextInput, MaskedInput, and Select all use the shared InputWrapper component to render label, description, and error content. This Checkbox component bypasses that pattern and renders them inline, creating API and styling inconsistencies across the form component library.
Refactor to wrap the checkbox with InputWrapper and pass label, description, error, and the generated IDs (labelId, descriptionId, errorId) to it, matching the pattern shown in the relevant code snippets from Select.
♻️ Suggested refactor to use InputWrapper
+import InputWrapper from '../InputWrapper';
+
const Checkbox = ({
checked,
onChange,
// ... other props
}) => {
const inputRef = useRef(null);
const autoId = useId();
const inputId = id || autoId;
+ const labelId = label ? `${inputId}-label` : undefined;
+ const descriptionId = description ? `${inputId}-desc` : undefined;
+ const errorId = error ? `${inputId}-err` : undefined;
- const labelId = label ? `${inputId}-label` : undefined;
- const descId = description ? `${inputId}-desc` : undefined;
- const errId = error ? `${inputId}-err` : undefined;
- const describedBy = [descId, errId].filter(Boolean).join(' ') || undefined;
+ const describedBy = [descriptionId, errorId].filter(Boolean).join(' ') || undefined;
// ... rest of logic
return (
+ <InputWrapper
+ label={label}
+ description={description}
+ error={error}
+ size={size}
+ labelId={labelId}
+ descriptionId={descriptionId}
+ errorId={errorId}
+ >
<StyledWrapper
className={className}
$size={size}
$disabled={disabled}
$labelPosition={labelPosition}
$color={color}
$iconColor={iconColor}
$radius={radius}
onClick={handleClick}
>
<div className="checkbox-box">
<input
ref={inputRef}
type="checkbox"
className={`checkbox-input ${indeterminate ? 'checkbox-indeterminate' : ''}`}
id={inputId}
name={name}
data-testid={testId}
value={value}
checked={checked}
defaultChecked={defaultChecked}
disabled={disabled}
onChange={onChange}
aria-labelledby={labelId}
aria-describedby={describedBy}
onClick={(e) => e.stopPropagation()}
/>
<span className="checkbox-icon">{checkedIcon}</span>
</div>
- {(label || description || error) && (
- <div className="checkbox-label-content">
- {label && <span id={labelId} className="checkbox-label">{label}</span>}
- {description && <span id={descId} className="checkbox-description">{description}</span>}
- {error && <span id={errId} className="checkbox-error">{error}</span>}
- </div>
- )}
</StyledWrapper>
+ </InputWrapper>
);
};Note: This may require adjustments to InputWrapper to support the checkbox's unique layout requirements (e.g., label positioning).
🤖 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/bruno-app/src/ui/Checkbox/index.js` around lines 109 - 114, The
Checkbox component currently renders label, description, and error inline
instead of using the shared InputWrapper; update the Checkbox render to wrap the
checkbox input with InputWrapper and pass the generated IDs (labelId, descId,
errId) along with label, description, and error props to InputWrapper so it
handles those elements consistently with TextInput/MaskedInput/Select; ensure
the checkbox input remains accessible (aria-labelledby/aria-describedby) and
adjust any className/layout props or InputWrapper usage to preserve the
checkbox-specific layout.
Comment on lines
+70
to
+100
| return ( | ||
| <InputWrapper label={label} description={description} error={error} htmlFor={id} required={required} size={size} className={className}> | ||
| <StyledWrapper $size={size}> | ||
| <div className={wrapperClasses}> | ||
| {leftSection && <span className="masked-input-left-section">{leftSection}</span>} | ||
| <input | ||
| id={id} | ||
| type={isVisible ? 'text' : 'password'} | ||
| name={name || id} | ||
| className="masked-input-field" | ||
| value={value} | ||
| onChange={onChange} | ||
| data-testid={testId} | ||
| placeholder={placeholder} | ||
| disabled={disabled} | ||
| autoComplete="off" | ||
| autoCorrect="off" | ||
| autoCapitalize="off" | ||
| spellCheck="false" | ||
| onFocus={() => setIsFocused(true)} | ||
| onBlur={() => setIsFocused(false)} | ||
| /> | ||
| {!disabled && ( | ||
| <button type="button" className="masked-input-toggle" onClick={handleToggle} aria-label={isVisible ? 'Hide value' : 'Show value'}> | ||
| {isVisible ? <IconEyeOff size={16} strokeWidth={2} /> : <IconEye size={16} strokeWidth={2} />} | ||
| </button> | ||
| )} | ||
| {rightSection && <span className="masked-input-right-section">{rightSection}</span>} | ||
| </div> | ||
| </StyledWrapper> | ||
| </InputWrapper> |
Contributor
There was a problem hiding this comment.
Add aria-describedby and aria-invalid for accessibility.
Like TextInput, this input should link to description/error via aria-describedby and signal error state with aria-invalid. Follow the pattern used in Select (lines 92-96, 308).
♿ Proposed fix to add accessibility attributes
After line 45, generate IDs:
const [isFocused, setIsFocused] = useState(false);
+
+const autoId = useId();
+const labelId = label ? `${autoId}-label` : undefined;
+const descriptionId = description ? `${autoId}-desc` : undefined;
+const errorId = error ? `${autoId}-err` : undefined;
+const describedBy = [descriptionId, errorId].filter(Boolean).join(' ') || undefined;Import useId in line 1:
-import React, { useState } from 'react';
+import React, { useState, useId } from 'react';Pass IDs to InputWrapper (line 71):
-<InputWrapper label={label} description={description} error={error} htmlFor={id} required={required} size={size} className={className}>
+<InputWrapper label={label} description={description} error={error} htmlFor={id} required={required} size={size} className={className} labelId={labelId} descriptionId={descriptionId} errorId={errorId}>Add aria attributes to input (after line 84):
disabled={disabled}
+ aria-invalid={error ? 'true' : undefined}
+ aria-describedby={describedBy}
autoComplete="off"🤖 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/bruno-app/src/ui/MaskedInput/index.js` around lines 70 - 100,
MaskedInput is missing accessibility attributes: import useId, generate unique
description and error IDs (e.g., descriptionId, errorId) inside the MaskedInput
component, pass those IDs into InputWrapper (so it can render aria targets), and
add aria-describedby on the input combining the descriptionId and errorId when
present and aria-invalid={!!error} on the <input>; keep using the existing id
prop for htmlFor, and update the input element in MaskedInput to include
aria-describedby and aria-invalid so screen readers can associate the field with
its description and error state.
Comment on lines
+81
to
+100
| <input | ||
| id={id} | ||
| type={type} | ||
| name={name || id} | ||
| className="text-input-field" | ||
| value={value} | ||
| onChange={onChange} | ||
| placeholder={placeholder} | ||
| disabled={disabled} | ||
| data-testid={testId} | ||
| readOnly={readOnly} | ||
| autoFocus={autoFocus} | ||
| autoComplete={autoComplete} | ||
| maxLength={maxLength} | ||
| min={min} | ||
| max={max} | ||
| step={step} | ||
| onFocus={() => setIsFocused(true)} | ||
| onBlur={() => setIsFocused(false)} | ||
| /> |
Contributor
There was a problem hiding this comment.
Add aria-describedby and aria-invalid for accessibility.
The input should link to description/error elements via aria-describedby and signal error state with aria-invalid. Currently, InputWrapper renders description/error but they're not connected to the input.
See how Select (lines 92-96, 308) generates IDs with useId and passes them to both InputWrapper and the trigger element. Apply the same pattern here.
♿ Proposed fix to add accessibility attributes
At the top of the component, after line 56:
const [isFocused, setIsFocused] = useState(false);
+
+const autoId = useId();
+const labelId = label ? `${autoId}-label` : undefined;
+const descriptionId = description ? `${autoId}-desc` : undefined;
+const errorId = error ? `${autoId}-err` : undefined;
+const describedBy = [descriptionId, errorId].filter(Boolean).join(' ') || undefined;Import useId in line 1:
-import React, { useState } from 'react';
+import React, { useState, useId } from 'react';Pass IDs to InputWrapper (line 69):
<InputWrapper
label={label}
description={description}
error={error}
htmlFor={id}
required={required}
size={size}
className={className}
+ labelId={labelId}
+ descriptionId={descriptionId}
+ errorId={errorId}
>Add aria attributes to input (after line 89):
disabled={disabled}
data-testid={testId}
+ aria-invalid={error ? 'true' : undefined}
+ aria-describedby={describedBy}
readOnly={readOnly}🤖 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/bruno-app/src/ui/TextInput/index.js` around lines 81 - 100, The
TextInput component's <input> is not exposing aria-describedby and aria-invalid
to connect it with InputWrapper's description/error nodes; use React's useId
inside the TextInput component to generate stable IDs (e.g. descriptionId and
errorId), pass them into InputWrapper (the same way Select does) and add
aria-describedby on the <input> pointing to the description/error id(s) and
aria-invalid={!!error} on the input; ensure you import useId, generate IDs near
the component start, pass the IDs into InputWrapper props, and set the input
attributes (aria-describedby and aria-invalid) on the input element to wire
accessibility properly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
JIRA
Old PR for reference
Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.