Organization dash cards tests migration (fixes : #2806) #2881
Conversation
…ashCards component and also merged css files into global css file
…ashCards component and also merged css files into global css file
|
Warning Rate limit exceeded@amaan-aly246 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 34 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request involves a comprehensive refactoring of CSS and component files within the Organization Dashboard components. The changes primarily focus on consolidating CSS styles from multiple module files into a single global Changes
Assessment against linked 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: 2
🧹 Nitpick comments (2)
src/style/app.module.css (2)
804-827: Consider using relative units for typographyThe mobile styles look good, but consider using relative units (rem/em) consistently for better scalability and maintainability.
.cardBody .textWrapper .primaryText { - font-size: 1.5rem; + font-size: clamp(1.25rem, 4vw, 1.5rem); } .cardBody .textWrapper .secondaryText { - font-size: 1rem; + font-size: clamp(0.875rem, 3vw, 1rem); }
998-1012: Improve color management and layout flexibilityThe styles can be improved for better maintainability and flexibility:
- Replace hardcoded color with CSS variable
- Use more flexible width constraints
.cardItem .creator { font-size: 1rem; - color: rgb(33, 208, 21); + color: var(--bs-success, #198754); } .rightCard { display: flex; gap: 7px; - min-width: 170px; + min-width: min(170px, 30%); justify-content: center; flex-direction: column; margin-left: 10px; overflow-x: hidden; - width: 210px; + width: clamp(170px, 30%, 210px); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/components/OrganizationDashCards/CardItem.module.css(0 hunks)src/components/OrganizationDashCards/CardItem.spec.tsx(1 hunks)src/components/OrganizationDashCards/CardItem.tsx(1 hunks)src/components/OrganizationDashCards/CardItemLoading.spec.tsx(1 hunks)src/components/OrganizationDashCards/CardItemLoading.tsx(2 hunks)src/components/OrganizationDashCards/DashBoardCardLoading.spec.tsx(1 hunks)src/components/OrganizationDashCards/DashboardCard.spec.tsx(1 hunks)src/components/OrganizationDashCards/DashboardCard.tsx(1 hunks)src/components/OrganizationDashCards/DashboardCardLoading.tsx(2 hunks)src/components/OrganizationDashCards/Dashboardcard.module.css(0 hunks)src/style/app.module.css(2 hunks)
💤 Files with no reviewable changes (2)
- src/components/OrganizationDashCards/Dashboardcard.module.css
- src/components/OrganizationDashCards/CardItem.module.css
✅ Files skipped from review due to trivial changes (4)
- src/components/OrganizationDashCards/DashboardCard.spec.tsx
- src/components/OrganizationDashCards/CardItem.spec.tsx
- src/components/OrganizationDashCards/DashboardCard.tsx
- src/components/OrganizationDashCards/CardItem.tsx
🔇 Additional comments (14)
src/components/OrganizationDashCards/CardItemLoading.tsx (3)
2-2: Import path successfully updated to the global CSS.
This change aligns with the PR goal of consolidating styling into a single app.module.css file.
Line range hint 8-29: Component name refactored to PascalCase.
The use of PascalCase (CardItemLoading) is consistent with React best practices for component naming. Code looks concise and correct.
31-31: Export statement correctly reflects the updated component name.
This ensures consistency with the named function and prevents any potential import issues.
src/components/OrganizationDashCards/CardItemLoading.spec.tsx (3)
1-4: Imports are properly set up for new test file.
It references the updated CardItemLoading component and necessary testing libraries.
5-9: Test suite confirms component rendering.
The test checks for the cardItemLoading test ID, verifying that the component mounts successfully.
11-23: Thorough child elements test coverage.
This test verifies all key child elements (iconWrapper, title, etc.) are present, ensuring UI integrity.
src/components/OrganizationDashCards/DashboardCardLoading.tsx (4)
4-4: Global stylesheet import is consistent.
Switching to app.module.css follows the consolidation strategy described in the PR.
10-10: Component name update to PascalCase.
This naming convention aligns with standard React patterns and ensures clarity.
12-12: Added data-testid attribute for improved testability.
Facilitates targeted queries in the new test suite.
40-40: Default export matches the updated PascalCase name.
Maintaining a consistent name prevents confusion when importing.
src/components/OrganizationDashCards/DashBoardCardLoading.spec.tsx (4)
1-4: Imports set up correctly for new test suite.
All required dependencies, including local styles, are in place.
5-8: Pre-render in beforeEach ensures each test starts fresh.
This pattern keeps tests isolated and reliable.
9-11: Test verifies main component renders properly.
Asserting the presence of data-testid confirms the component’s visibility.
13-27: Comprehensive coverage of child elements.
Ensures each sub-element (e.g., cardBody, iconWrapper, themeOverlay) is rendered, confirming UI integrity.
|
Please resolve the conflicting file |
amaan-aly246
dismissed stale reviews from palisadoes and coderabbitai[bot]
via
cb432e7
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/style/app.module.css (1)
2016-2087: Use CSS variables for color consistencyThe creator text color is hardcoded to
rgb(33, 208, 21). For better maintainability and theme consistency, consider using a CSS variable.Apply this diff to use a CSS variable:
.cardItem .creator { font-size: 1rem; - color: rgb(33, 208, 21); + color: var(--bs-success, rgb(33, 208, 21)); }
📜 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)
1624-1647: LGTM! Well-implemented responsive design.
The media query for small screens effectively adjusts the card layout and typography for better mobile display. The absolute positioning of icons and text alignment changes provide an optimal viewing experience on smaller devices.
1649-1683: Ensure sufficient color contrast for accessibility
The secondary text color uses a CSS variable without a fallback value. Consider adding a fallback and ensuring the contrast ratio meets WCAG guidelines.
amaan-aly246
force-pushed
the
OrganizationDashCards_tests-migration
branch
from
cb432e7 to
fa7f183
Compare
amaan-aly246
force-pushed
the
OrganizationDashCards_tests-migration
branch
from
ee9cce1 to
e77e371
Compare
amaan-aly246
force-pushed
the
OrganizationDashCards_tests-migration
branch
from
aae108f to
daf829d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #2881 +/- ##
=====================================================
+ Coverage 49.35% 88.13% +38.77%
=====================================================
Files 299 316 +17
Lines 7414 8265 +851
Branches 1621 1866 +245
=====================================================
+ Hits 3659 7284 +3625
+ Misses 3509 770 -2739
+ Partials 246 211 -35 ☔ View full report in Codecov by Sentry. |
|
Please fix the conflicting file |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/style/app.module.css (1)
1624-1647: Consider improving mobile layout robustness.While the responsive design handles text sizing well, the absolute positioning of
.iconWrappercould cause layout issues.Apply this diff to make the layout more robust:
@media (max-width: 600px) { .cardBody { min-height: 120px; + position: relative; } .cardBody .iconWrapper { position: absolute; top: 1rem; left: 1rem; + z-index: 1; } .cardBody .textWrapper { - margin-top: calc(0.5rem + 36px); + margin-top: calc(0.5rem + 48px); text-align: right; + position: relative; + z-index: 0; } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/components/OrganizationDashCards/CardItem.module.css(0 hunks)src/components/OrganizationDashCards/CardItem.spec.tsx(1 hunks)src/components/OrganizationDashCards/CardItem.tsx(4 hunks)src/components/OrganizationDashCards/CardItemLoading.spec.tsx(1 hunks)src/components/OrganizationDashCards/CardItemLoading.tsx(2 hunks)src/components/OrganizationDashCards/DashBoardCardLoading.spec.tsx(1 hunks)src/components/OrganizationDashCards/DashboardCard.spec.tsx(1 hunks)src/components/OrganizationDashCards/DashboardCard.tsx(1 hunks)src/components/OrganizationDashCards/DashboardCardLoading.tsx(2 hunks)src/components/OrganizationDashCards/Dashboardcard.module.css(0 hunks)src/style/app.module.css(2 hunks)
💤 Files with no reviewable changes (2)
- src/components/OrganizationDashCards/Dashboardcard.module.css
- src/components/OrganizationDashCards/CardItem.module.css
🚧 Files skipped from review as they are similar to previous changes (8)
- src/components/OrganizationDashCards/DashboardCard.spec.tsx
- src/components/OrganizationDashCards/DashboardCard.tsx
- src/components/OrganizationDashCards/CardItem.spec.tsx
- src/components/OrganizationDashCards/CardItemLoading.spec.tsx
- src/components/OrganizationDashCards/CardItemLoading.tsx
- src/components/OrganizationDashCards/CardItem.tsx
- src/components/OrganizationDashCards/DashboardCardLoading.tsx
- src/components/OrganizationDashCards/DashBoardCardLoading.spec.tsx
🔇 Additional comments (1)
src/style/app.module.css (1)
1649-1683: 🛠️ Refactor suggestion
Remove duplicate overlay styles and improve accessibility.
- The
.themeOverlayclass duplicates the overlay styles from.cardItem .iconWrapper .themeOverlay. - The secondary text color might not meet WCAG contrast requirements.
Apply these changes:
- Remove duplicate overlay styles:
- .cardBody .iconWrapper .themeOverlay {
- background: var(--bs-primary);
- position: absolute;
- top: 0;
- right: 0;
- bottom: 0;
- left: 0;
- opacity: 0.12;
- border-radius: 50%;
- }- Improve text contrast:
.cardBody .textWrapper .secondaryText {
font-size: 14px;
display: block;
- color: var(--bs-secondary);
+ color: var(--bs-secondary, #6c757d);
+ /* Ensure color meets WCAG AA contrast ratio of 4.5:1 */
}Likely invalid or redundant comment.
|
@palisadoes It's showing me that this branch is out-of-date with the |
|
Reopening. You don't have to do this. Just update your local branch with the latest upstream. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/style/app.module.css (2)
2346-2363: Consider extracting common overlay styles.The overlay styles are duplicated between
.cardItem .iconWrapper .themeOverlayand.cardBody .iconWrapper .themeOverlay. Consider creating a shared class to follow the DRY principle.+.themeOverlay { + background: var(--bs-primary); + position: absolute; + top: 0; + right: 0; + bottom: 0; + left: 0; + opacity: 0.12; + border-radius: 50%; +} .cardItem .iconWrapper .themeOverlay { - background: var(--bs-primary); - position: absolute; - top: 0; - right: 0; - bottom: 0; - left: 0; - opacity: 0.12; - border-radius: 50%; + @extend .themeOverlay; }
2395-2398: Standardize color value format.The creator text color uses an RGB format while other colors use CSS variables. Consider using the CSS variable with a fallback for consistency.
.cardItem .creator { font-size: 1rem; - color: rgb(33, 208, 21); + color: var(--bs-success, #21d015); }
📜 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)
1624-1647: LGTM! Well-structured responsive design.
The media query implementation follows best practices with appropriate breakpoints and layout adjustments for mobile devices.
1649-1683: Ensure sufficient color contrast for accessibility.
The secondary text color uses a CSS variable without a fallback value. Consider adding a fallback and ensuring the contrast ratio meets WCAG guidelines.
|
Please make coderabbit.ai approve your PR |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/style/app.module.css (2)
1649-1683: Consolidate duplicate overlay styles.The overlay styles are duplicated between
.cardBody .iconWrapper .themeOverlayand.cardItem .iconWrapper .themeOverlay. Consider creating a shared class or CSS custom property for the overlay.+.overlayBase { + position: absolute; + top: 0; + right: 0; + bottom: 0; + left: 0; + opacity: 0.12; + border-radius: 50%; +} .cardBody .iconWrapper .themeOverlay { background: var(--bs-primary); - position: absolute; - top: 0; - right: 0; - bottom: 0; - left: 0; - opacity: 0.12; - border-radius: 50%; + composes: overlayBase; } .cardItem .iconWrapper .themeOverlay { background: var(--bs-primary); - position: absolute; - top: 0; - right: 0; - bottom: 0; - left: 0; - opacity: 0.12; - border-radius: 50%; + composes: overlayBase; }
2366-2375: Remove duplicate text truncation properties.The
.cardItem .titlehas duplicatetext-overflowandwhite-spacedeclarations..cardItem .title { font-size: 1rem; flex: 1; overflow: hidden; text-overflow: ellipsis; white-space: nowrap; - text-overflow: ellipsis; - white-space: nowrap; /* Fallback for browsers that don't support line-clamp */ margin-left: 3px; }
📜 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)
1624-1647: LGTM! Mobile-first responsive design implementation.
The media query effectively handles the card layout for mobile screens, with appropriate adjustments to spacing, positioning, and typography.
@palisadoes all |
Thanks, please address the other questions |
I have requested your review on those unresolved conversations. |
|
I don't understand. You haven't responded to my queries about the deleted sections in the dialog boxes. |
|
@palisadoes I have replied to you in those dialog boxes. I mentioned there that CodeRabbit requested the deletion of those sections because they were duplicates, in order to resolve the conversation. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/style/app.module.css (2)
2390-2410: Remove duplicate text-overflow properties.The
.cardItem .titlehas duplicatetext-overflowandwhite-spaceproperties. Also, consider using the shared text truncation styles from the@supportsblock..cardItem .title { font-size: 1rem; flex: 1; overflow: hidden; - text-overflow: ellipsis; - white-space: nowrap; - text-overflow: ellipsis; - white-space: nowrap; /* Fallback for browsers that don't support line-clamp */ + text-overflow: ellipsis; + white-space: nowrap; margin-left: 3px; }
2411-2424: Remove duplicate line-clamp properties from location styles.The
.cardItem .locationduplicates line-clamp properties that are already defined in the@supportsblock above..cardItem .location { font-size: 0.9rem; color: var(--bs-primary); overflow: hidden; - display: -webkit-box; - -webkit-line-clamp: 1; - line-clamp: 1; - -webkit-box-orient: vertical; }
📜 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 (5)
src/style/app.module.css (5)
1624-1647: LGTM! Responsive design implementation looks good.
The media query properly handles layout adjustments for smaller screens, with appropriate positioning and sizing of elements.
1649-1683: Remove duplicate overlay styles between .cardBody and .cardItem.
The .cardBody .iconWrapper .themeOverlay styles are duplicated with .cardItem .iconWrapper .themeOverlay. Consider extracting these into a shared class.
+.themeOverlay {
+ background: var(--bs-primary);
+ position: absolute;
+ top: 0;
+ right: 0;
+ bottom: 0;
+ left: 0;
+ opacity: 0.12;
+ border-radius: 50%;
+}
.cardBody .iconWrapper .themeOverlay {
- background: var(--bs-primary);
- position: absolute;
- top: 0;
- right: 0;
- bottom: 0;
- left: 0;
- opacity: 0.12;
- border-radius: 50%;
+ composes: themeOverlay;
}
.cardItem .iconWrapper .themeOverlay {
- background: var(--bs-primary);
- position: absolute;
- top: 0;
- right: 0;
- bottom: 0;
- left: 0;
- opacity: 0.12;
- border-radius: 50%;
+ composes: themeOverlay;
}2337-2344: LGTM! Card item layout implementation is solid.
The flexbox layout with proper spacing and border handling using CSS variables is well implemented.
2370-2388: LGTM! Icon wrapper implementation is clean.
The icon wrapper properly handles centering and dimensions while maintaining consistency with the design system.
2425-2454: Remove duplicate animation keyframes from .rightCard.
The zoomIn keyframes are duplicated within the .rightCard class. These animations already exist in the global scope.
.rightCard {
display: flex;
gap: 7px;
min-width: 170px;
justify-content: center;
flex-direction: column;
margin-left: 10px;
overflow-x: hidden;
width: 210px;
-
- @keyframes zoomIn {
- 0% {
- opacity: 0;
- -webkit-transform: scale(0.5);
- transform: scale(0.5);
- }
-
- 100% {
- opacity: 1;
- -webkit-transform: scale(1);
- transform: scale(1);
- }
- }
}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/style/app.module.css (3)
1624-1647: Consider using a CSS variable for the breakpoint value.The media query implementation is good, but could be more maintainable by defining the breakpoint as a CSS variable.
+:root { + --breakpoint-sm: 600px; +} -@media (max-width: 600px) { +@media (max-width: var(--breakpoint-sm)) { .cardBody { min-height: 120px; } /* ... rest of the media query ... */ }
1679-1683: Add fallback color for better browser support.The secondary text color uses a CSS variable without a fallback value.
.cardBody .textWrapper .secondaryText { font-size: 14px; display: block; - color: var(--bs-secondary); + color: var(--bs-secondary, #6c757d); }
2391-2400: Remove duplicate text-overflow properties.The text truncation properties are duplicated.
.cardItem .title { font-size: 1rem; flex: 1; overflow: hidden; text-overflow: ellipsis; white-space: nowrap; - text-overflow: ellipsis; - white-space: nowrap; /* Fallback for browsers that don't support line-clamp */ margin-left: 3px; }
📜 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)
1653-1671: Remove duplicate overlay styles.
The .cardBody .iconWrapper overlay styles are duplicated with .cardItem .iconWrapper (lines 2371-2389).
2442-2454: 🛠️ Refactor suggestion
Remove duplicate animation keyframes.
The zoomIn keyframes are duplicated. They already exist in the global scope (lines 2344-2370).
.rightCard {
/* ... existing styles ... */
- @keyframes zoomIn {
- 0% {
- opacity: 0;
- -webkit-transform: scale(0.5);
- transform: scale(0.5);
- }
-
- 100% {
- opacity: 1;
- -webkit-transform: scale(1);
- transform: scale(1);
- }
- }
}Likely invalid or redundant comment.
amaan-aly246
force-pushed
the
OrganizationDashCards_tests-migration
branch
from
8ebcb99 to
c17efbf
Compare
amaan-aly246
force-pushed
the
OrganizationDashCards_tests-migration
branch
2 times, most recently
from
72dc159 to
2a46807
Compare
|
@palisadoes Don't know what's happening , I tried to resolved this issue using Issue: |
|
Suggest me what should I do now. |
|
Please update your local branch with the latest upstream. That should fix the issue. If not, please ask the #talawa-admin channel in slack |
What kind of change does this PR introduce?
Migration of test cases written in Jest to Vitest and merged CSS files into
app.module.cssIssue Number:
Fixes #2806
Did you add tests for your changes?
Yes, additional test cases have been added for components that did not have test cases written in Jest.
Snapshots/Videos:
If relevant, did you update the documentation?
No
Does this PR introduce a breaking change?
No
Other information
Have you read the contributing guide?
Yes
Summary by CodeRabbit
New Features
Bug Fixes
Tests
CardItemLoadingandDashBoardCardLoadingcomponents to verify rendering and child elements.Chores