Issue #288 Moved mui-root stylings in various css files to src/assets… #354
Conversation
…to src/assets/theme.jsx
WalkthroughThe changes in this pull request focus on updating the theme configuration in Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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
🧹 Outside diff range and nitpick comments (1)
frontend/src/assets/theme.jsx (1)
258-275: Consolidate MuiTab Styles – One Shot to SimplifyYour
MuiTaboverrides in both the light and dark themes share the same rhythm. This is your one shot to extract these shared styles into a common place. Don't let this opportunity slip; unify them to keep your code tight and on beat.Consider creating a shared style object or a base theme to hold the common
MuiTabstyles, reducing repetition and making future changes smoother.Also applies to: 405-420
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
frontend/src/assets/theme.jsx(5 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/src/assets/theme.jsx
[error] 16-22: This property value named MuiSvgIcon is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named MuiSvgIcon
(lint/suspicious/noDuplicateObjectKeys)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
frontend/src/assets/theme.jsx (3)
293-294: Standardize box shadow valuesThese box shadow values are repeating like a rap battle! Let's extract them into variables.
Consider creating shadow variables:
+ // In your CSS variables + --shadow-focus-primary: 0px 0px 0px 4px #9E77ED3D, 0px 1px 2px 0px #1018280D; + --shadow-focus-error: 0px 0px 0px 4px #FCCED7, 0px 1px 2px 0px #1018280D; + --shadow-focus-neutral: 0px 0px 0px 4px #98A2B324, 0px 1px 2px 0px #1018280D; - boxShadow: "0px 0px 0px 4px #9E77ED3D, 0px 1px 2px 0px #1018280D", + boxShadow: "var(--shadow-focus-primary)",Also applies to: 313-315, 337-340, 353-355
Line range hint
368-421: Dark theme needs component parity with light themeYo! The dark theme's looking a bit light on components compared to its sibling! We're missing overrides for:
- MuiTextField
- MuiInputLabel
- MuiListItem
- MuiButton
- And more...
This could cause inconsistent styling when switching themes. Consider mirroring all component overrides from the light theme with appropriate dark mode values.
Would you like me to help generate the missing dark theme component styles?
Line range hint
1-421: Consider enhancing accessibility featuresThe theme's got the basic moves, but we need to make sure everyone can dance to this beat! Consider:
- Verifying color contrast ratios meet WCAG guidelines
- Enhancing focus indicators for keyboard navigation
- Adding aria-labels where necessary
Consider using a tool like
color-contrast-checkerto validate your color combinations:#!/bin/bash # Verify color contrast ratios npx color-contrast-checker \ --background "var(--main-purple)" \ --foreground "white" \ --wcag "AA"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
frontend/src/assets/theme.jsx(5 hunks)
🔇 Additional comments (2)
frontend/src/assets/theme.jsx (2)
6-6: LGTM! Quote style standardization looks good.
The palette mode strings have been consistently updated to use double quotes.
Also applies to: 369-369
16-22: Remove duplicate MuiSvgIcon definition
Yo! This MuiSvgIcon definition is getting double-time like mom's spaghetti! Let's clean it up.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
frontend/src/index.css (1)
4-22: Yo! Let's standardize those opacity values!The CSS variables look solid, but we've got some inconsistency in how opacity is represented:
--text-secondary: #808080e6--text-secondary-light: #808080e5Let's make it consistent by using uppercase hex and standardized opacity values:
- --text-secondary: #808080e6; - --text-secondary-light: #808080e5; + --text-secondary: #808080E6; + --text-secondary-light: #808080E6;frontend/src/assets/theme.jsx (2)
171-197: Mom's spaghetti! Let's clean up these hardcoded values!The button styles have some hardcoded values that should be moved to CSS variables:
"1px solid #7F56D9""0px 1px 2px 0px rgba(16, 24, 40, 0.05)"Consider adding these CSS variables to index.css:
:root { --button-primary-border: #7F56D9; --button-shadow: 0px 1px 2px 0px rgba(16, 24, 40, 0.05); }Then update the button styles to use these variables.
371-371: Cleanup that spacing, it's making me nervous!There's an extra space in the CSS variable reference:
- main: "var( --main-purple)", + main: "var(--main-purple)",
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
frontend/src/assets/theme.jsx(4 hunks)frontend/src/index.css(1 hunks)
🔇 Additional comments (3)
frontend/src/index.css (1)
1-1: LGTM! Font import looks clean!
The Google Fonts import with double quotes is consistent with the styling standards.
frontend/src/assets/theme.jsx (2)
6-11: Sweet! The palette's looking fresh!
The light theme palette is properly configured with CSS variables, making it maintainable and consistent.
15-22: Yo! We still got that duplicate MuiSvgIcon definition!
This configuration is duplicated later in the file. Let's keep it DRY!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
frontend/src/assets/theme.jsx (6)
3-19: Yo! Nice job extracting common styles, that's the way to keep it DRY!The commonTabStyles object is well-structured and promotes code reuse. Just one tiny suggestion to make it even better!
Add a brief JSDoc comment to document the purpose and usage:
+/** + * Common styles for MuiTab component shared between light and dark themes. + * Defines font properties, colors, and states (selected/unselected). + */ const commonTabStyles = {
24-24: Yo! Clean up these spacing inconsistencies, they're making me nervous!The palette configuration has inconsistent indentation. Let's make it uniform!
palette: { - mode: "light", + mode: "light", primary: { - main: "var(--main-purple)", + main: "var(--main-purple)", }, background: { - default: "var(--background-default-light)", + default: "var(--background-default-light)", }, },Also applies to: 26-26, 29-29
197-199: These magic numbers are heavy, there's vomit on his sweater already!Consider extracting button dimensions into CSS variables for better maintainability.
// In your CSS variables +:root { + --button-width-standard: 142px; + --button-height-standard: 34px; + --button-gap-standard: var(--spacing-xs, 4px); +} // In the theme -width: "142px", -height: "34px", -gap: "var(--spacing-xs, 4px)", +width: "var(--button-width-standard)", +height: "var(--button-height-standard)", +gap: "var(--button-gap-standard)",Also applies to: 207-209
202-202: These shadow values are making me lose myself in the moment!Consider extracting box shadow values into CSS variables for consistency and easier maintenance.
// In your CSS variables +:root { + --shadow-standard: 0px 1px 2px 0px rgba(16, 24, 40, 0.05); + --shadow-active-primary: 0px 0px 0px 4px #9E77ED3D, 0px 1px 2px 0px #1018280D; + --shadow-active-error: 0px 0px 0px 4px #FCCED7, 0px 1px 2px 0px #1018280D; +} // In the theme -boxShadow: "0px 1px 2px 0px rgba(16, 24, 40, 0.05)", +boxShadow: "var(--shadow-standard)", -boxShadow: "0px 0px 0px 4px #9E77ED3D, 0px 1px 2px 0px #1018280D", +boxShadow: "var(--shadow-active-primary)", -boxShadow: "0px 0px 0px 4px #FCCED7, 0px 1px 2px 0px #1018280D", +boxShadow: "var(--shadow-active-error)",Also applies to: 297-297, 317-317
372-372: Same spacing issues in dark theme, knees weak!Let's maintain consistent indentation in the palette configuration.
palette: { - mode: "dark", + mode: "dark", primary: { - main: "var( --main-purple)", + main: "var(--main-purple)", // Also removed extra space in var }, background: { - default: "var(--background-default-dark)", + default: "var(--background-default-dark)", }, },Also applies to: 374-374, 377-377
Line range hint
389-397: Mom's spaghetti! We can DRY up this drawer configuration!The MuiDrawer configuration is duplicated between light and dark themes. Let's extract it!
+// At the top of the file +const commonDrawerStyles = { + styleOverrides: { + paper: { + width: "250px", + flexShrink: 0, + zIndex: 1200, + }, + }, +}; // Then in both themes MuiDrawer: commonDrawerStyles,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
frontend/src/assets/theme.jsx(4 hunks)
🔇 Additional comments (1)
frontend/src/assets/theme.jsx (1)
Line range hint 1-414: Verify CSS variable definitions and fallbacks!
Let's ensure all CSS variables used in the theme have proper definitions and fallback values to prevent style breaking.
✅ Verification successful
CSS variables are properly defined and have fallback values - all good!
After checking the CSS variable definitions in frontend/src/index.css, all variables used in theme.jsx are properly defined in the :root selector with appropriate values. The theme also includes fallback values for critical properties:
- Component button borders have fallbacks:
#7F56D9and#D0D5DD - Spacing variables have fallbacks:
--spacing-xs, 4px - Brand colors have fallbacks:
#EAECF0
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all CSS variable usages and verify their definitions
echo "Searching for CSS variable usages..."
rg "var\(--[^)]+\)" frontend/src/assets/theme.jsx | sort -u
echo "Checking for CSS variable definitions..."
rg ":root" -A 50 frontend/src/index.css
Length of output: 3355
frontend/src/index.css
Outdated
|
|
||
| /* Defining CSS Variables */ | ||
| :root { | ||
| --color-black: #000; |
There was a problem hiding this comment.
Same variables are also in variables.css
Is it possible to get rid of one of them?
There was a problem hiding this comment.
we can do one thing that
we can shift these variable color to variable.css and remove it from index.css.
Kindly Confirm if i am thinking right , open to suggestions
There was a problem hiding this comment.
made all the changes as requested
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (9)
frontend/src/scenes/login/Login.css (2)
123-132: Cleanup time! There's commented code on his sweater already!Remove commented-out font styles to keep the codebase clean.
.login-container h3 { - /* font-size: 16px; */ color: var(--second-text-color); text-align: center; - /* font-weight: 500; */ margin-bottom: 30px; font-size: 13px; font-weight: 400; line-height: 24px; }
152-158: Knees weak from hardcoded colors!The error message color should use a CSS variable for consistency with the theme system.
.error-message { - color: #d92d20; + color: var(--error-color); font-size: 13px; font-weight: 400; margin-bottom: 1rem; margin-top: 0.25rem; }frontend/src/components/LeftMenu/LeftMenu.css (1)
Yo, these MUI styles need to bounce from the CSS and move to their new home! 🏃♂️
The theme.jsx file is already set up with proper MUI component customizations, including:
- MuiTypography
- MuiListItemButton
- MuiSvgIcon
These styles in LeftMenu.css should be moved to the theme configuration since:
- The theme.jsx already handles similar component styling
- It's following the project's pattern of centralizing MUI styles
- Other CSS files in the project don't contain these MUI classes
Move these media query styles to the appropriate component sections in theme.jsx, using the styleOverrides.root pattern that's already established.
🔗 Analysis chain
Line range hint
11-54: Yo, we still got some MUI classes in here! 😰These media queries still contain MUI-specific classes that should be moved to the theme configuration:
.title .MuiTypography-root.menu-item .MuiTypography-root.left-menu .MuiListItemButton-root.left-menu .MuiSvgIcon-rootThis conflicts with the PR objective of moving MUI styles to theme.jsx.
Yo dawg, these MUI classes are still hanging around! Time to clean house! 🧹
Let me check if there's a theme.jsx file where these styles should be moved to.
Let me check what's in that theme file to see if it's ready for these MUI styles.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for remaining MUI classes across CSS files rg -g '*.css' "Mui[A-Z][a-zA-Z]+-root"Length of output: 771
Script:
#!/bin/bash # Look for theme configuration files fd "theme.jsx?" --type f # Also check for any existing MUI theme customization rg -g "*.jsx?" "createTheme|ThemeProvider" -A 5Length of output: 255
Script:
#!/bin/bash # Check the contents of the theme file cat frontend/src/assets/theme.jsx # Also let's see if there are any other CSS files that might have already moved MUI styles to themes rg -g "*.css" "MuiTypography|MuiListItemButton|MuiSvgIcon" --files-without-matchLength of output: 16373
frontend/src/components/DropdownList/DropdownList.jsx (2)
Line range hint
41-43: Hold up, we got some CSS classes here! 🤔The component uses CSS classes for styling:
className={`select ${className}`}Since we're moving MUI styles to theme.jsx, consider using MUI's styling solution:
- Use
sxprop for custom styles- Define component overrides in theme.jsx
Example refactor:
-className={`select ${className}`} +sx={{ + // Move styles from CSS to here + ...theme.components.MuiSelect?.styleOverrides?.root, + ...(className && { className }) +}}
Line range hint
44-52: Same deal with MenuItem styling! 🎯The MenuItem components also use CSS classes that should be moved to theme configuration.
Example refactor:
-className="menuItem" +sx={theme.components.MuiMenuItem?.styleOverrides?.root}frontend/src/styles/variables.css (1)
Line range hint
68-88: Yo! Fix that indentation, it's making me nervous!The dark mode section's indentation is inconsistent with the rest of the file. Let's clean it up to maintain readability.
body[data-theme="dark"] { - --main-text-color: #ffffff; - --second-text-color: #b0b0b0; - --third-text-color: #a0a0a0; + --main-text-color: #ffffff; + --second-text-color: #b0b0b0; + --third-text-color: #a0a0a0;frontend/src/assets/theme.jsx (2)
382-384: Extract these magic numbers to CSS variables, they're making me dizzy!Let's move these common values to CSS variables for better maintainability.
- borderRadius: "4px", - textTransform: "none", - boxShadow: "none", + borderRadius: "var(--border-radius-sm)", + textTransform: "none", + boxShadow: "var(--shadow-none)",
Line range hint
477-513: Reduce that code duplication, it's heavy on my sweater already!The dark theme duplicates many styles from the light theme. Consider extracting more common styles into shared objects.
Create a common styles object:
const commonComponents = { MuiDrawer: { styleOverrides: { paper: { width: "250px", flexShrink: 0, zIndex: 1200, }, }, }, MuiTabs: { styleOverrides: { indicator: { backgroundColor: "var(--main-purple)", }, }, }, }; // Then use it in both themes components: { ...commonComponents, // theme-specific overrides here }frontend/src/components/DatePicker/DatePickerStyles.css (1)
33-33: Yo dawg, this order property ain't doing much!The
order: 0declaration is redundant since it's already the default value. We can drop it like it's hot! 🔥- order: 0;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (24)
frontend/src/assets/theme.jsx(4 hunks)frontend/src/components/Button/Button.jsx(3 hunks)frontend/src/components/Button/ButtonStyles.css(0 hunks)frontend/src/components/Checkbox/Checkbox.jsx(5 hunks)frontend/src/components/Checkbox/CheckboxStyles.css(0 hunks)frontend/src/components/DataTable/TableStyles.css(1 hunks)frontend/src/components/DatePicker/DatePickerStyles.css(4 hunks)frontend/src/components/DropdownList/DropdownList.css(0 hunks)frontend/src/components/DropdownList/DropdownList.jsx(1 hunks)frontend/src/components/DropdownMenu/DropdownMenu.css(1 hunks)frontend/src/components/DropdownMenu/DropdownMenuList/DropdownMenuList.css(1 hunks)frontend/src/components/HeadingTabs/TabStyles.css(1 hunks)frontend/src/components/LeftMenu/LeftMenu.css(3 hunks)frontend/src/components/Links/Card/Card.module.scss(0 hunks)frontend/src/components/Links/Settings/Settings.module.scss(0 hunks)frontend/src/components/TextFieldComponents/CustomTextField/CustomTextField.jsx(4 hunks)frontend/src/components/TextFieldComponents/CustomTextField/CustomTextFieldStyles.css(0 hunks)frontend/src/index.css(1 hunks)frontend/src/scenes/login/Login.css(1 hunks)frontend/src/scenes/login/Login.module.css(3 hunks)frontend/src/scenes/settings/TeamTab/TeamTab.module.css(1 hunks)frontend/src/scenes/tours/CreateToursPopup/CreateToursPopup.module.scss(4 hunks)frontend/src/styles/variables.css(4 hunks)frontend/src/templates/GuideMainPageTemplate/GuideMainPageComponents/List/ListItem/ListItem.css(1 hunks)
💤 Files with no reviewable changes (6)
- frontend/src/components/Links/Card/Card.module.scss
- frontend/src/components/DropdownList/DropdownList.css
- frontend/src/components/Checkbox/CheckboxStyles.css
- frontend/src/components/Button/ButtonStyles.css
- frontend/src/components/TextFieldComponents/CustomTextField/CustomTextFieldStyles.css
- frontend/src/components/Links/Settings/Settings.module.scss
✅ Files skipped from review due to trivial changes (10)
- frontend/src/scenes/login/Login.module.css
- frontend/src/components/DropdownMenu/DropdownMenu.css
- frontend/src/templates/GuideMainPageTemplate/GuideMainPageComponents/List/ListItem/ListItem.css
- frontend/src/scenes/settings/TeamTab/TeamTab.module.css
- frontend/src/components/Button/Button.jsx
- frontend/src/components/DropdownMenu/DropdownMenuList/DropdownMenuList.css
- frontend/src/components/DataTable/TableStyles.css
- frontend/src/components/Checkbox/Checkbox.jsx
- frontend/src/index.css
- frontend/src/scenes/tours/CreateToursPopup/CreateToursPopup.module.scss
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/src/components/DatePicker/DatePickerStyles.css
[error] 66-66: Unexpected shorthand property background after background-color
(lint/suspicious/noShorthandPropertyOverrides)
[error] 88-88: Unexpected shorthand property background after background-color
(lint/suspicious/noShorthandPropertyOverrides)
🔇 Additional comments (9)
frontend/src/scenes/login/Login.css (2)
69-80: These button styles are sweating to get into theme.jsx!
If these buttons are MUI Button components, their styles should be in theme.jsx as per PR objectives.
Let's check for MUI Button usage:
If using MUI Button, migrate these styles to theme.jsx:
MuiButton: {
styleOverrides: {
root: {
width: '100%',
padding: '10px',
marginBottom: '15px',
borderRadius: '8px',
fontSize: '13px'
}
}
}44-58: Hold up! These input styles might clash with MUI TextField!
According to the PR objectives, MUI component styles should be moved to theme.jsx. These input styles might conflict with MUI TextField component styling.
Let's check if these inputs are using MUI components:
If using MUI TextField, consider moving these styles to theme.jsx under MuiTextField override:
MuiTextField: {
styleOverrides: {
root: {
'& .MuiInputBase-input': {
padding: '8px 12px',
fontSize: '13px',
// ... other styles
}
}
}
}frontend/src/components/HeadingTabs/TabStyles.css (1)
1-9: Yo dawg, these container styles are lookin' fresh! 🔥
The container styles are properly separated from MUI-specific styles, maintaining only layout and positioning. Good use of consistent hex color casing (#ebebeb).
frontend/src/components/LeftMenu/LeftMenu.css (1)
Line range hint 1-9: Aight, the base styles are clean! 💯
The base styles for .left-menu look good with consistent hex color usage (#ebebeb).
frontend/src/components/DatePicker/DatePickerStyles.css (5)
Line range hint 1-99: Verify MUI styles migration, don't lose yourself in the moment! ✨
The PR objective was to move MUI-specific styles to theme.jsx. While some styles have been removed, let's verify that all MUI-specific styles have been properly migrated.
✅ Verification successful
All MUI styles have been properly migrated, yo! 🍝
Based on the search results, we can see that:
- No MUI-specific classes remain in
DatePickerStyles.css - The MUI styles have been successfully moved to
theme.jsx, including:MuiPickersDaycomponent stylingMuiDateRangePickerDayrelated styles- Range interval day styling
The code's clean like mom's kitchen after spaghetti night!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining MUI-specific classes
rg "Mui[A-Z]" frontend/src/components/DatePicker/DatePickerStyles.css
# Check if these styles exist in theme.jsx
rg "MuiPickersDay|MuiDateRangePickerDay" frontend/src/assets/theme.jsx
Length of output: 507
🧰 Tools
🪛 Biome (1.9.4)
[error] 88-88: Unexpected shorthand property background after background-color
(lint/suspicious/noShorthandPropertyOverrides)
46-50: Heads up about that fallback value, fam!
The fallback to var(--light-border-color) might cause issues if that variable isn't defined. Consider providing a solid color as the final fallback.
✅ Verification successful
Yo, that fallback's actually solid as a rock!
The --light-border-color CSS variable is properly defined in frontend/src/styles/variables.css with specific values for both light and dark themes:
- Light theme:
#d0d5dd - Dark theme:
#444444
It's widely used across the codebase in 30+ locations, making it a reliable fallback choice. No need to sweat about this one!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if --light-border-color is defined elsewhere
rg --type css "light-border-color" -B 2 -A 2
Length of output: 14183
83-91:
Same spaghetti, different sauce - duplicate background properties!
Let's clean up these duplicate background properties just like we did for the apply button.
- background-color: #fff !important;
- color: #344054 !important;
- border: 1px solid
- var(
- --Component-colors-Components-Buttons-Secondary-button-secondary-border,
- var(--light-border-color)
- ) !important;
- background: var(
- --Component-colors-Components-Buttons-Secondary-button-secondary-bg,
- #fff
- );
+ background: var(--Component-colors-Components-Buttons-Secondary-button-secondary-bg, #fff) !important;
+ color: #344054 !important;
+ border: 1px solid var(--Component-colors-Components-Buttons-Secondary-button-secondary-border, #e4e7ec) !important;Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 88-88: Unexpected shorthand property background after background-color
(lint/suspicious/noShorthandPropertyOverrides)
61-69:
Mom's spaghetti moment - we've got duplicate background properties!
You're setting both background-color and background, which can lead to unexpected behaviour. Let's clean this up!
- background-color: #7f56d9 !important;
- color: white !important;
- border: 1px solid
- var(
- --Component-colors-Components-Buttons-Primary-button-primary-border,
- #7f56d9
- ) !important;
- background: var(
- --Component-colors-Components-Buttons-Primary-button-primary-bg,
- #7f56d9
- );
+ background: var(--Component-colors-Components-Buttons-Primary-button-primary-bg, #7f56d9) !important;
+ color: white !important;
+ border: 1px solid var(--Component-colors-Components-Buttons-Primary-button-primary-border, #7f56d9) !important;Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 66-66: Unexpected shorthand property background after background-color
(lint/suspicious/noShorthandPropertyOverrides)
97-99: This class looking kinda slim, like losing yourself in the music! 🎵
Is this class necessary? It only has display: flex and might be redundant. Consider removing it if it's not serving a specific purpose.
frontend/src/components/TextFieldComponents/CustomTextField/CustomTextField.jsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
frontend/src/assets/theme.jsx(4 hunks)frontend/src/components/TextFieldComponents/CustomTextField/CustomTextField.jsx(4 hunks)frontend/src/styles/variables.css(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/TextFieldComponents/CustomTextField/CustomTextField.jsx
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/src/styles/variables.css
[error] 9-9: Unexpected value or character.
Expected one of:
(parse)
[error] 13-13: Unexpected value or character.
Expected one of:
(parse)
🔇 Additional comments (4)
frontend/src/styles/variables.css (1)
Line range hint 84-104: LGTM! Dark mode overrides looking clean!
The dark mode color overrides are well-structured and follow consistent naming patterns.
frontend/src/assets/theme.jsx (3)
2-19: Knees weak, but this code is strong! Nice extraction of common styles!
The extraction of common tab styles improves maintainability and follows the DRY principle.
405-410: Accessibility contrast ratio concerns persist!
The disabled button state's color combination might not meet WCAG contrast requirements.
Line range hint 477-521: Dark theme implementation looking clean!
Good job on reusing the common styles and maintaining consistency with CSS variables.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
frontend/src/styles/variables.css (3)
6-10: Consider adding documentation for the spacing system.The spacing variables follow a logical progression, but adding comments about their intended usage would improve maintainability.
/* Spacing */ + /* xs: 4px - Minimal spacing for tight layouts + * md: 8px - Default spacing between related elements + * lg: 12px - Spacing between distinct elements + * xl: 16px - Section spacing + * 3xl: 24px - Major section spacing + */ --spacing-xs: 4px;
17-21: Yo! Let's make these color names consistent!The text color variables use inconsistent naming patterns. Some use 'text-' prefix while others don't.
- --text-secondary: #808080e6; - --text-secondary-light: #808080e5; - --second-text-color: #667085; - --secondary-muted-text-color: #98a2b3; - --secondary-muted-text-color-light: #98a2b324; + --text-color-secondary: #808080e6; + --text-color-secondary-light: #808080e5; + --text-color-secondary-alt: #667085; + --text-color-muted: #98a2b3; + --text-color-muted-light: #98a2b324;
76-77: Consider documenting the border radius system.Add comments to explain when to use each radius size.
/* Border Radius */ + /* xs: 4px - For small elements like chips + * sm: 6px - For buttons and cards + */ --radius-sm: 6px; --radius-xs: 4px;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
frontend/src/assets/theme.jsx(4 hunks)frontend/src/components/HeadingTabs/TabStyles.css(1 hunks)frontend/src/styles/variables.css(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/HeadingTabs/TabStyles.css
🔇 Additional comments (7)
frontend/src/styles/variables.css (3)
24-25: LGTM! Hover states are well defined.
The hover state variables for the main purple color are properly implemented with both solid and light variants.
29-31: Mom's spaghetti! We've got duplicate color definitions!
The border error colors are defined with different values:
--border-error-solid: #d92d20--border-error-alt: #d32f2f
58-62: LGTM! Shadow and outline system is well structured.
The shadow and outline variables are well organized with consistent naming and color formats.
frontend/src/assets/theme.jsx (4)
3-19: LGTM! Common tab styles are well organized.
Good job extracting common styles into a shared object and using CSS variables consistently.
22-30: LGTM! Theme configuration uses CSS variables properly.
The theme configuration correctly uses CSS variables for colors and backgrounds.
430-434: Yo! We've got accessibility issues with the disabled button state!
The disabled button state uses a literal "grey" color which may not provide sufficient contrast.
Line range hint 479-519: LGTM! Dark theme is properly configured.
The dark theme correctly reuses common tab styles and uses CSS variables for colors.
| } | ||
|
|
||
| /* .primary */ | ||
| .primary.MuiButton-root { |
There was a problem hiding this comment.
You may have understood the task wrong. You shouldn't delete this file or delete classes like these. .primary.MuiButton-root is not the same as .MuiButton-root. We are looking for the latter to refactor. all others should stay in place
There was a problem hiding this comment.
hey @erenfn i undertsood your issue
but i have to ask that
1.
in this image the primary things is handled using the contained primary
so are you saying that the contained primary logic is extra and i should remove that or is there a still difference in both.
so if this is the case like i have written an extra logic so do i have to remove all those extra logic and just only refactor the root MUI. kindly correct me if i am going in the wrong direction
There was a problem hiding this comment.
It may work, but I cannot test it because this error occurs during the build process:
Uncaught Error: MUI: Unsupported `var(--main-purple)` color.
The following formats are supported: #nnn, #nnnnnn, rgb(), rgba(), hsl(), hsla(), color().
Please test your code
|
@Bugbuster2004 Any updates? |
|
hey @erenfn apologies for keeping u waiting , got stuck in some college work , i will work on this tonight and will update you by tomorrow morning. (Indian time) |
|
Hey, that's fine. Thanks for letting me know |
|
hey @erenfn i have made the changes |
|
You can disregard the warnings and send the PR |
|
Hello @erenfn I have raised the PR; however, I was unable to test the frontend code as the backend was not functioning as expected due to some issues with the Docker setup. Though the build process completed successfully without any errors. If needed, please let me know, and I would appreciate your guidance in resolving the backend setup process. Thank you! |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
frontend/src/assets/theme.jsx (2)
5-6: Yo! This cssVar utility is fire! 🔥The
cssVarutility function is a clean way to access CSS variables. However, it might throw if called before the document is ready.Consider adding a fallback:
-const cssVar = (name) => - getComputedStyle(document.documentElement).getPropertyValue(name).trim(); +const cssVar = (name) => { + try { + return getComputedStyle(document.documentElement).getPropertyValue(name).trim(); + } catch (err) { + console.warn(`Failed to get CSS variable ${name}:`, err); + return ''; + } +};
Line range hint
485-528: Dark theme needs more love, arms are heavy!The dark theme is missing many of the component customizations present in the light theme. This could lead to inconsistent styling in dark mode.
Consider adding the missing component customizations to the dark theme, particularly:
- MuiTextField
- MuiButton
- MuiCheckbox
- MuiSelect
- MuiMenuItem
Would you like me to help generate the dark theme equivalents for these components?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
frontend/dist/index.htmlis excluded by!**/dist/**frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
frontend/src/App.jsx(2 hunks)frontend/src/assets/theme.jsx(5 hunks)frontend/src/components/TextFieldComponents/CustomTextField/CustomTextField.jsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/TextFieldComponents/CustomTextField/CustomTextField.jsx
🔇 Additional comments (3)
frontend/src/App.jsx (1)
43-105: LGTM! ThemeProvider integration looks solid!
The ThemeProvider is correctly wrapping the entire application, ensuring consistent theming across all components.
frontend/src/assets/theme.jsx (2)
9-25: LGTM! Common tab styles are clean and reusable!
Good job extracting common tab styles into a shared object. This reduces code duplication and improves maintainability.
436-437: Palms are sweaty! Still got some hardcoded colors!
There are still hardcoded colors that should be moved to CSS variables:
"grey"on line 436"white"on line 463
These were previously flagged in past reviews.
Also applies to: 462-463
| <Route path="/banner/create" element={<BannerPage />} /> | ||
| <Route path="/popup/create" element={<Private Component={CreatePopupPage} />} /> | ||
| <Route path="/banner" element={<Private Component={BannerDefaultPage} />} /> | ||
| <Route path="/popup" element={<Private Component={PopupDefaultPage} />} /> | ||
| <Route | ||
| path="/popup/create" | ||
| element={<Private Component={CreatePopupPage} />} | ||
| /> | ||
| <Route | ||
| path="/banner" | ||
| element={<Private Component={BannerDefaultPage} />} | ||
| /> | ||
| <Route | ||
| path="/popup" | ||
| element={<Private Component={PopupDefaultPage} />} | ||
| /> | ||
| <Route path="/link" element={<LinksDefaultPage />} /> | ||
| <Route path="/tour" element={<ToursDefaultPage />} /> |
There was a problem hiding this comment.
Mom's spaghetti! We've got duplicate routes here!
These routes are already defined within the private route section (lines 47-67). Having duplicate routes can lead to confusion and maintenance issues.
Remove the duplicate routes as they're already defined within the Private route component:
- <Route path="/banner/create" element={<BannerPage />} />
- <Route
- path="/popup/create"
- element={<Private Component={CreatePopupPage} />}
- />
- <Route
- path="/banner"
- element={<Private Component={BannerDefaultPage} />}
- />
- <Route
- path="/popup"
- element={<Private Component={PopupDefaultPage} />}
- />
- <Route path="/link" element={<LinksDefaultPage />} />
- <Route path="/tour" element={<ToursDefaultPage />} />| setIsAdminLogin(!teamExists); | ||
| } catch (err) { | ||
| } | ||
| } | ||
| } catch (err) {} | ||
| }; |
There was a problem hiding this comment.
Yo! That empty catch block is making me nervous!
The empty catch block in fetchTeamCount could swallow important errors without any logging or user feedback.
Add error handling:
- } catch (err) {}
+ } catch (err) {
+ console.error('Failed to fetch team count:', err);
+ // Consider showing a user-friendly error message
+ }Committable suggestion skipped: line range outside the PR's diff.
| backgroundColor: `cssVar("--main-purple")!important`, | ||
| color: "white !important", |
There was a problem hiding this comment.
Fix that template literal syntax, knees weak!
The cssVar function is incorrectly used within a template literal.
Fix the syntax:
-backgroundColor: `cssVar("--main-purple")!important`,
+backgroundColor: `${cssVar("--main-purple")}!important`,Or better yet, remove the template literal entirely:
-backgroundColor: `cssVar("--main-purple")!important`,
+backgroundColor: `${cssVar("--main-purple")} !important`,Committable suggestion skipped: line range outside the PR's diff.
|
hey @erenfn i got your point , i have to refactor the code again by sticking to the original issue it will take me some days to do that as i have my exams going on currently. |
|
Hey, it has taken a while, so we solved it with #430 . Thanks for your time! |
Description:
This pull request addresses issue #288 by consolidating MUI-specific component styles from various CSS files into the centralized theme.jsx file. This approach ensures better maintainability, consistency, and scalability across the application’s design system.
Key Changes Made:
Migration of Styles:
Moved the MuiTab-root styles from src/components to the theme.jsx file under the overrides section.
Reviewed and migrated similar MUI-related component styles from other CSS files to the centralized theme configuration.
Theme Customization:
Added custom typography configurations such as font-family, font-size, font-style, font-weight, and line-height for MuiTab-root and other applicable components in the theme.
Ensured that the styles align with the Inter font family as per the design requirements.
Code Cleanup:
Removed redundant CSS files or declarations for MUI components that are now covered under the theme.jsx configuration.
Refactored the CSS file for HeadingTabs to retain only non-MUI-specific stylings.
Branch Specifics:
All changes have been made on the develop branch, which is the most up-to-date branch for this project.