Refactor CSS for LeftDrawerOrg #3131
Conversation
WalkthroughThis pull request focuses on refactoring the CSS for the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
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
|
![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 (5)
src/components/LeftDrawerOrg/LeftDrawerOrg.tsx (1)
128-131: Shimmer state distinguished well.
The placeholder classshimmeris a neat approach for the loading state. Ensure you have an accessible equivalent for screen-readers (e.g., aria-busy).src/style/app.module.css (4)
3711-3721: Duplicate.reusable-focus-visible.
Consider reorganizing the repeated definitions to avoid collisions in the final compiled CSS.
3754-3759:.inactiveDrawertransitions.
Smooth sliding effect withtransform: translateX(-100%). The code snippet for[data-hidden='true']is empty. Possibly a placeholder.
3760-3766:.activeDrawertransitions.
Ensures the drawer slides in cleanly. The[data-hidden='false']is also a placeholder but nice for future expansions.
3807-3809:.profileText
Margin-left for spacing. Confirm it doesn’t break the layout in RTL mode, or rely on logical properties.
📜 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(6 hunks)src/style/app.module.css(1 hunks)
💤 Files with no reviewable changes (1)
- src/components/LeftDrawerOrg/LeftDrawerOrg.module.css
🔇 Additional comments (33)
src/components/LeftDrawerOrg/LeftDrawerOrg.tsx (9)
14-14: Global styling import recognized.
Importing fromapp.module.cssinstead of a locally scoped CSS file is consistent with the PR objective of consolidating CSS.
34-34: Function signature clarity.
The parameter list and usage are clear. The docstring above it is consistent and helpful for future reference.
49-49: Comment readability.
Though short, the comment effectively clarifies the intent behind indexing logic.
51-51: Safety check not performed.
You return an empty string ifsegments.lengthis less than 3. If there's a possibility of an even shorter path or an unexpected format, consider logging or gracefully handling the scenario.
72-72: UseMemo usage is appropriate.
This is a good usage ofuseMemoto avoid repeatedly executing the parsing logic.
78-78: Cleanup handling.
Nice use of a boolean flag within the effect to avoid setting state after unmount.
135-135: Red danger background usage.
Usingstyles.bgDangerreferences the new utility class. This is consistent with the approach to unify all CSS.
179-182: Active vs. inactive state logic.
Smooth usage ofisActiveto manage variant and class name. This aligns with the standardized approach of.activeButtonand.inactiveButton.
188-188: Dynamic icon color.
Conditionally applying the color via'var(--bs-black)'or'var(--bs-secondary)'is correct. Make sure these variables align well with your dark-mode or theme overrides.src/style/app.module.css (24)
3631-3637: Introduce.reusable-text-ellipsis.
A concise utility for truncating text, ensuring improved readability for limited-width containers.
3638-3647: Introduce.reusable-indicator.
Positions a colored indicator bar on the left side of the element. Good for highlighting states or statuses.
3648-3660: Introduce.reusable-arrow-indicator.
Applying'▶'with positioning for directional feedback is a clean, minimal solution.
3661-3665: Introduce.reusable-focus-visible.
Enhances keyboard accessibility by providing a visible outline on focus.
3666-3672: Reduced motion preference check.
Good job disabling transitions and animations whenprefers-reduced-motionis enabled. This is inclusive and accessible.
3673-3679: RTL support (repeat).
Positioning transformations are reversed for RTL. This ensures correct alignment for right-to-left interfaces.
3680-3686: Duplicate.reusable-text-ellipsis.
This block appears to be repeated lines. Ensure it won’t create merge conflicts or duplication in your final CSS.
3697-3710: Duplicate.reusable-arrow-indicator.
Again, repeated lines. Confirm if they differ in ways that might cause unintended side effects.
3722-3728: Duplicate RTL support.
Check if you intend to define this twice or if it’s a merge artifact.
3729-3741: Enhanced.leftDrawerstyles.
Height set to100vhwith overflow-y scroll ensures a stable layout. The transitions andwill-changeproperty help with rendering performance.
3742-3753:.hideElemByDefault& data-hidden approach.
The data attributes are a flexible way to toggle visibility. Just ensure the conditions are well-synchronized in the React code.
3767-3778:.organizationContainerand.profileContainer.
Proper spacing and alignment. Use of focus-visible approach is a plus for accessibility.
3779-3782: Chained SCSS or placeholder usage.
@extend .reusable-focus-visible;implies SCSS usage or Sass placeholder. Make sure your build pipeline supports it.
3783-3786:.bg-dangershort usage.
Reuses the existing--delete-button-color. Straightforward naming that indicates its intention.
3787-3794:.text-start&.text-white.
Helpful utility classes. They mirror Bootstrap's pattern. Keep them from clashing with the .bootstrap classes.
3796-3805:.imageContainer img
Applying consistent sizing and alt text styling. This can help maintain a uniform avatar size throughout the UI.
3811-3815:.primaryText
Extending.reusable-text-ellipsisensures the text truncates gracefully. Looks consistent and prevents overflow.
3817-3821:.secondaryText
Mirrors.primaryTextwith slightly smaller font. Sufficient for showing subtitles or less important info. Good approach.
3823-3829:.titleHeader
Clear usage for headings with bigger font-size. Theline-heightand margin variables keep it well spaced.
3831-3844:.activeButton&.inactiveButton
Having them share transitions but differ in background color is good. The usage of pseudo-elements via@extendfor indicators is neat.
3846-3857: Hover effect on.activeButton
.activeButton:hover .arrow-indicatorshows fancy scaling. Make sure.arrow-indicatoris properly referenced in HTML or SCSS.
3859-3863:.inactiveButtonhover
Provides a color fill on hover. It's consistent with the.activeButtonapproach and respects design.
3864-3871:.iconWrapperdynamic tooltip
A slick approach for accessible tooltips. Confirm that screen readers handle these well, or consider ARIA roles for extended accessibility.
3873-3905: Mass additions from 3873 onward
This block primarily adds or modifies utility classes, ensuring consistent styling across large components (e.g., toggles, popups). Overall this standardizes the UI with minimal repeated rules.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #3131 +/- ##
=====================================================
+ Coverage 21.89% 89.51% +67.62%
=====================================================
Files 301 322 +21
Lines 7686 8454 +768
Branches 1679 1902 +223
=====================================================
+ Hits 1683 7568 +5885
+ Misses 5897 655 -5242
- Partials 106 231 +125 ☔ View full report in Codecov by Sentry. |
|
Please change the title of the PR to be correctly searchable |
|
Fix the failing test |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/style/app.module.css (6)
Line range hint
1-101: Improve CSS variable naming consistency.Consider standardizing the naming convention for color variables. Currently, there are inconsistencies:
- Some use "grey" while others use "gray"
- Some use descriptive names while others use functional names
Examples to standardize:
- --grey-light: #eaebef; - --gray-blue: #1e293b; + --gray-100: #eaebef; + --gray-700: #1e293b;
3631-3642: Optimize drawer performance and touch handling.The drawer implementation shows good use of
will-changeandtransform, but consider these additional optimizations:.leftDrawer { height: 100vh; overflow-y: auto; transition: transform 0.3s ease; will-change: transform; position: fixed; padding-bottom: 1rem; overscroll-behavior: contain; -webkit-overflow-scrolling: touch; + /* Prevent layout shifts during animation */ + backface-visibility: hidden; + /* Improve touch handling */ + touch-action: pan-y pinch-zoom; + /* Use GPU acceleration */ + transform: translateZ(0); }
3686-3696: Improve utility class modularity and specificity.The utility classes could be more modular and follow a consistent pattern:
- .bg-danger { - background-color: var(--delete-button-color); - } - .text-start { - text-align: start; - } - .text-white { - color: var(--bs-white); - } + /* Use a more systematic approach */ + .u-bg-danger { + background-color: var(--delete-button-color) !important; + } + .u-text-start { + text-align: start !important; + } + .u-text-white { + color: var(--bs-white) !important; + }
3775-3806: Enhance accessibility features.The icon wrapper implementation has good ARIA support, but consider these accessibility improvements:
.iconWrapper { &[aria-label]:not([aria-label='']) { &::after { content: attr(aria-label); background: var(--bs-dark); color: var(--bs-white); + /* Ensure sufficient color contrast */ + background: var(--bs-gray-900); + /* Improve visibility */ + box-shadow: 0 2px 4px rgba(0,0,0,0.2); + /* Add screen reader only text */ + @media screen and (forced-colors: active) { + outline: 2px solid currentColor; + } } } }
3733-3773: Optimize animation performance.The button animations could be optimized to prevent layout thrashing:
.activeButton, .inactiveButton { position: relative; - transition: all 0.2s ease; + /* Only animate opacity and transform to avoid layout recalculation */ + transition: opacity 0.2s ease, transform 0.2s ease; + /* Promote to GPU layer */ + transform: translateZ(0); + /* Hint browser about changes */ + will-change: opacity, transform; &:active { transform: scale(0.98); } }
3644-3654: Standardize responsive breakpoints.Consider using CSS custom properties for breakpoints to maintain consistency:
+:root { + --breakpoint-sm: 576px; + --breakpoint-md: 768px; + --breakpoint-lg: 992px; + --breakpoint-xl: 1200px; +} .hideElemByDefault { display: none; } -[data-hidden='true'] { +@media (max-width: var(--breakpoint-md)) { + [data-hidden='true'] { visibility: hidden; + } } -[data-hidden='false'] { +@media (max-width: var(--breakpoint-md)) { + [data-hidden='false'] { visibility: visible; + } }
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
Style Changes
Refactor