Conversation
📝 WalkthroughWalkthroughSGLButton's props were refactored: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 (3)
src/components/UI/Button/SGLButton.tsx (2)
5-10: Use camelCase for constant name.
variantcolorshould follow JavaScript/TypeScript camelCase convention.Proposed fix
-const variantcolor = { +const variantColor = { white: 'background.paper', black: 'black', purple: 'purple.main', orange: 'orange.main', } as constNote: If renamed, update usages on lines 34, 35, 38, and 39.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/UI/Button/SGLButton.tsx` around lines 5 - 10, Rename the constant variantcolor to follow camelCase (e.g., variantColor) and update all references inside SGLButton to use the new name; specifically change the declaration "variantcolor" to "variantColor" and update any usages where the object is accessed (the places currently referencing variantcolor in the SGLButton component) so imports and lookups remain consistent.
16-17: Consider clearer prop naming forcolor.Since
colorcontrolsbackgroundColorfor contained variant andborderColorfor outlined variant, naming itbgColororbaseColormight be clearer and reduce confusion with MUI'scolorprop semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/UI/Button/SGLButton.tsx` around lines 16 - 17, Rename the prop `color` on the SGLButton component to a clearer name like `bgColor` or `baseColor` and update all usages and internal references where `color` controls backgroundColor for the contained variant and borderColor for the outlined variant (look for the SGLButton component and its props `color` and `textColor`), including prop types, destructuring, and any places that map that value to style/class logic so MUI's `color` prop semantics are not confused; keep `textColor` as-is or rename consistently if desired and ensure external consumers are updated or a deprecation alias (accepting both `color` and the new name) is added temporarily to avoid breaking changes.src/components/UI/Button/styles.ts (1)
3-3: Inconsistent palette color notation.The codebase convention uses dot notation for palette colors (e.g.,
'primary.main'), as seen insrc/components/UI/Checkbox/styles.ts. Using just'primary'deviates from this pattern and may behave differently in MUI's sx system.Proposed fix
export const buttonStyles = { borderRadius: '50px', - color: 'primary', + color: 'primary.main', }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/UI/Button/styles.ts` at line 3, The Button styles currently use color: 'primary' which is inconsistent with the codebase palette dot-notation (e.g., 'primary.main'); update the Button styles (the color property in src/components/UI/Button/styles.ts) to use the full palette key 'primary.main' so it matches Checkbox/styles.ts and MUI's sx/palette expectations, and run a quick UI check to confirm no visual regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/UI/Button/SGLButton.tsx`:
- Around line 33-40: SGLButton’s style block only handles 'contained' and
'outlined', so add handling for the 'text' variant to apply the text color (use
variantcolor[textColor] or variantcolor[color] as appropriate) and add '&:hover'
rules for each variant to override MUI defaults (for 'contained' adjust
backgroundColor on hover, for 'outlined' adjust borderColor/backgroundColor, and
for 'text' adjust color/hover background if needed). Locate the style object
where variant, variantcolor, color, and textColor are used and add the 'text'
branch plus '&:hover' entries for consistency across variants.
---
Nitpick comments:
In `@src/components/UI/Button/SGLButton.tsx`:
- Around line 5-10: Rename the constant variantcolor to follow camelCase (e.g.,
variantColor) and update all references inside SGLButton to use the new name;
specifically change the declaration "variantcolor" to "variantColor" and update
any usages where the object is accessed (the places currently referencing
variantcolor in the SGLButton component) so imports and lookups remain
consistent.
- Around line 16-17: Rename the prop `color` on the SGLButton component to a
clearer name like `bgColor` or `baseColor` and update all usages and internal
references where `color` controls backgroundColor for the contained variant and
borderColor for the outlined variant (look for the SGLButton component and its
props `color` and `textColor`), including prop types, destructuring, and any
places that map that value to style/class logic so MUI's `color` prop semantics
are not confused; keep `textColor` as-is or rename consistently if desired and
ensure external consumers are updated or a deprecation alias (accepting both
`color` and the new name) is added temporarily to avoid breaking changes.
In `@src/components/UI/Button/styles.ts`:
- Line 3: The Button styles currently use color: 'primary' which is inconsistent
with the codebase palette dot-notation (e.g., 'primary.main'); update the Button
styles (the color property in src/components/UI/Button/styles.ts) to use the
full palette key 'primary.main' so it matches Checkbox/styles.ts and MUI's
sx/palette expectations, and run a quick UI check to confirm no visual
regressions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3bfe8f00-7b3b-44b6-b752-cc600fad1125
📒 Files selected for processing (2)
src/components/UI/Button/SGLButton.tsxsrc/components/UI/Button/styles.ts
| ...(variant === 'contained' && { | ||
| backgroundColor: variantcolor[color], | ||
| color: variantcolor[textColor], | ||
| }), | ||
| ...(variant === 'outlined' && { | ||
| borderColor: variantcolor[color], | ||
| color: variantcolor[textColor], | ||
| }), |
There was a problem hiding this comment.
Missing text variant handling and hover states.
The text variant won't receive color/textColor styling since only contained and outlined are handled. Additionally, custom backgroundColor on contained variant will have default MUI hover states that may look inconsistent.
Consider:
- Adding
textvariant handling (at minimum for text color) - Adding
'&:hover'styles to maintain visual consistency
Example addition for text variant and hover
...(variant === 'outlined' && {
borderColor: variantcolor[color],
color: variantcolor[textColor],
}),
+ ...(variant === 'text' && {
+ color: variantcolor[color],
+ }),
+ ...(variant === 'contained' && {
+ '&:hover': {
+ backgroundColor: variantcolor[color],
+ opacity: 0.9,
+ },
+ }),
...style,📝 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.
| ...(variant === 'contained' && { | |
| backgroundColor: variantcolor[color], | |
| color: variantcolor[textColor], | |
| }), | |
| ...(variant === 'outlined' && { | |
| borderColor: variantcolor[color], | |
| color: variantcolor[textColor], | |
| }), | |
| ...(variant === 'contained' && { | |
| backgroundColor: variantcolor[color], | |
| color: variantcolor[textColor], | |
| }), | |
| ...(variant === 'outlined' && { | |
| borderColor: variantcolor[color], | |
| color: variantcolor[textColor], | |
| }), | |
| ...(variant === 'text' && { | |
| color: variantcolor[color], | |
| }), | |
| ...(variant === 'contained' && { | |
| '&:hover': { | |
| backgroundColor: variantcolor[color], | |
| opacity: 0.9, | |
| }, | |
| }), | |
| ...style, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/UI/Button/SGLButton.tsx` around lines 33 - 40, SGLButton’s
style block only handles 'contained' and 'outlined', so add handling for the
'text' variant to apply the text color (use variantcolor[textColor] or
variantcolor[color] as appropriate) and add '&:hover' rules for each variant to
override MUI defaults (for 'contained' adjust backgroundColor on hover, for
'outlined' adjust borderColor/backgroundColor, and for 'text' adjust color/hover
background if needed). Locate the style object where variant, variantcolor,
color, and textColor are used and add the 'text' branch plus '&:hover' entries
for consistency across variants.
Tamir198
left a comment
Tamir198
left a comment
There was a problem hiding this comment.
In order to show text on the button we must pass the text prop, lets change it to children so that we can render this as a normal button, because right now text only accept strings which is not dynamic.
Also please check how the button is being rendered because on some variant the text is invisible :
| sx={{ | ||
| ...buttonStyles, | ||
| ...(variant === 'contained' && { | ||
| backgroundColor: variantcolor[color], |
There was a problem hiding this comment.
We dont want hardcoded values, the styling should occur inside styles.ts file
|
|
||
| interface ButtonProps extends Omit<MuiButtonProps, 'onClick' | 'style'> { | ||
| const variantcolor = { | ||
| white: 'background.paper', |
There was a problem hiding this comment.
Much better, lets remove everything style related into the styles.ts file, for reference look at SGLCard component
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/components/UI/Button/styles.ts (2)
3-8: Consider exporting the color type unions for reuse.The color type unions are duplicated between
styles.tsandSGLButton.tsx. Extracting them into shared types would improve maintainability and ensure consistency.Optional: Extract shared types
+export type ButtonColor = 'primary' | 'secondary' | 'error' | 'purple' | 'orange' +export type ButtonTextColor = 'primary' | 'secondary' | 'error' | 'purple' | 'orange' | 'lightGrey' + export const buttonStyles = ( theme: Theme, variant?: 'contained' | 'outlined', - color: 'primary' | 'secondary' | 'error' | 'purple' | 'orange' = 'secondary', - textColor: 'primary' | 'secondary' | 'error' | 'purple' | 'orange' | 'lightGrey' = 'primary', + color: ButtonColor = 'secondary', + textColor: ButtonTextColor = 'primary', ): SxProps<Theme> => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/UI/Button/styles.ts` around lines 3 - 8, Extract the literal unions used in buttonStyles into shared exported types (e.g., export type ButtonColor = 'primary' | 'secondary' | 'error' | 'purple' | 'orange' and export type ButtonTextColor = ButtonColor | 'lightGrey') in a common types file (or export from styles.ts) and update the buttonStyles signature to use ButtonColor and ButtonTextColor; then import and use those same types in SGLButton.tsx so both files reference the single source of truth rather than duplicating the unions.
9-22: Missingtextvariant handling and hover states for custom backgrounds.The function only handles
containedandoutlinedvariants, returning an empty object fortext. This meanstextvariant buttons won't receive thecolor/textColorstyling. Additionally, when using custombackgroundColoroncontained, MUI's default hover states may look inconsistent with the custom colors.Consider adding:
- A
textvariant case to apply text color'&:hover'styles forcontainedto maintain visual consistency with the custom background colorSuggested enhancement
): SxProps<Theme> => { if (variant === 'contained') { return { backgroundColor: theme.palette[color].main, color: theme.palette[textColor].main, borderRadius: '30px', + '&:hover': { + backgroundColor: theme.palette[color].dark ?? theme.palette[color].main, + opacity: 0.9, + }, } } else if (variant === 'outlined') { return { borderColor: theme.palette[color].main, color: theme.palette[textColor].main, borderRadius: '30px', } + } else if (variant === 'text') { + return { + color: theme.palette[color].main, + } } return {} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/UI/Button/styles.ts` around lines 9 - 22, The style branch only covers 'contained' and 'outlined' so add a 'text' branch that applies color: theme.palette[textColor].main (so text buttons get themed color), and enhance the 'contained' branch to include a '&:hover' rule that uses theme.palette[color].dark (or a slightly darker/emphasized value) for backgroundColor and preserves the text color—update the same function handling variant/color/textColor in src/components/UI/Button/styles.ts to return these cases instead of an empty object.src/components/UI/Button/SGLButton.tsx (2)
10-10: Consider addingtextto the variant type if the component should support it.MUI Button supports
'contained' | 'outlined' | 'text'variants. Currently, the type restricts to only'contained' | 'outlined'. Iftextvariant is needed (as mentioned in past reviews), add it to both this type and handle it inbuttonStyles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/UI/Button/SGLButton.tsx` at line 10, The variant union for SGLButton is missing the 'text' option; update the prop type (the variant?: 'contained' | 'outlined') to include 'text' and ensure the styling logic in buttonStyles (used by SGLButton) handles the 'text' case (add styles or fallback behavior for 'text' so rendering matches MUI's text variant). Verify both the prop type and the branch in buttonStyles/SGLButton that applies styles for variants are updated consistently.
17-17: Remove commented-out code.The commented
// text,is a remnant from the previous API and should be removed to keep the code clean.Remove dead code
export const SGLButton = ({ children, - // text, onClick,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/UI/Button/SGLButton.tsx` at line 17, Remove the dead commented-out prop `// text,` from the SGLButton component in src/components/UI/Button/SGLButton.tsx; locate the SGLButton component and its props/interface declaration and delete that commented line so the file no longer contains the leftover commented code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/UI/Button/SGLButton.tsx`:
- Line 10: The variant union for SGLButton is missing the 'text' option; update
the prop type (the variant?: 'contained' | 'outlined') to include 'text' and
ensure the styling logic in buttonStyles (used by SGLButton) handles the 'text'
case (add styles or fallback behavior for 'text' so rendering matches MUI's text
variant). Verify both the prop type and the branch in buttonStyles/SGLButton
that applies styles for variants are updated consistently.
- Line 17: Remove the dead commented-out prop `// text,` from the SGLButton
component in src/components/UI/Button/SGLButton.tsx; locate the SGLButton
component and its props/interface declaration and delete that commented line so
the file no longer contains the leftover commented code.
In `@src/components/UI/Button/styles.ts`:
- Around line 3-8: Extract the literal unions used in buttonStyles into shared
exported types (e.g., export type ButtonColor = 'primary' | 'secondary' |
'error' | 'purple' | 'orange' and export type ButtonTextColor = ButtonColor |
'lightGrey') in a common types file (or export from styles.ts) and update the
buttonStyles signature to use ButtonColor and ButtonTextColor; then import and
use those same types in SGLButton.tsx so both files reference the single source
of truth rather than duplicating the unions.
- Around line 9-22: The style branch only covers 'contained' and 'outlined' so
add a 'text' branch that applies color: theme.palette[textColor].main (so text
buttons get themed color), and enhance the 'contained' branch to include a
'&:hover' rule that uses theme.palette[color].dark (or a slightly
darker/emphasized value) for backgroundColor and preserves the text color—update
the same function handling variant/color/textColor in
src/components/UI/Button/styles.ts to return these cases instead of an empty
object.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 519aea49-e510-4102-a7d2-7e6ccc80254d
📒 Files selected for processing (3)
src/App.tsxsrc/components/UI/Button/SGLButton.tsxsrc/components/UI/Button/styles.ts
|
@Tamir198 can you please review my PR? |
Tamir198
left a comment
Tamir198
left a comment
There was a problem hiding this comment.
The outlined variant need more dark text we can hardly see it:
| export const SGLButton = ({ | ||
| text, | ||
| children, | ||
| // text, |
| style?: SxProps<Theme> | ||
| variant?: 'contained' | 'outlined' | ||
| color?: 'primary' | 'secondary' | 'error' | 'purple' | 'orange' | ||
| textColor?: 'primary' | 'secondary' | 'error' | 'purple' | 'orange' | 'lightGrey' |
There was a problem hiding this comment.
Why do we want to use textColor prop? we can just past style object, same fore color prop
| interface ButtonProps extends Omit<MuiButtonProps, 'onClick' | 'style' | 'color'> { | ||
| children: ReactNode | ||
| onClick: () => void | ||
| style?: SxProps<Theme> |
There was a problem hiding this comment.
Can you change the style object into CSSProperties ? I will be a lot simpler to pass the style object like this
| <Button | ||
| sx={(theme) => ({ | ||
| ...buttonStyles(theme, variant, color, textColor), | ||
| ...(style || {}), |
There was a problem hiding this comment.
Also please have a look on how the select is built:
<Select
sx={selectStyles}And the styles object is:
export const selectStyles = (theme: Theme) => ({
backgroundColor: theme.palette.lightGrey.main,
'&:hover': {
backgroundColor: alpha(theme.palette.lightGrey.main, 0.8),
},
borderRadius: '5px',
'& .MuiSelect-select': {
textAlign: 'right',
paddingLeft: '24px',
},
'& .MuiSelect-icon': {
left: 0,
right: 'unset',
},
})This is cleaner
|
@rachelishteren Added comments |
2 participants
Implemented a reusable SGLButton component based on MUI.
Added support for different variant types (contained, outlined, text)
Enabled dynamic color customization via props (color, textColor)
Integrated MUI sx prop for flexible styling and overrides
Connected component to project theme (palette-based colors)
Summary by CodeRabbit
New Features
Improvements