Refactor CSS Structure for EventVolunteers: Improved Maintainability and UI Consistency #2651
Conversation
WalkthroughThe changes in this pull request involve the complete removal of the Changes
Assessment against linked issues
Possibly related issues
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
|
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}
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #2651 +/- ##
=====================================================
- Coverage 95.82% 83.81% -12.01%
=====================================================
Files 295 312 +17
Lines 7037 8118 +1081
Branches 1520 1773 +253
=====================================================
+ Hits 6743 6804 +61
- Misses 98 1172 +1074
+ Partials 196 142 -54 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (12)
src/screens/EventVolunteers/VolunteerContainer.tsx (2)
Line range hint
18-18: Component name should follow React naming conventionsReact components should use PascalCase naming convention. Rename the function from
volunteerContainertoVolunteerContainerto align with React best practices.-function volunteerContainer(): JSX.Element { +function VolunteerContainer(): JSX.Element {
Line range hint
8-11: Consider extracting radio button options to a constantThe radio button options for individual/group/requests could be extracted to a constant array to improve maintainability and reusability.
+const VOLUNTEER_TYPES = [ + { id: 'individual', icon: HiUser, label: 'individuals' }, + { id: 'group', icon: HiUserGroup, label: 'groups' }, + { id: 'requests', icon: FaRegFile, label: 'requests' } +] as const;src/screens/EventVolunteers/VolunteerGroups/VolunteerGroupModal.tsx (2)
Line range hint
171-186: Consider using a form validation libraryThe form validation is currently handled manually. Consider using a form validation library like Formik or React Hook Form for more robust form handling and validation.
Line range hint
187-195: Improve error handling specificityThe catch block currently displays the raw error message. Consider handling specific error types and providing user-friendly error messages.
} catch (error: unknown) { - toast.error((error as Error).message); + if (error instanceof Error) { + if (error.message.includes('duplicate')) { + toast.error(t('groupNameExists')); + } else { + toast.error(t('errorCreatingGroup')); + } + } }src/screens/EventVolunteers/VolunteerGroups/VolunteerGroups.tsx (2)
Line range hint
27-31: Consider using a reducer for modal state managementThe current modal state management using multiple useState hooks could be simplified using useReducer for better state management and maintainability.
+type ModalAction = + | { type: 'OPEN_MODAL'; modalType: ModalState } + | { type: 'CLOSE_MODAL'; modalType: ModalState }; +function modalReducer(state: Record<ModalState, boolean>, action: ModalAction) { + switch (action.type) { + case 'OPEN_MODAL': + return { ...state, [action.modalType]: true }; + case 'CLOSE_MODAL': + return { ...state, [action.modalType]: false }; + default: + return state; + } +}
Line range hint
119-121: Consider extracting DataGrid columns configurationThe columns configuration is quite large and could be moved to a separate file to improve code organization and maintainability.
src/screens/EventVolunteers/Volunteers/Volunteers.tsx (2)
Line range hint
694-713: Consider enhancing error handling for volunteer data.While the DataGrid implementation is solid, consider adding error boundaries or fallback UI for cases where volunteer data might be malformed.
<DataGrid disableColumnMenu columnBufferPx={7} hideFooter={true} getRowId={(row) => row._id} + error={error} + components={{ + ErrorOverlay: () => ( + <Stack height="100%" alignItems="center" justifyContent="center"> + {t('errorLoadingVolunteers')} + </Stack> + ), + ...slots + }} slots={{ noRowsOverlay: () => ( <Stack height="100%" alignItems="center" justifyContent="center"> {t('noVolunteers')} </Stack> ), }}
Line range hint
776-789: Consider simplifying modal state management.The current implementation uses separate boolean flags for each modal. Consider using a single state variable with the ModalState enum for better state management.
-const [modalState, setModalState] = useState<{ - [key in ModalState]: boolean; -}>({ - [ModalState.ADD]: false, - [ModalState.DELETE]: false, - [ModalState.VIEW]: false, -}); +const [activeModal, setActiveModal] = useState<ModalState | null>(null); -const openModal = (modal: ModalState): void => { - setModalState((prevState) => ({ ...prevState, [modal]: true })); +const openModal = (modal: ModalState): void => { + setActiveModal(modal); }; -const closeModal = (modal: ModalState): void => { - setModalState((prevState) => ({ ...prevState, [modal]: false })); +const closeModal = (): void => { + setActiveModal(null); };src/style/app.module.css (3)
738-744: Consider using CSS variables for consistent colors.The dropdown styles use hardcoded color values. Consider using CSS variables for better maintainability and consistency.
.dropdown { background-color: white; - border: 1px solid #31bb6b; + border: 1px solid var(--primary-color); position: relative; display: inline-block; - color: #31bb6b; + color: var(--primary-color); }
766-774: Consolidate status-related colors into CSS variables.The active and pending status colors are hardcoded. Consider using CSS variables for better maintainability.
.active { - background-color: #31bb6a50 !important; + background-color: var(--status-active-bg) !important; } .pending { - background-color: #ffd76950 !important; - color: #bb952bd0 !important; - border-color: #bb952bd0 !important; + background-color: var(--status-pending-bg) !important; + color: var(--status-pending-color) !important; + border-color: var(--status-pending-border) !important; }
818-821: Consider using CSS Grid for modal table layout.The modal table uses fixed height and overflow. Consider using CSS Grid for a more flexible and responsive layout.
.modalTable { - max-height: 220px; - overflow-y: auto; + display: grid; + grid-template-rows: auto minmax(0, 220px); + overflow-y: auto; }src/screens/EventVolunteers/VolunteerGroups/VolunteerGroupViewModal.tsx (1)
Line range hint
41-196: Consider architectural improvements for better maintainability.While not directly related to the CSS changes, consider these improvements:
The component mixes Material-UI and React-Bootstrap components, which could lead to:
- Inconsistent styling and behavior
- Increased bundle size
- Maintenance complexity
Consider enhancing keyboard navigation by:
- Adding
role="dialog"- Implementing focus trap
- Adding escape key handler
Would you like me to provide a code example implementing these improvements?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/screens/EventVolunteers/EventVolunteers.module.css(0 hunks)src/screens/EventVolunteers/Requests/Requests.tsx(1 hunks)src/screens/EventVolunteers/VolunteerContainer.tsx(1 hunks)src/screens/EventVolunteers/VolunteerGroups/VolunteerGroupDeleteModal.tsx(1 hunks)src/screens/EventVolunteers/VolunteerGroups/VolunteerGroupModal.tsx(1 hunks)src/screens/EventVolunteers/VolunteerGroups/VolunteerGroupViewModal.tsx(1 hunks)src/screens/EventVolunteers/VolunteerGroups/VolunteerGroups.tsx(1 hunks)src/screens/EventVolunteers/Volunteers/VolunteerCreateModal.tsx(1 hunks)src/screens/EventVolunteers/Volunteers/VolunteerDeleteModal.tsx(1 hunks)src/screens/EventVolunteers/Volunteers/VolunteerViewModal.tsx(1 hunks)src/screens/EventVolunteers/Volunteers/Volunteers.tsx(1 hunks)src/screens/UserPortal/Volunteer/Groups/GroupModal.tsx(1 hunks)src/screens/UserPortal/Volunteer/Groups/Groups.tsx(1 hunks)src/style/app.module.css(1 hunks)
💤 Files with no reviewable changes (1)
- src/screens/EventVolunteers/EventVolunteers.module.css
✅ Files skipped from review due to trivial changes (7)
- src/screens/EventVolunteers/Volunteers/VolunteerCreateModal.tsx
- src/screens/EventVolunteers/Volunteers/VolunteerDeleteModal.tsx
- src/screens/EventVolunteers/VolunteerGroups/VolunteerGroupDeleteModal.tsx
- src/screens/EventVolunteers/Volunteers/VolunteerViewModal.tsx
- src/screens/UserPortal/Volunteer/Groups/GroupModal.tsx
- src/screens/EventVolunteers/Requests/Requests.tsx
- src/screens/UserPortal/Volunteer/Groups/Groups.tsx
🔇 Additional comments (2)
src/screens/EventVolunteers/Volunteers/Volunteers.tsx (1)
23-23: LGTM: Import path updated correctly.
The import statement has been properly updated to reference the consolidated global CSS file, aligning with the PR's objective of CSS restructuring.
src/screens/EventVolunteers/VolunteerGroups/VolunteerGroupViewModal.tsx (1)
3-3: Verify CSS classes in the new global stylesheet.
The import path change aligns with the PR objective of consolidating CSS files. However, we should verify that all required CSS classes are properly transferred to the global stylesheet.
Let's verify the CSS classes used in this component:
✅ Verification successful
CSS import path change is safe and all required classes are present
The verification confirms that all CSS classes used in the component are properly defined in the global stylesheet (src/style/app.module.css). Specifically:
.titlemodal,.modalCloseBtn,.noOutline,.TableImage,.avatarContainer,.imageContainer, and.modalTableare all present in the global CSS file- These classes are consistently used across multiple components, indicating proper shared styling
- The only missing class
volunteerCreateModalis actually used in this component and two other volunteer-related modals, suggesting it's a deliberate shared modal style
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify CSS classes exist in the global CSS file and check for potential conflicts
# Check if all required CSS classes exist in the global CSS file
echo "Checking for required CSS classes in global CSS file..."
REQUIRED_CLASSES=(
"volunteerCreateModal"
"titlemodal"
"modalCloseBtn"
"noOutline"
"TableImage"
"avatarContainer"
"imageContainer"
"modalTable"
)
for class in "${REQUIRED_CLASSES[@]}"; do
echo "Checking for .$class..."
rg "\\.$class\\s*{" "src/style/app.module.css"
done
# Check for potential naming conflicts across components
echo -e "\nChecking for potential CSS class naming conflicts..."
for class in "${REQUIRED_CLASSES[@]}"; do
echo "Checking usage of .$class across components..."
rg "className=\{.*$class.*\}" --type ts --type tsx
done
Length of output: 4443
Script:
#!/bin/bash
# Let's fix the regex issues and file type recognition
# First, check if the global CSS file exists
echo "Checking if global CSS file exists..."
cat src/style/app.module.css
# Then search for CSS classes with fixed regex
echo -e "\nChecking for CSS classes..."
rg -l "\.volunteerCreateModal" src/style/app.module.css
rg -l "\.titlemodal" src/style/app.module.css
rg -l "\.modalCloseBtn" src/style/app.module.css
rg -l "\.noOutline" src/style/app.module.css
rg -l "\.TableImage" src/style/app.module.css
rg -l "\.avatarContainer" src/style/app.module.css
rg -l "\.imageContainer" src/style/app.module.css
rg -l "\.modalTable" src/style/app.module.css
# Check component usage with fixed file types
echo -e "\nChecking component usage..."
rg "className.*volunteerCreateModal" "src/"
rg "className.*titlemodal" "src/"
rg "className.*modalCloseBtn" "src/"
rg "className.*noOutline" "src/"
rg "className.*TableImage" "src/"
rg "className.*avatarContainer" "src/"
rg "className.*imageContainer" "src/"
rg "className.*modalTable" "src/"
Length of output: 32276
|
@gurramkarthiknetha Please fix the title of the PR to reflect what the PR is doing. Something doesn't seem right here, as its only running one check. Please make sure all the tests run, pass their tests, and there are not conflicting files. |
|
Please resolve the conflicting files. Hopefully if that gets resolved the rest of the tests will run |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
src/style/app.module.css (6)
283-284: Remove commented-out CSS propertiesThe properties
width: 25px !important;andheight: 25px !important;are commented out. If they're no longer needed, consider removing them to keep the code clean and maintainable.
526-528: Minimize the use of!importantin your CSSUsing
!importantcan make the CSS harder to maintain and override, as it increases specificity and can lead to unexpected behavior. Consider refactoring the styles to reduce reliance on!importantdeclarations.For example, in the following selectors:
/* Line 527 */ .toggleBtn:hover { - color: #31bb6b !important; + color: #31bb6b; } /* Line 531 */ input[type='radio']:checked + label { - background-color: #31bb6a50 !important; + background-color: #31bb6a50; } /* Line 535 */ input[type='radio']:checked + label:hover { - color: black !important; + color: black; } /* Line 673 */ .chipIcon { - height: 0.9rem !important; + height: 0.9rem; } /* Line 681 */ .active { - background-color: #31bb6a50 !important; + background-color: #31bb6a50; } /* Lines 685-687 */ .pending { - background-color: #ffd76950 !important; - color: #bb952bd0 !important; - border-color: #bb952bd0 !important; + background-color: #ffd76950; + color: #bb952bd0; + border-color: #bb952bd0; }This will make your CSS more maintainable and easier to override when necessary.
Also applies to: 531-532, 535-536, 673-674, 681-682, 685-687
553-571: Consolidate duplicate and conflicting CSS propertiesIn the
.greenregbtnclass, there are duplicate or conflicting properties that can be simplified:
Duplicate
widthproperties (Lines 561 and 570):Remove the second
width: 100%;declaration.background-color: var(--bs-primary); width: 100%; font-size: 16px; color: var(--bs-white); outline: none; font-weight: 600; cursor: pointer; transition: transform 0.2s, box-shadow 0.2s;
- width: 100%;
2. **Conflicting `margin` properties (Lines 554 and 555):** There is a conflict between `margin: 1rem 0 0;` and `margin-top: 15px;`. Choose one to accurately reflect the desired spacing. ```diff margin: 1rem 0 0; - margin-top: 15px; border: 1px solid var(--bs-gray-300);
605-605: Remove unnecessary commented-out codeThere are commented-out CSS properties that can be removed if they're no longer needed. Cleaning up commented code improves readability.
Line 605 (
.btnsContainerclass):- /* margin: 0.5rem 0 1.5rem 0; */Lines 283-284 (Commented-out widths in an unspecified selector):
- /* width: 25px !important; */ - /* height: 25px !important; */Also applies to: 283-284
607-607: Remove duplicatemargindeclaration in.btnsContainerThe
margin: 2.5rem 0;property is declared twice in the.btnsContainerclass. Remove the duplicate to avoid confusion and maintain clean code.display: flex; margin: 2.5rem 0; align-items: center; gap: 10px; /* Adjust spacing between items */ - margin: 2.5rem 0;Also applies to: 611-611
640-645: Use CSS variables instead of hardcoded color valuesFor consistency and easier theming, consider replacing hardcoded color values with existing CSS variables. This will make it easier to maintain and update colors across the application.
Examples:
Line 527 (
.toggleBtn:hover):.toggleBtn:hover {color: #31bb6b !important;
color: var(--bs-primary) !important;
Line 644 (
.inputField):.inputField { margin-top: 10px; margin-bottom: 10px; background-color: white;box-shadow: 0 1px 1px #31bb6b;
box-shadow: 0 1px 1px var(--bs-primary);
Line 657 (
.dropdown):.dropdown { background-color: white; border: 1px solid #31bb6b; position: relative; display: inline-block;color: #31bb6b;
color: var(--bs-primary);
Line 681 (
.active):.active {background-color: #31bb6a50 !important;
background-color: rgba(var(--bs-primary-rgb), 0.5) !important;
Lines 685-687 (
.pending):.pending {background-color: #ffd76950 !important;color: #bb952bd0 !important;border-color: #bb952bd0 !important;
background-color: rgba(var(--custom-warning-bg), 0.5) !important;color: var(--custom-warning-text) !important;border-color: var(--custom-warning-border) !important;
Line 698 (
.titlemodal):.titlemodal {color: #707070;
color: var(--bs-gray-600); font-weight: 600; font-size: 32px;Ensure that the variables like
--bs-primary,--bs-gray-600, and any new custom variables (--custom-warning-bg, etc.) are defined in your CSS.Also applies to: 527-527, 644-644, 657-657, 681-682, 685-687, 698-698
📜 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 (1)
src/style/app.module.css (1)
722-723: Verify the aspect ratio of .avatarContainer dimensions
The .avatarContainer class has a width of 28px and a height of 26px. If the avatar images are intended to be square, consider adjusting the dimensions to maintain a consistent aspect ratio.
Please confirm if the current dimensions are intentional or if they should be updated to make the avatar images appear proportionate.
gurramkarthiknetha
changed the title
thank you for you suggestion @Cioppolo14 |
|
Please fix the conflicting file |
Ok @palisadoes I will fix tonight |
What kind of change does this PR introduce?
Feature/Refactoring
Issue Number:
Fixes #2505
Did you add tests for your changes?
Yes
Snapshots/Videos:
N/A
If relevant, did you update the documentation?
No
Summary
This PR refactors the CSS structure for the
EventVolunteersand its related components. The changes involve merging all CSS files from the subdirectory and related components into the global CSS file. After the merge, the old CSS files were deleted, and the components were updated to reference the global CSS. This improves maintainability and adheres to the updated design patterns.Does this PR introduce a breaking change?
No, these changes only refactor the existing CSS structure.
Other information
Testing was completed to ensure no UI regressions. All screens were verified for consistency with the updated global CSS styles.
Have you read the contributing guide?
Yes
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Style