Refactored CSS for LeftDrawerOrg #2996
Conversation
WalkthroughThe pull request focuses on refactoring the CSS for the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
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
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/components/LeftDrawerOrg/LeftDrawerOrg.tsx (3)
14-14: Use absolute path imports for consistency (optional).
While this relative path import works, converting it to an absolute path (e.g.,import styles from 'src/style/app.module.css';) can often improve clarity, especially in larger projects where directory structures can become complex.
108-111: Consider adding accessibility attributes for loading states.
Using a “shimmer” styling is a great approach for placeholders during loading. For the sake of accessibility, consider adding anaria-busy="true"or using a ARIA role to communicate that the content is loading.
115-115: Avoid mixing inline color classes with external classes.
You’re mixingstyles.bgDanger,text-start, andtext-whitein one class list. Ensure these remain consistent with your theming approach and naming conventions. If you plan on theming repeatedly, consider adding a specialized CSS class for this exact style combination.src/style/app.module.css (1)
3484-3595: Unify with existing drawer transitions and naming practices.
This block introduces new styles for the left drawer transitions (.inactiveDrawer,.activeDrawer, etc.). Ensure that these are consistent with existing transition durations, if any, elsewhere in the codebase. Also, confirm the naming scheme (e.g.,hideElemByDefault) is aligned with your typical BEM or utility-class approach for future maintainability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/LeftDrawerOrg/LeftDrawerOrg.module.css(0 hunks)src/components/LeftDrawerOrg/LeftDrawerOrg.tsx(4 hunks)src/style/app.module.css(1 hunks)
💤 Files with no reviewable changes (1)
- src/components/LeftDrawerOrg/LeftDrawerOrg.module.css
🔇 Additional comments (2)
src/components/LeftDrawerOrg/LeftDrawerOrg.tsx (2)
62-62: No substantial change detected.
This line appears to be blank or unchanged in a functional sense. No concerns here.
159-168: Align variant and color usage to ensure consistent theming.
While toggling the variant prop and linking activeButton / inactiveButton classes works, double-check that using variant={isActive ? 'success' : ''} aligns with your global theming. If you rely heavily on custom CSS classes, you may not need to specify the Bootstrap variant.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/LeftDrawerOrg/LeftDrawerOrg.tsx (2)
34-34: Consider restoring component documentationThe removed JSDoc comments provided valuable context about the component's purpose and props. While cleaning up styles, it's important to maintain good documentation practices.
Consider restoring the following documentation:
/** * LeftDrawerOrg component for displaying organization details and navigation options. * * @param orgId - ID of the current organization * @param targets - List of navigation targets * @param hideDrawer - Determines if the drawer should be hidden or shown * @param setHideDrawer - Function to update the visibility state of the drawer * @returns JSX element for the left navigation drawer with organization details */
71-72: Remove extra whitespace linesExtra whitespace lines were detected. While they don't affect functionality, they should be removed for consistency.
- - // Get the ID from the current path + // Get the ID from the current path - - // Check if the current page is admin profile page + // Check if the current page is admin profile pageAlso applies to: 77-78
🧰 Tools
🪛 eslint
[error] 71-71: Delete
··(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/LeftDrawerOrg/LeftDrawerOrg.tsx(6 hunks)
🧰 Additional context used
🪛 eslint
src/components/LeftDrawerOrg/LeftDrawerOrg.tsx
[error] 71-71: Delete ··
(prettier/prettier)
[error] 77-77: Delete ··
(prettier/prettier)
🔇 Additional comments (4)
src/components/LeftDrawerOrg/LeftDrawerOrg.tsx (4)
14-14: LGTM: CSS import change aligns with refactoring goals
The change from local to global CSS module import aligns with the PR objective of consolidating styles into app.module.css.
128-131: LGTM: Loading state implementation
The loading state button implementation is clean and uses appropriate CSS classes from the global stylesheet.
179-182: LGTM: Button styling implementation
The button styling implementation correctly uses the global CSS classes and maintains the active/inactive state visual feedback.
Line range hint 1-207: Verify complete CSS migration
The component has been successfully refactored to use global CSS. To ensure no styles were missed:
✅ Verification successful
Let me check the actual content of the CSS classes to ensure they were properly migrated.
CSS migration successfully verified
The verification confirms that:
- No CSS module files remain in the LeftDrawerOrg component directory
- All relevant styles have been properly migrated to
src/style/app.module.css, including:- Layout styles (
.leftDrawer) - Branding styles (
.brandingContainer) - Organization section styles (
.organizationContainer,.profileContainer) - All nested styles for images, text, and responsive behavior
- Layout styles (
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining CSS module files in the LeftDrawerOrg directory
fd -e css "LeftDrawerOrg\.module\.css$" src/components/LeftDrawerOrg
# Verify all styles were moved to app.module.css
rg -l "leftDrawer|brandingContainer|organizationContainer|profileContainer" src/style/app.module.css
Length of output: 198
Script:
#!/bin/bash
# Check the content of app.module.css for the migrated styles
rg "leftDrawer|brandingContainer|organizationContainer|profileContainer" src/style/app.module.css -A 2
Length of output: 1561
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #2996 +/- ##
=====================================================
+ Coverage 26.46% 88.93% +62.47%
=====================================================
Files 300 321 +21
Lines 7568 8408 +840
Branches 1652 1893 +241
=====================================================
+ Hits 2003 7478 +5475
+ Misses 5434 688 -4746
- Partials 131 242 +111 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/style/app.module.css (2)
3577-3579: Use CSS variables for consistent theming.Replace hardcoded color values with CSS variables for better maintainability and consistent theming.
.activeButton { background-color: var(--grey-bg-color); - color: black; + color: var(--bs-black); }
3576-3584: Add hover and focus states for buttons.Interactive elements like buttons should have hover and focus states for better accessibility and user experience.
.activeButton { background-color: var(--grey-bg-color); color: var(--bs-black); + transition: background-color 0.2s ease; } +.activeButton:hover, +.activeButton:focus { + background-color: var(--grey-border-box-color); + outline: 2px solid var(--highlight-color, #a8c7fa); +} .inactiveButton { background-color: transparent; color: var(--bs-secondary); + transition: background-color 0.2s ease; } +.inactiveButton:hover, +.inactiveButton:focus { + background-color: var(--dropdown-hover-color); + outline: 2px solid var(--highlight-color, #a8c7fa); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/style/app.module.css(2 hunks)
🔇 Additional comments (2)
src/style/app.module.css (2)
3484-3491: LGTM! Core drawer styles follow best practices.
The core drawer styles are well-implemented with:
- Full viewport height coverage
- Smooth transitions
- Proper overflow handling
- Appropriate padding to prevent content clipping
3493-3503: Great use of transform for animations!
Using transform instead of position properties for drawer animations is excellent for performance as it:
- Triggers GPU acceleration
- Reduces layout recalculation
- Provides smoother animations
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/style/app.module.css (1)
3519-3564: Enhance maintainability with CSS variables.Consider the following improvements:
- Use CSS variables for padding values
- Make image dimensions configurable
.organizationContainer { - padding-right: 3rem; + padding-right: var(--drawer-padding-right, 3rem); } .imageContainer img { - width: 40px; - height: 40px; + width: var(--profile-image-size, 40px); + height: var(--profile-image-size, 40px); border-radius: 50%; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/style/app.module.css(2 hunks)
🔇 Additional comments (3)
src/style/app.module.css (3)
3485-3505: Well-implemented drawer layout and transitions!
Good use of transform for animations and proper overflow handling. The implementation follows performance best practices.
3506-3517: Remove empty rulesets or add documentation.
Several empty CSS rulesets should either be removed if unused or documented if they are intentional placeholders for future use.
3565-3599:
Enhance button states and accessibility.
The current implementation has several issues:
- Missing hover/focus states for buttons
- Lacks non-color indicators for active/inactive states
- Uses hardcoded colors instead of CSS variables
Apply these changes to improve the implementation:
.activeButton {
- background-color: var(--grey-bg-color);
+ background-color: var(--drawer-active-bg, var(--grey-bg-color));
color: black;
+ position: relative;
+ border-left: 3px solid var(--drawer-active-border, var(--bs-primary));
+ transition: background-color 0.2s ease;
}
+.activeButton::before {
+ content: "▶";
+ position: absolute;
+ left: 8px;
+ color: var(--drawer-active-border, var(--bs-primary));
+}
.inactiveButton {
background-color: transparent;
color: var(--bs-secondary);
+ transition: background-color 0.2s ease;
}
+.activeButton:hover,
+.inactiveButton:hover {
+ background-color: var(--drawer-hover-bg, var(--dropdown-hover-color));
+}
+.activeButton:focus,
+.inactiveButton:focus {
+ outline: 2px solid var(--drawer-focus-color, var(--bs-primary));
+ outline-offset: -2px;
+}Likely invalid or redundant comment.
|
Please fix the conflicting file(s) |
98268ca
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/style/app.module.css (2)
3761-3768: LGTM! Consider performance optimization.The core LeftDrawer styles are well-structured with proper handling of scrolling and positioning. For better performance, consider using
transformwithwill-changeproperty to optimize hardware acceleration..leftDrawer { height: 100vh; overflow-y: auto; transition: transform 0.3s ease; position: fixed; padding-bottom: 1rem; + will-change: transform; }
3795-3840: Enhance accessibility for organization section.While the implementation is solid, consider adding the following accessibility improvements:
.profileContainer { display: flex; align-items: center; justify-content: space-between; width: 100%; + role: "button"; + aria-label: "Organization profile"; + &:focus-visible { + outline: 2px solid var(--bs-primary); + outline-offset: 2px; + } } .primaryText { font-weight: bold; + color: var(--bs-emphasis-color, #000); } .secondaryText { font-size: 0.9rem; color: var(--grey-bg-color); + color: var(--bs-secondary-color, #6c757d); + @media (prefers-contrast: more) { + color: var(--bs-emphasis-color, #000); + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/style/app.module.css(2 hunks)
🔇 Additional comments (3)
src/style/app.module.css (3)
3770-3780: LGTM! Clean implementation of drawer states.
The visibility control classes use transform-based animations which is the recommended approach for performance. The state management is clean and follows best practices.
3782-3793: Remove empty rulesets in branding section.
These empty rulesets should either be removed or populated with actual styles.
3842-3845: Remove empty ruleset for optionList.
The empty ruleset should either be removed or populated with actual styles.
|
Please make coderabbit.ai approve your changes |
|
Please merge your local branch with the latest upstream, commit and then push to your origin. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/style/app.module.css (3)
10-10: Enhance color variable naming and accessibility.The background color should be defined as a semantic variable for better maintainability and accessibility.
- background-color: #f6f8fc; + --background-color-primary: #f6f8fc; + background-color: var(--background-color-primary);Consider adding these accessibility-focused variables:
:root { /* High-contrast theme colors */ --color-primary-contrast: #0056b3; --color-text-on-primary: #ffffff; --color-background-contrast: #f6f8fc; /* Color-blind safe palette */ --color-status-success: #2e7d32; --color-status-error: #d32f2f; --color-status-warning: #ed6c02; /* Focus indicators */ --focus-ring-color: #1976d2; --focus-ring-width: 2px; }
4059-4065: Optimize drawer transitions for performance.The drawer layout is well-structured, but the transitions could be optimized.
.leftDrawer { height: 100vh; overflow-y: auto; - transition: transform 0.3s ease; + transition: transform 0.3s ease-out; + will-change: transform; position: fixed; padding-bottom: 1rem; + overscroll-behavior: contain; }These changes will:
- Use
will-changeto optimize transform animations- Add
overscroll-behaviorto prevent scroll chaining- Use
ease-outtiming for smoother transitions
4071-4077: Optimize drawer state transitions.The drawer state transitions could be optimized for better performance.
.inactiveDrawer { transform: translateX(-100%); + visibility: hidden; + transform: translate3d(-100%, 0, 0); } .activeDrawer { transform: translateX(0); + visibility: visible; + transform: translate3d(0, 0, 0); }These changes will:
- Use
translate3dto trigger GPU acceleration- Add
visibilityfor better accessibility
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/style/app.module.css(3 hunks)
🔇 Additional comments (1)
src/style/app.module.css (1)
4130-4193: 🛠️ Refactor suggestion
Enhance button states and keyboard navigation.
The button states and keyboard navigation could be improved for better accessibility.
.activeButton {
background-color: var(--grey-bg-color);
color: black;
position: relative;
+ /* Add ARIA state indicator */
+ &[aria-current="true"]::before {
+ content: '';
+ position: absolute;
+ left: 0;
+ top: 0;
+ bottom: 0;
+ width: 3px;
+ background-color: var(--bs-primary);
+ }
+ /* Add focus ring for keyboard navigation */
&:focus-visible {
outline: 2px solid var(--bs-primary);
outline-offset: 2px;
+ box-shadow: 0 0 0 4px rgba(0, 86, 179, 0.25);
}
+ /* Add pressed state */
+ &:active {
+ transform: translateY(1px);
+ }
}
.iconWrapper {
&[aria-label]:not([aria-label='']) {
&::after {
content: attr(aria-label);
+ /* Improve tooltip positioning */
+ transform: translateY(-50%);
+ top: 50%;
+ z-index: 1000;
+ /* Add animation */
+ transition: opacity 0.2s, visibility 0.2s;
}
}
}These changes will:
- Add ARIA state indicators for screen readers
- Improve focus styles for keyboard navigation
- Add pressed states for better interaction feedback
- Enhance tooltip positioning and animation
Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/style/app.module.css (5)
10-10: Convert hardcoded color to CSS variable.The background color should use a CSS variable for consistency and maintainability.
- background-color: #f6f8fc; + --app-bg-color: #f6f8fc; + background-color: var(--app-bg-color);
4060-4066: Enhance drawer transition properties.The drawer transition could be more specific to improve performance.
.leftDrawer { height: 100vh; overflow-y: auto; - transition: transform 0.3s ease; + transition: transform 0.3s ease, visibility 0.3s ease; position: fixed; padding-bottom: 1rem; }
4072-4078: Use CSS variables for transform values.Extract transform values into CSS variables for better maintainability and consistency.
+:root { + --drawer-offset: -100%; +} .inactiveDrawer { - transform: translateX(-100%); + transform: translateX(var(--drawer-offset)); } .activeDrawer { transform: translateX(0); }
4081-4084: Standardize padding values.Use consistent padding values through CSS variables.
+:root { + --drawer-padding: 3rem; +} .organizationContainer { - padding-right: 3rem; + padding-right: var(--drawer-padding); }
4183-4207: Extract tooltip styles into a reusable class.The tooltip implementation could be more maintainable by extracting it into a separate class.
+.tooltip { + position: relative; +} + +.tooltip::after { + content: attr(aria-label); + position: absolute; + left: 100%; + background: var(--bs-dark); + color: var(--bs-white); + padding: 0.25rem 0.5rem; + border-radius: 4px; + font-size: 0.875rem; + opacity: 0; + visibility: hidden; +} + +.tooltip:hover::after { + opacity: 1; + visibility: visible; +} .iconWrapper { display: flex; align-items: center; margin-right: 8px; /* Add tooltip for icon-only buttons */ &[aria-label]:not([aria-label='']) { - position: relative; - &::after { - content: attr(aria-label); - position: absolute; - left: 100%; - background: var(--bs-dark); - color: var(--bs-white); - padding: 0.25rem 0.5rem; - border-radius: 4px; - font-size: 0.875rem; - opacity: 0; - visibility: hidden; - } - &:hover::after { - opacity: 1; - visibility: visible; - } + @extend .tooltip; } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/style/app.module.css(3 hunks)
🔇 Additional comments (1)
src/style/app.module.css (1)
4058-4207: LGTM! The LeftDrawer styles are well-implemented.
The changes successfully:
- Consolidate styles into the global CSS file
- Enhance accessibility with proper contrast, focus states, and tooltips
- Maintain consistent styling through semantic colors and variables
|
Please make sure you do not duplicate the css in the app.module.css , do a final check remove any duplications which you have added. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/style/app.module.css (3)
3569-3588: Enhance keyboard navigation and screen reader support.While the drawer implementation is solid, it could benefit from improved accessibility:
.leftDrawer { height: 100vh; overflow-y: auto; transition: transform 0.3s ease; position: fixed; padding-bottom: 1rem; + /* Improve keyboard navigation */ + &:focus-within { + outline: 2px solid var(--bs-primary); + outline-offset: -2px; + } + /* Add screen reader support */ + &[aria-hidden="true"] { + display: none; + } }
3589-3643: Add responsive design improvements for mobile devices.The organization section could benefit from better mobile support:
.organizationContainer { padding-right: 3rem; + /* Add responsive padding */ + @media (max-width: 768px) { + padding-right: 1rem; + } } .profileContainer { display: flex; align-items: center; justify-content: space-between; width: 100%; + /* Improve mobile layout */ + @media (max-width: 480px) { + flex-direction: column; + gap: 0.5rem; + text-align: center; + } } .imageContainer img { width: 40px; height: 40px; + /* Adjust image size on small screens */ + @media (max-width: 480px) { + width: 48px; + height: 48px; + } }
3652-3723: Improve CSS organization and reduce specificity.The button and icon styles could be better organized:
-/* Button specific styles */ +/* Component: Button + * Contains styles for active and inactive states, + * focus management, and hover effects + */ .button { + /* Common button styles */ + position: relative; + transition: background-color 0.2s ease; + + /* Focus management */ + &:focus-visible { + outline: 2px solid var(--bs-primary); + outline-offset: 2px; + } + } .activeButton { + composes: button; background-color: var(--grey-bg-color); color: black; - position: relative; &::before { content: ''; position: absolute; left: 0; top: 0; bottom: 0; width: 3px; background-color: var(--bs-primary); } } .inactiveButton { + composes: button; background-color: transparent; color: var(--bs-emphasis-color, #000); &:hover { background-color: var(--grey-bg-color); } } -/* Common focus-visible styles */ -.focus-visible { - outline: 2px solid var(--bs-primary); - outline-offset: 2px; -} -/* Applying the focus-visible class to both buttons */ -.activeButton, -.inactiveButton { - &.focus-visible { - outline: 2px solid var(--bs-primary); - outline-offset: 2px; - } -}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/style/app.module.css (3)
3582-3589: Add keyboard focus management for drawer states.While the animation implementation is performant using transform, consider adding focus management for better accessibility:
.activeDrawer { transform: translateX(0); + /* Ensure drawer content is focusable when visible */ + &:focus-within { + outline: none; + } + /* Restore focus to first interactive element when opened */ + &[aria-expanded="true"] { + animation: focusFirst 0.1s ease-out; + } } .inactiveDrawer { transform: translateX(-100%); + /* Prevent focusing hidden elements */ + &[aria-hidden="true"] { + visibility: hidden; + } }
3590-3644: Enhance color contrast for better accessibility.While the implementation is good overall, the secondary text color could have better contrast:
.secondaryText { font-size: 0.9rem; - color: var(--bs-secondary-text); + /* Ensure WCAG 2.1 AA contrast ratio of at least 4.5:1 */ + color: var(--bs-gray-700, #495057); }
3700-3724: Enhance tooltip accessibility.While the tooltip implementation is good, it could be more accessible:
.iconWrapper { display: flex; align-items: center; margin-right: 8px; /* Add tooltip for icon-only buttons */ &[aria-label]:not([aria-label='']) { position: relative; + /* Use aria-describedby for screen readers */ + &::before { + content: ''; + position: absolute; + width: 1px; + height: 1px; + padding: 0; + margin: -1px; + overflow: hidden; + clip: rect(0, 0, 0, 0); + white-space: nowrap; + border: 0; + } &::after { content: attr(aria-label); position: absolute; left: 100%; background: var(--bs-dark); color: var(--bs-white); padding: 0.25rem 0.5rem; border-radius: 4px; font-size: 0.875rem; opacity: 0; visibility: hidden; + /* Improve tooltip positioning */ + transform: translateX(8px); + transition: all 0.2s ease; } &:hover::after { opacity: 1; visibility: visible; + transform: translateX(0); } } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/style/app.module.css(3 hunks)
🔇 Additional comments (2)
src/style/app.module.css (2)
3569-3581: LGTM! Well-structured base styles for the drawer.
The implementation follows best practices with proper positioning, scrolling, and smooth transitions.
3654-3690: 🛠️ Refactor suggestion
Add non-color indicators for active state.
To better support color-blind users and align with the PR objectives:
.activeButton {
background-color: var(--grey-bg-color);
color: black;
position: relative;
+ /* Add left border and arrow indicator */
&::before {
content: '';
position: absolute;
left: 0;
top: 0;
bottom: 0;
width: 3px;
background-color: var(--bs-primary);
}
+ /* Add arrow indicator */
+ &::after {
+ content: '▶';
+ position: absolute;
+ right: 1rem;
+ top: 50%;
+ transform: translateY(-50%);
+ font-size: 0.8rem;
+ }
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/style/app.module.css (6)
3569-3588: Enhance drawer performance and accessibility.Consider these improvements for the drawer implementation:
- Add
will-change: transformto optimize animation performance- Add
aria-hiddenattribute handling for better accessibility.leftDrawer { height: 100vh; overflow-y: auto; - transition: transform 0.3s ease; + transition: transform 0.3s ease; + will-change: transform; position: fixed; padding-bottom: 1rem; } .inactiveDrawer { transform: translateX(-100%); + aria-hidden: true; } .activeDrawer { transform: translateX(0); + aria-hidden: false; }
3589-3643: Add text overflow handling for profile text.The profile text containers should handle overflow gracefully to prevent layout issues with long names or descriptions.
.primaryText { font-weight: bold; color: var(--bs-emphasis-color, #000); + max-width: 200px; + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; } .secondaryText { font-size: 0.9rem; color: var(--bs-secondary-text); + max-width: 200px; + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; }
3644-3650: Improve typography for better readability.Enhance the title header's readability by adding proper spacing and line height.
.titleHeader { font-size: 1.2rem; color: var(--bs-secondary); + line-height: 1.5; + margin: var(--spacing-md, 1rem) 0; }
3651-3674: Enhance visual feedback for indicators.Improve the visibility and interaction feedback of indicators.
.indicator { position: absolute; top: 0; bottom: 0; width: 3px; background-color: var(--bs-primary); + transition: opacity 0.2s ease; + opacity: 0.8; } .arrow-indicator { content: '▶'; position: absolute; right: 1rem; top: 50%; transform: translateY(-50%); font-size: 0.8rem; + color: var(--bs-emphasis-color, #000); + transition: transform 0.2s ease; } +.arrow-indicator:hover { + transform: translateY(-50%) scale(1.1); + opacity: 1; +}
3675-3703: Improve button state transitions.Enhance the button states with smooth transitions and better interaction feedback.
.activeButton { background-color: var(--grey-bg-color); color: black; position: relative; + transition: all 0.2s ease; &::before { @extend .indicator; } &::after { @extend .arrow-indicator; } + + &:active { + transform: scale(0.98); + } } .inactiveButton { background-color: transparent; color: var(--bs-emphasis-color, #000); + transition: all 0.2s ease; &:hover { background-color: var(--grey-bg-color); } &.focus-visible { @extend .focus-visible; } + + &:active { + transform: scale(0.98); + } }
3704-3743: Add motion preferences and RTL support.Enhance accessibility by respecting user motion preferences and supporting RTL layouts.
.iconWrapper { display: flex; align-items: center; margin-right: 8px; + margin-inline-end: 8px; + margin-inline-start: 0; &[aria-label]:not([aria-label='']) { position: relative; &::after { content: attr(aria-label); position: absolute; - left: 100%; + inset-inline-end: auto; + inset-inline-start: 100%; background: var(--bs-dark); color: var(--bs-white); padding: 0.25rem 0.5rem; border-radius: 4px; font-size: 0.875rem; opacity: 0; visibility: hidden; - transform: translateX(8px); + transform: translateX(var(--transform-direction, 8px)); transition: all 0.2s ease; } &:hover::after { opacity: 1; visibility: visible; transform: translateX(0); } } } +@media (prefers-reduced-motion: reduce) { + .iconWrapper[aria-label]:not([aria-label=''])::after { + transition: none; + } +} +[dir="rtl"] { + .iconWrapper { + --transform-direction: -8px; + } +}
|
Please get coderabbit.ai to approve your PR |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/style/app.module.css (1)
2-100: Enhance CSS variable organization and documentation.The CSS variables section could benefit from better organization and documentation:
- Some color variables have duplicate definitions (e.g.,
--grey-bg-colorand--table-bg)- Several variables lack clear purpose or usage documentation
Consider reorganizing the variables section:
:root { + /* Theme Colors - Primary */ --bs-primary: #0056b3; --blue-primary: #0056b3; /* Consider consolidating with bs-primary */ + /* Theme Colors - Secondary */ --bs-secondary: #6c757d; + /* Background Colors */ --grey-bg-color: #eaebef; --table-bg: #eaebef; /* Use --grey-bg-color instead */ + /* Documentation */ + /* @use: Primary button backgrounds */ --blue-primary: #0056b3; + /* @use: Table backgrounds and hover states */ --grey-bg-color: #eaebef; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/style/app.module.css(77 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/style/app.module.css
[error] 3747-3747: Unknown property is not allowed.
See CSS Specifications and browser specific properties for more details.
To resolve this issue, replace the unknown property with a valid CSS property.
(lint/correctness/noUnknownProperty)
[error] 3752-3752: Unknown property is not allowed.
See CSS Specifications and browser specific properties for more details.
To resolve this issue, replace the unknown property with a valid CSS property.
(lint/correctness/noUnknownProperty)
🔇 Additional comments (1)
src/style/app.module.css (1)
3634-3891: LGTM! Well-structured reusable classes.
The new reusable classes are well-organized and follow best practices:
- Text ellipsis handling
- Consistent focus indicators
- RTL support
- Reduced motion preferences
🧰 Tools
🪛 Biome (1.9.4)
[error] 3747-3747: Unknown property is not allowed.
See CSS Specifications and browser specific properties for more details.
To resolve this issue, replace the unknown property with a valid CSS property.
(lint/correctness/noUnknownProperty)
[error] 3752-3752: Unknown property is not allowed.
See CSS Specifications and browser specific properties for more details.
To resolve this issue, replace the unknown property with a valid CSS property.
(lint/correctness/noUnknownProperty)
|
|
||
| .inactiveDrawer { | ||
| transform: translateX(-100%); | ||
| aria-hidden: true; /* NEW */ |
There was a problem hiding this comment.
Fix invalid aria-hidden CSS properties.
The static analysis correctly identified invalid CSS properties.
Replace the invalid aria-hidden CSS properties with proper ARIA attributes in the HTML:
- .inactiveDrawer {
- aria-hidden: true;
- }
+ /* In the HTML/JSX */
+ <div
+ className="inactiveDrawer"
+ aria-hidden="true"
+ >Also applies to: 3752-3752
🧰 Tools
🪛 Biome (1.9.4)
[error] 3747-3747: Unknown property is not allowed.
See CSS Specifications and browser specific properties for more details.
To resolve this issue, replace the unknown property with a valid CSS property.
(lint/correctness/noUnknownProperty)
| } | ||
|
|
||
| .profileText { | ||
| margin-left: 1rem; | ||
| } | ||
|
|
||
| .primaryText { | ||
| font-weight: bold; | ||
| color: var(--bs-emphasis-color, #000); | ||
| @extend .reusable-text-ellipsis; /* NEW */ | ||
| } | ||
|
|
||
| .secondaryText { | ||
| font-size: 0.9rem; | ||
| color: var(--bs-secondary-text); | ||
| @extend .reusable-text-ellipsis; /* NEW */ | ||
| } | ||
|
|
||
| /* Dropdown and options list */ | ||
| .titleHeader { | ||
| font-size: 1.2rem; | ||
| color: var(--bs-secondary); | ||
| line-height: 1.5; /* NEW */ | ||
| margin: var(--spacing-md, 1rem) 0; /* NEW */ | ||
| } | ||
|
|
||
| /* Active and inactive buttons */ | ||
| .activeButton, | ||
| .inactiveButton { | ||
| position: relative; | ||
| transition: all 0.2s ease; | ||
|
|
||
| &:active { | ||
| transform: scale(0.98); | ||
| } | ||
|
|
||
| &.focus-visible { | ||
| @extend .reusable-focus-visible; | ||
| } | ||
| } | ||
|
|
||
| .activeButton { | ||
| background-color: var(--grey-bg-color); | ||
| color: black; | ||
|
|
||
| &::before { | ||
| @extend .reusable-indicator; /* NEW */ | ||
| } | ||
|
|
||
| &::after { | ||
| @extend .reusable-arrow-indicator; /* NEW */ | ||
| } | ||
| } | ||
|
|
||
| .activeButton:hover .arrow-indicator { | ||
| transform: translateY(-50%) scale(1.1); | ||
| opacity: 1; | ||
| } | ||
|
|
||
| .inactiveButton { | ||
| background-color: transparent; | ||
| color: var(--bs-emphasis-color, #000); | ||
|
|
||
| &:hover { | ||
| background-color: var(--grey-bg-color); | ||
| } | ||
| } | ||
|
|
||
| /* Icon wrapper styles */ | ||
| .iconWrapper { | ||
| display: flex; | ||
| align-items: center; | ||
| margin-inline-end: 8px; | ||
| margin-inline-start: 0; | ||
|
|
||
| &[aria-label]:not([aria-label='']) { | ||
| position: relative; | ||
|
|
||
| &::after { | ||
| content: attr(aria-label); | ||
| position: absolute; | ||
| inset-inline-start: 100%; | ||
| background: var(--bs-dark); | ||
| color: var(--bs-white); | ||
| padding: 0.25rem 0.5rem; | ||
| border-radius: 4px; | ||
| font-size: 0.875rem; | ||
| opacity: 0; | ||
| visibility: hidden; | ||
| transform: translateX(var(--transform-direction, 8px)); | ||
| transition: all 0.2s ease; | ||
| } | ||
|
|
||
| &:hover::after { | ||
| opacity: 1; | ||
| visibility: visible; | ||
| transform: translateX(0); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve accessibility and performance of the LeftDrawer component.
The LeftDrawer implementation has some accessibility and performance concerns:
Apply these improvements:
.leftDrawer {
height: 100vh;
overflow-y: auto;
transition: transform 0.3s ease;
will-change: transform;
position: fixed;
padding-bottom: 1rem;
+ /* Add overscroll behavior to prevent page scroll bleeding */
+ overscroll-behavior: contain;
+ /* Improve scroll performance */
+ -webkit-overflow-scrolling: touch;
}
.inactiveDrawer {
transform: translateX(-100%);
- aria-hidden: true; /* Invalid attribute */
+ /* Use data attribute for aria hidden state */
+ &[data-hidden="true"] {
+ visibility: hidden;
+ }
}
.activeDrawer {
transform: translateX(0);
- aria-hidden: false; /* Invalid attribute */
+ /* Use data attribute for aria hidden state */
+ &[data-hidden="false"] {
+ visibility: visible;
+ }
}📝 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.
| /* LeftDrawer general styles */ | |
| .leftDrawer { | |
| height: 100vh; /* Ensure it spans the full height */ | |
| overflow-y: auto; /* Enable vertical scrolling */ | |
| transition: transform 0.3s ease; | |
| will-change: transform; /* NEW */ | |
| position: fixed; /* Ensure it's positioned properly */ | |
| padding-bottom: 1rem; /* Prevent last item clipping */ | |
| } | |
| .hideElemByDefault { | |
| display: none; | |
| } | |
| .inactiveDrawer { | |
| transform: translateX(-100%); | |
| aria-hidden: true; /* NEW */ | |
| } | |
| .activeDrawer { | |
| transform: translateX(0); | |
| aria-hidden: false; /* NEW */ | |
| } | |
| /* Organization Section */ | |
| .organizationContainer { | |
| padding-right: 3rem; | |
| } | |
| .profileContainer { | |
| display: flex; | |
| align-items: center; | |
| justify-content: space-between; | |
| width: 100%; | |
| /* Add focus styles for keyboard navigation */ | |
| &:focus-within { | |
| @extend .reusable-focus-visible; | |
| } | |
| } | |
| .bg-danger { | |
| background-color: var(--delete-button-color); | |
| } | |
| .text-start { | |
| text-align: start; | |
| } | |
| .text-white { | |
| color: var(--bs-white); | |
| } | |
| .imageContainer img { | |
| width: 40px; | |
| height: 40px; | |
| border-radius: 50%; | |
| /* Add alt text styles */ | |
| &[alt] { | |
| font-size: 0.875rem; | |
| color: var(--bs-emphasis-color); | |
| } | |
| } | |
| .profileText { | |
| margin-left: 1rem; | |
| } | |
| .primaryText { | |
| font-weight: bold; | |
| color: var(--bs-emphasis-color, #000); | |
| @extend .reusable-text-ellipsis; /* NEW */ | |
| } | |
| .secondaryText { | |
| font-size: 0.9rem; | |
| color: var(--bs-secondary-text); | |
| @extend .reusable-text-ellipsis; /* NEW */ | |
| } | |
| /* Dropdown and options list */ | |
| .titleHeader { | |
| font-size: 1.2rem; | |
| color: var(--bs-secondary); | |
| line-height: 1.5; /* NEW */ | |
| margin: var(--spacing-md, 1rem) 0; /* NEW */ | |
| } | |
| /* Active and inactive buttons */ | |
| .activeButton, | |
| .inactiveButton { | |
| position: relative; | |
| transition: all 0.2s ease; | |
| &:active { | |
| transform: scale(0.98); | |
| } | |
| &.focus-visible { | |
| @extend .reusable-focus-visible; | |
| } | |
| } | |
| .activeButton { | |
| background-color: var(--grey-bg-color); | |
| color: black; | |
| &::before { | |
| @extend .reusable-indicator; /* NEW */ | |
| } | |
| &::after { | |
| @extend .reusable-arrow-indicator; /* NEW */ | |
| } | |
| } | |
| .activeButton:hover .arrow-indicator { | |
| transform: translateY(-50%) scale(1.1); | |
| opacity: 1; | |
| } | |
| .inactiveButton { | |
| background-color: transparent; | |
| color: var(--bs-emphasis-color, #000); | |
| &:hover { | |
| background-color: var(--grey-bg-color); | |
| } | |
| } | |
| /* Icon wrapper styles */ | |
| .iconWrapper { | |
| display: flex; | |
| align-items: center; | |
| margin-inline-end: 8px; | |
| margin-inline-start: 0; | |
| &[aria-label]:not([aria-label='']) { | |
| position: relative; | |
| &::after { | |
| content: attr(aria-label); | |
| position: absolute; | |
| inset-inline-start: 100%; | |
| background: var(--bs-dark); | |
| color: var(--bs-white); | |
| padding: 0.25rem 0.5rem; | |
| border-radius: 4px; | |
| font-size: 0.875rem; | |
| opacity: 0; | |
| visibility: hidden; | |
| transform: translateX(var(--transform-direction, 8px)); | |
| transition: all 0.2s ease; | |
| } | |
| &:hover::after { | |
| opacity: 1; | |
| visibility: visible; | |
| transform: translateX(0); | |
| } | |
| } | |
| } | |
| /* LeftDrawer general styles */ | |
| .leftDrawer { | |
| height: 100vh; /* Ensure it spans the full height */ | |
| overflow-y: auto; /* Enable vertical scrolling */ | |
| transition: transform 0.3s ease; | |
| will-change: transform; /* NEW */ | |
| position: fixed; /* Ensure it's positioned properly */ | |
| padding-bottom: 1rem; /* Prevent last item clipping */ | |
| /* Add overscroll behavior to prevent page scroll bleeding */ | |
| overscroll-behavior: contain; | |
| /* Improve scroll performance */ | |
| -webkit-overflow-scrolling: touch; | |
| } | |
| .hideElemByDefault { | |
| display: none; | |
| } | |
| .inactiveDrawer { | |
| transform: translateX(-100%); | |
| /* Use data attribute for aria hidden state */ | |
| &[data-hidden="true"] { | |
| visibility: hidden; | |
| } | |
| } | |
| .activeDrawer { | |
| transform: translateX(0); | |
| /* Use data attribute for aria hidden state */ | |
| &[data-hidden="false"] { | |
| visibility: visible; | |
| } | |
| } | |
| /* Rest of the CSS remains unchanged */ |
🧰 Tools
🪛 Biome (1.9.4)
[error] 3747-3747: Unknown property is not allowed.
See CSS Specifications and browser specific properties for more details.
To resolve this issue, replace the unknown property with a valid CSS property.
(lint/correctness/noUnknownProperty)
[error] 3752-3752: Unknown property is not allowed.
See CSS Specifications and browser specific properties for more details.
To resolve this issue, replace the unknown property with a valid CSS property.
(lint/correctness/noUnknownProperty)
There was a problem hiding this comment.
Replace --light-green with an equivalent blue/grey color
Use the color scheme here as a guide:
- https://www.figma.com/design/dmKt00m9GVSeA1nebnkxql/Untitled?node-id=0-1&node-type=canvas&t=TPzfh1DE9IxjzsEe-0
- Chore/css changes #2466
Also this is not a light green. You'll need to find a pre-existing blue color of a similar tone in the stylesheet
| font-weight: 600; | ||
| font-size: 20px; | ||
| margin-bottom: 30px; | ||
| padding-bottom: 5px; | ||
| border-bottom: 3px solid #31bb6b; | ||
| border-bottom: 3px solid var(--light-green); |
There was a problem hiding this comment.
- We are migrating away from the green theme as part of our support for color blind users.
- Take cues from other similar elements in this file and the PR mentioned in the issue.
| font-weight: 600; | ||
| font-size: 18px; | ||
| margin-bottom: 20px; | ||
| padding-bottom: 5px; | ||
| border-bottom: 3px solid #31bb6b; | ||
| border-bottom: 3px solid var(--light-green); |
There was a problem hiding this comment.
- We are migrating away from the green theme as part of our support for color blind users.
- Take cues from other similar elements in this file and the PR mentioned in the issue.
| @@ -615,7 +682,7 @@ hr { | |||
| .pageNotFound h1.head { | |||
| font-size: 250px; | |||
| font-weight: 900; | |||
| color: #31bb6b; | |||
| color: var(--light-green); | |||
There was a problem hiding this comment.
- We are migrating away from the green theme as part of our support for color blind users.
- Take cues from other similar elements in this file and the PR mentioned in the issue.
| @@ -695,7 +762,7 @@ hr { | |||
|
|
|||
| .inputFieldPledge { | |||
| background-color: var(--bs-white); | |||
| box-shadow: 0 1px 1px #31bb6b; | |||
| box-shadow: 0 1px 1px var(--light-green); | |||
There was a problem hiding this comment.
- We are migrating away from the green theme as part of our support for color blind users.
- Take cues from other similar elements in this file and the PR mentioned in the issue.
| @@ -861,7 +928,7 @@ hr { | |||
| } | |||
|
|
|||
| .toggleBtnPledge:hover { | |||
| color: #31bb6b !important; | |||
| color: var(--light-green) !important; | |||
There was a problem hiding this comment.
- We are migrating away from the green theme as part of our support for color blind users.
- Take cues from other similar elements in this file and the PR mentioned in the issue.
| @@ -1102,7 +1169,7 @@ hr { | |||
| } | |||
|
|
|||
| .customcell { | |||
| background-color: #31bb6b !important; | |||
| background-color: var(--light-green) !important; | |||
There was a problem hiding this comment.
- We are migrating away from the green theme as part of our support for color blind users.
- Take cues from other similar elements in this file and the PR mentioned in the issue.
| @@ -1564,15 +1631,15 @@ hr { | |||
| } | |||
|
|
|||
| .toggleBtn:hover { | |||
| color: #31bb6b !important; | |||
| color: var(--light-green) !important; | |||
There was a problem hiding this comment.
- We are migrating away from the green theme as part of our support for color blind users.
- Take cues from other similar elements in this file and the PR mentioned in the issue.
| @@ -1675,10 +1742,10 @@ input[type='radio']:checked + label:hover { | |||
|
|
|||
| .dropdowns { | |||
| background-color: var(--bs-white); | |||
| border: 1px solid #31bb6b; | |||
| border: 1px solid var(--light-green); | |||
There was a problem hiding this comment.
- We are migrating away from the green theme as part of our support for color blind users.
- Take cues from other similar elements in this file and the PR mentioned in the issue.
| position: relative; | ||
| display: inline-block; | ||
| color: #31bb6b; | ||
| color: var(--light-green); |
There was a problem hiding this comment.
- We are migrating away from the green theme as part of our support for color blind users.
- Take cues from other similar elements in this file and the PR mentioned in the issue.
| @@ -3314,7 +3381,7 @@ button[data-testid='createPostBtn'] { | |||
| display: flex !important; | |||
| align-items: center; | |||
| background-color: transparent; | |||
| color: #31bb6b; | |||
| color: var(--light-green); | |||
There was a problem hiding this comment.
- We are migrating away from the green theme as part of our support for color blind users.
- Take cues from other similar elements in this file and the PR mentioned in the issue.
|
Pull Request Details
Type of Change:
Refactoring
Issue Reference:
Fixes #2900
Description:
This PR improves the UI/UX of the Talawa-Admin by:
Making the application more accessible to color-blind users.
Streamlining all CSS into a single global file: src/style/app.module.css.
Summary of Changes:
Removed all embedded CSS from the relevant file.
Merged embedded CSS into the global app.module.css file.
Used pre-existing CSS patterns for consistency.
Summary by CodeRabbit
Summary by CodeRabbit
Refactor
Style
Chores