border radius for button and textinput components #345
Conversation
WalkthroughAdds a border-radius design token set and applies it across button and text-input SCSS; exposes optional Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant UI as UI Component (Button/TextInput)
participant CSS as SCSS / Tailwind tokens
Dev->>UI: pass props (including optional borderRadius)
UI->>UI: compute style (merge borderRadius into props.style if present)
UI->>CSS: render element with inline style + class selectors
CSS-->>UI: class rules include border-radius token (theme(digitv2.borderRadius.radius0))
Note right of CSS `#EEE8D5`: Visual result = class token OR inline override
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
react/ui-components/src/atoms/TextInput.js (1)
237-237: LGTM! Consistent borderRadius implementation across all input variants.The borderRadius prop is correctly extracted and applied to inline styles in all three render branches (DatePicker, required input, and non-required input), ensuring consistent styling behavior. The PropTypes declaration properly documents the new prop.
Minor formatting suggestion:
Add a space after the comma in the style spread for better readability:
-style={{ borderRadius,...props.style }} +style={{ borderRadius, ...props.style }}This applies to lines 287, 347, and 431.
Also applies to: 287-287, 347-347, 431-431, 530-530
react/css/src/digitv2/components/textInputV2.scss (1)
13-13: Border-radius applied consistently; note pre-existing duplicate selectors.The border-radius property has been consistently applied to all relevant input selectors using
theme(digitv2.borderRadius.radius0).Observation: The file contains duplicate class definitions:
.digit-card-inputErrorappears at lines 4, 221, and 355.digit-employeeCard-inputErrorappears at lines 260 and 341While this duplication predates this PR, consider consolidating these duplicate selectors in a future refactor to improve maintainability and reduce the risk of inconsistent styling.
Note: Like the button components, this uses radius0 (0rem) for sharp corners. Verify this aligns with the design system requirements.
Also applies to: 230-230, 243-243, 270-270, 329-329, 352-352, 365-365
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
react/css/src/digitv2/components/buttonsV2.scss(5 hunks)react/css/src/digitv2/components/textInputV2.scss(7 hunks)react/css/tailwind.config.js(1 hunks)react/ui-components/src/atoms/Button.js(3 hunks)react/ui-components/src/atoms/TextInput.js(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-09-20T07:41:23.926Z
Learnt from: rachana-egov
Repo: egovernments/DIGIT-UI-LIBRARIES PR: 119
File: flutter/digit-ui-components/digit_components/lib/widgets/atoms/digit_search_bar.dart:9-89
Timestamp: 2024-09-20T07:41:23.926Z
Learning: In the `DigitSearchBar` component, the team prefers to use a `Widget` for the `icon` property to make it more customizable.
Applied to files:
react/css/src/digitv2/components/textInputV2.scss
🧬 Code graph analysis (2)
react/ui-components/src/atoms/TextInput.js (1)
react/ui-components/src/atoms/Button.js (1)
borderRadius(93-93)
react/ui-components/src/atoms/Button.js (1)
react/ui-components/src/atoms/TextInput.js (1)
borderRadius(237-237)
🔇 Additional comments (2)
react/css/src/digitv2/components/buttonsV2.scss (1)
56-56: Verify the choice of radius0 (0rem) for default border-radius.The border-radius is consistently applied across all button variants using
theme(digitv2.borderRadius.radius0), which equals"0rem"(sharp corners). While this maintains backwards compatibility, it may not align with modern UI expectations where buttons typically have slightly rounded corners.Consider whether radius0 is intentional or if a small radius like radius1 (0.25rem) or radius2 (0.375rem) would be more appropriate for the design system. You can verify this by checking the design specifications or consulting with the design team.
Also applies to: 73-73, 135-135, 205-205, 265-265
react/css/tailwind.config.js (1)
284-299: LGTM! Well-structured borderRadius design tokens.The borderRadius configuration is well-designed with a comprehensive range of options (radius0 through radius13) and follows the same pattern as the existing spacers configuration. The incremental scale from 0rem to 2.5rem provides good flexibility for different component styling needs.
Note: The radius0 value of "0rem" provides sharp corners, which is useful for backwards compatibility or specific design requirements. The other tokens offer a good range of modern rounded corner options.
- Fixed style prop to properly handle borderRadius as an object instead of string - Added conditional logic to only create style object when borderRadius is defined - Fixed spacing in object spread syntax for consistency - Applied fix to Button.js (1 location) and TextInput.js (3 locations)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
react/ui-components/src/atoms/Button.js(3 hunks)react/ui-components/src/atoms/TextInput.js(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- react/ui-components/src/atoms/Button.js
🧰 Additional context used
🧬 Code graph analysis (1)
react/ui-components/src/atoms/TextInput.js (1)
react/ui-components/src/atoms/Button.js (1)
borderRadius(93-93)
🔇 Additional comments (2)
react/ui-components/src/atoms/TextInput.js (2)
237-237: LGTM!Clean extraction pattern that mirrors the implementation in Button.js, maintaining consistency across components.
530-530: LGTM!Appropriate type definition. Using
stringallows flexible CSS units (px, rem, %, etc.) which is the right choice for a border-radius value.
| ? new Date(props?.populators?.max) | ||
| : undefined | ||
| } | ||
| style={props.style ? { borderRadius, ...props.style } : borderRadius ? { borderRadius } : undefined} |
There was a problem hiding this comment.
Fix style precedence: borderRadius prop should override style.borderRadius.
The current spread order allows props.style.borderRadius to override the explicit borderRadius prop, which is counterintuitive. When a developer passes a specific prop, it should take precedence over generic style object properties.
Apply this pattern instead:
-style={props.style ? { borderRadius, ...props.style } : borderRadius ? { borderRadius } : undefined}
+style={borderRadius ? { ...props.style, borderRadius } : props.style}This ensures:
- The
borderRadiusprop always takes precedence when provided - No unnecessary
borderRadius: undefinedin the style object props.styleis preserved when only style is provided- Cleaner conditional logic
Apply this change to all three occurrences (lines 287, 347, 431).
Also applies to: 347-347, 431-431
🤖 Prompt for AI Agents
In react/ui-components/src/atoms/TextInput.js around lines 287, 347, and 431,
the style spread currently places props.style after the explicit borderRadius
prop which lets props.style.borderRadius override the borderRadius prop; change
the merge order so the computed borderRadius value is applied last (i.e., spread
props.style first, then, if borderRadius is defined, set borderRadius) so the
borderRadius prop always wins, avoid adding borderRadius: undefined, and
preserve props.style when borderRadius is not provided.
| ? new Date(props?.populators?.max) | ||
| : undefined | ||
| } | ||
| style={props.style ? { borderRadius, ...props.style } : borderRadius ? { borderRadius } : undefined} |
There was a problem hiding this comment.
@akhil-egov
The style prop already supports setting any styling properties for the button. Introducing a separate prop specifically for borderRadius doesn’t make sense — otherwise, we’d have to create individual props for color, text color, size, and so on. It’s better to avoid adding this additional prop since your logic would still merge under style.
You can simply achieve the same result using:
<Button style={{ borderRadius: "0.5rem" }} />
2 participants
The Border Radius property has been added to Button and Text Input Components.