Refactored css in src/screens/OrganizationPeople/OrganizationPeople.tsx #2978
Conversation
WalkthroughThis pull request focuses on refactoring CSS in 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: 0
🧹 Nitpick comments (4)
src/style/app.module.css (4)
Line range hint
5-25: LGTM! Consider adding fallback values for CSS variables.The new CSS variables are well-organized and follow consistent naming conventions. The
--grey-border-box-colorvariable will help maintain consistency across the application.Consider adding fallback values for all CSS variables similar to how
--table-head-bghas a fallback value ofblue. This ensures graceful degradation if a variable is undefined::root { - --grey-border-box-color: #e8e5e5; + --grey-border-box-color: var(--bs-gray-300, #e8e5e5); - --loader-size: 10em; + --loader-size: var(--custom-loader-size, 10em); - --loader-border-width: 1.1em; + --loader-border-width: var(--custom-loader-border-width, 1.1em); - --loader-color: #febc59; + --loader-color: var(--custom-loader-color, #febc59); }
968-968: Continue replacing hardcoded colors with CSS variables.Good progress on replacing hardcoded colors with CSS variables. However, there are still several instances where colors are hardcoded.
Consider creating and using CSS variables for commonly used colors:
:root { + /* Add new color variables */ + --color-primary: #31bb6b; + --color-text-secondary: #707070; + --color-success: #23864c; } /* Example replacements */ -.customcell { - color: var(--bs-white) !important; + color: var(--bs-white, #fff) !important; } -.greenregbtnOrganizationFundCampaign { - color: var(--bs-white); + color: var(--bs-white, #fff); } -.memberfontcreatedbtn { - background-color: #eaebef; + background-color: var(--grey-bg-color, #eaebef); }Also applies to: 1021-1021, 1051-1051, 1476-1476, 1884-1884, 2075-2075, 2262-2262, 2608-2608, 2981-2981
613-613: Create shared variables for box shadows.Good use of
--grey-border-box-colorfor borders and shadows. However, box shadow patterns are repeated across components.Consider creating shared variables for common box shadow patterns:
:root { + /* Add box shadow variables */ + --shadow-sm: 0 2px 2px var(--grey-border-box-color, #e8e5e5); + --shadow-md: 0 3px 5px var(--bs-gray-400, #c9c9c9); + --shadow-lg: 0 0.5rem 1rem rgb(0 0 0 / 0.15); } .greenregbtn { - box-shadow: 0 2px 2px var(--grey-border-box-color); + box-shadow: var(--shadow-sm); } .invitebtn { - box-shadow: 0 2px 2px var(--grey-border-box-color); + box-shadow: var(--shadow-sm); }Also applies to: 1015-1015, 1045-1046, 1471-1472, 1879-1880, 1965-1966, 2445-2446, 2975-2976
Line range hint
3051-3196: Create shared variables for animations.The animations are well-structured with proper vendor prefixes. However, animation durations and timing functions are hardcoded.
Consider creating shared variables for animation properties:
:root { + /* Add animation variables */ + --animation-duration-fast: 0.3s; + --animation-duration-normal: 0.5s; + --animation-timing-ease: ease-in-out; } .activeDrawer { - animation: comeToRightBigScreen 0.5s ease-in-out; + animation: comeToRightBigScreen var(--animation-duration-normal) var(--animation-timing-ease); } .inactiveDrawer { - animation: goToLeftBigScreen 0.5s ease-in-out; + animation: goToLeftBigScreen var(--animation-duration-normal) var(--animation-timing-ease); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/screens/OrganizationPeople/OrganizationPeople.tsx(1 hunks)src/style/app.module.css(29 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/screens/OrganizationPeople/OrganizationPeople.tsx
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #2978 +/- ##
=====================================================
+ Coverage 38.68% 88.50% +49.81%
=====================================================
Files 299 320 +21
Lines 7427 8288 +861
Branches 1624 1869 +245
=====================================================
+ Hits 2873 7335 +4462
+ Misses 4337 731 -3606
- Partials 217 222 +5 ☔ View full report in Codecov by Sentry. |
palisadoes
merged commit 2327a4b
into
PalisadoesFoundation:develop-postgres
What kind of change does this PR introduce?
Refactoring
Issue Number:
Fixes #2890
Did you add tests for your changes?
No
Snapshots/Videos:
Regarding the file... there was no in file styles and the style sheet of this particular file was already refactored into
src/style/app.module.cssThere are certain things that I mentioned in the Issue #2890 as well for the changes I made... I'll mention them here as well
we have sx prop for styling inside the react components like this:
For better consistency since we are already importing the styles and applying the classes... we can also apply the root element vars to the components ensuring further consistency like this and result would be the same:
I mentioned one more example in the issue for further context.
If relevant, did you update the documentation?
No
Summary
Does this PR introduce a breaking change?
No
Other information
I also made essentially 2 changes in the main CSS file for better consistency with the global vars:
color: white;with--bs-white: #fffandcolor:var(--bs-white).--grey-border-box-color: #e8e5e5;as #e8e5e5 was repeated many times for the border and box shadows and there was no variable for that and replaced#e8e5e5withvar(--grey-border-box-color)like inborder-color: var(--grey-border-box-color)There could be further some changes that can be made but I want to confirm about such these changes first are they good to be made?
Have you read the contributing guide?
Yes
Summary by CodeRabbit
New Features
DataGridcomponent for improved visual consistency.Bug Fixes
Refactor