Refactor styles: move SubTags.module.css styles to app.module.css #2538
Conversation
WalkthroughThe changes involve the deletion of the Changes
Assessment against linked issues
Possibly related issues
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
🧹 Outside diff range and nitpick comments (4)
src/style/app.module.css (4)
Line range hint
75-89: Consolidate duplicate .btnsContainer stylesThere are duplicate style definitions for
.btnsContainerand its child elements. This can lead to maintenance issues and potential style conflicts.Consolidate the styles into a single definition:
.btnsContainer { display: flex; margin: 2.5rem 0; align-items: center; gap: 10px; } .btnsContainer .btnsBlock { display: flex; width: max-content; } .btnsContainer .btnsBlock button { margin-left: 1rem; display: flex; justify-content: center; align-items: center; } .btnsContainer .input { flex: 1; position: relative; max-width: 60%; justify-content: space-between; } .btnsContainer input { outline: 1px solid var(--bs-gray-400); } .btnsContainer .input button { width: 52px; } - /* Remove duplicate definitions at lines 616-641 */Also applies to: 616-641
691-707: Enhance error message accessibilityWhile the error styles are well-structured, they could benefit from improved accessibility for screen readers.
Add ARIA attributes and role to improve accessibility:
.errorContainer { min-height: 100vh; + role: alert; + aria-live: polite; } .errorMessage { margin-top: 25%; display: flex; justify-content: center; align-items: center; flex-direction: column; + aria-atomic: true; } .errorIcon { transform: scale(1.5); color: var(--bs-danger); margin-bottom: 1rem; + aria-hidden: true; }
709-717: Approve color accessibility improvementsThe use of Bootstrap color variables improves accessibility for color-blind users. The contrast between the white text and primary background color meets WCAG guidelines.
Consider adding a hover state for table rows to improve interactivity:
.rowBackground { background-color: var(--bs-white); max-height: 120px; + transition: background-color 0.2s ease; } + .rowBackground:hover { + background-color: var(--bs-gray-100); + }
767-773: Enhance scrolling behaviorThe scrollbar styling is good, but the scrolling experience could be improved.
Add smooth scrolling behavior:
.subTagsScrollableDiv { scrollbar-width: auto; scrollbar-color: var(--bs-gray-400) var(--bs-white); max-height: calc(100vh - 18rem); overflow: auto; + scroll-behavior: smooth; + -webkit-overflow-scrolling: touch; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/screens/SubTags/SubTags.module.css(0 hunks)src/style/app.module.css(2 hunks)
💤 Files with no reviewable changes (1)
- src/screens/SubTags/SubTags.module.css
🔇 Additional comments (1)
src/style/app.module.css (1)
643-689: 🛠️ Refactor suggestion
Consolidate duplicate media queries
Similar to the button container styles, there are duplicate media query definitions that should be consolidated.
Consolidate the media queries into a single location in the file:
@media (max-width: 1020px) {
.btnsContainer {
flex-direction: column;
margin: 1.5rem 0;
}
.btnsContainer .btnsBlock {
margin: 1.5rem 0 0 0;
justify-content: space-between;
}
.btnsContainer .btnsBlock button {
margin: 0;
}
.btnsContainer .btnsBlock div button {
margin-right: 1.5rem;
}
}
@media (max-width: 520px) {
.btnsContainer {
margin-bottom: 0;
}
.btnsContainer .btnsBlock {
display: block;
margin-top: 1rem;
margin-right: 0;
}
.btnsContainer .btnsBlock div {
flex: 1;
}
.btnsContainer .btnsBlock div[title='Sort organizations'] {
margin-right: 0.5rem;
}
.btnsContainer .btnsBlock button {
margin-bottom: 1rem;
margin-right: 0;
width: 100%;
}
}
- /* Remove duplicate media queries from other locations */Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
src/style/app.module.css (3)
75-75: Consider accessibility and responsive design improvements.While the layout structure is improved, there are a few concerns:
- Setting outline directly might affect keyboard navigation accessibility.
- The fixed max-width of 60% on input might cause issues on certain viewports.
Consider these improvements:
.btnsContainer .input { flex: 1; position: relative; - max-width: 60%; + max-width: min(60%, 600px); justify-content: space-between; } .btnsContainer input { - outline: 1px solid var(--bs-gray-400); + border: 1px solid var(--bs-gray-400); + &:focus-visible { + outline: 2px solid var(--bs-primary); + outline-offset: 2px; + } }Also applies to: 618-643
645-691: Improve maintainability of responsive styles.The media queries handle responsive behavior well, but there are opportunities for improvement:
- The breakpoint values should be defined as CSS custom properties for better maintainability.
- There's a duplicate media query for max-width: 520px (also appears earlier in the file).
Consider these improvements:
+:root { + --breakpoint-tablet: 1020px; + --breakpoint-mobile: 520px; +} -@media (max-width: 1020px) { +@media (max-width: var(--breakpoint-tablet)) { /* ... */ } -@media (max-width: 520px) { +@media (max-width: var(--breakpoint-mobile)) { /* ... */ }Also, consider consolidating the duplicate media queries to avoid maintenance issues.
761-767: Add cross-browser compatibility for scrollbar styles.The scrollbar styles need webkit prefixes for better browser support.
Add webkit prefixes:
.subTagsScrollableDiv { scrollbar-width: auto; scrollbar-color: var(--bs-gray-400) var(--bs-white); + &::-webkit-scrollbar { + width: auto; + } + &::-webkit-scrollbar-track { + background: var(--bs-white); + } + &::-webkit-scrollbar-thumb { + background-color: var(--bs-gray-400); + } max-height: calc(100vh - 18rem); overflow: auto; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/style/app.module.css(3 hunks)
🔇 Additional comments (1)
src/style/app.module.css (1)
721-759: LGTM! Excellent implementation of layout shift prevention.
The navigation styles demonstrate best practices:
- Layout shift prevention using pseudo-elements.
- Proper focus states for keyboard navigation.
- Consistent hover and focus styles.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/style/app.module.css (1)
Line range hint
75-648: Remove duplicate btnsContainer stylesThe styles for
.btnsContainerand its child elements are duplicated in the file. This can lead to maintenance issues and inconsistencies.Consolidate the styles into a single definition:
- /* First definition at line 75 */ - .btnsContainer { - display: flex; - margin: 2.5rem 0; - align-items: center; - gap: 10px; - } - /* ... other duplicate styles ... */ /* Keep the complete definition at line 622 */ .btnsContainer { display: flex; margin: 2.5rem 0; align-items: center; gap: 10px; /* ... rest of the styles ... */ }Also applies to: 622-696
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/style/app.module.css(4 hunks)
🔇 Additional comments (3)
src/style/app.module.css (3)
344-348: Add ARIA attributes for error messages
While adding a non-color indication for errors is good, the error messages should be properly announced to screen readers.
As mentioned in a previous review, add ARIA attributes to improve accessibility:
.errorContainer {
min-height: 100vh;
+ role: "alert";
+ aria-live: "polite";
}
.errorIcon {
transform: scale(1.5);
color: var(--bs-danger);
margin-bottom: 1rem;
&::before {
content: '⚠️';
margin-right: 0.5rem;
}
}706-715: LGTM: Table styles improvements
The changes address previous review comments:
- Table header now uses
--bs-primary-text-emphasisfor better contrast - Added overflow handling for row content
717-755: LGTM: Navigation accessibility improvements
Excellent improvements for accessibility and user experience:
- Focus states match hover states for keyboard navigation
- Layout shift prevention using pseudo-elements
It was already there in src/screens/SubTags/SubTags.tsx
|
|
Thanks. Please fix the failing tests |
the failing tests are due to some modules missing, which is out of scope of this PR. Do you want me to include it? There are some issues like: "Cannot find module '@testing-library/dom' from 'node_modules/@testing-library/react/dist/pure.js'" so to fix failiing tests i have to install this. |
Yes. @AVtheking will provide further guidance |
|
any updates? @AVtheking @palisadoes |
|
Please fix the tests that need to be updated because of your changes |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/style/app.module.css (2)
679-696: Consider using a more flexible width constraintThe fixed max-width of 60% might be too restrictive on certain viewport sizes. Consider using
clamp()ormin()for better flexibility..btnsContainer .input { flex: 1; position: relative; - max-width: 60%; + max-width: clamp(300px, 60%, 800px); justify-content: space-between; }
813-819: Consider adding smooth scrollingFor better user experience, consider adding smooth scrolling behavior.
.subTagsScrollableDiv { scrollbar-width: auto; scrollbar-color: var(--bs-gray-400) var(--bs-white); max-height: calc(100vh - 18rem); overflow: auto; + scroll-behavior: smooth; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/style/app.module.css(4 hunks)
🔇 Additional comments (4)
src/style/app.module.css (4)
706-752: LGTM! Well-structured responsive design
The media queries effectively handle different viewport sizes with appropriate breakpoints and layout adjustments.
762-771: LGTM! Improved table accessibility and usability
The changes address both color contrast and content overflow issues effectively.
773-811: LGTM! Excellent layout shift prevention
The pseudo-element technique effectively prevents layout shifts during hover/focus states while maintaining accessibility.
349-353:
Complete the accessibility improvements
While the non-color indication is good, the error message still needs ARIA attributes for better screen reader support.
.errorContainer {
min-height: 100vh;
+ role: "alert";
+ aria-live: "polite";
}
.errorMessage {
margin-top: 25%;
display: flex;
justify-content: center;
align-items: center;
flex-direction: column;
+ aria-atomic: "true";
}Also applies to: 754-760
which failing tests? |
|
@amitb0ra you must fix this failing test. https://github.com/PalisadoesFoundation/talawa-admin/actions/runs/12215418229/job/34077354372?pr=2538 |
amitb0ra
force-pushed
the
refactor/css-global-subtags-2526
branch
from
1fa30fe to
f674e32
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
src/style/app.module.css (1)
767-771: Consider dynamic max-height for table rows.The fixed max-height of 120px might truncate content unexpectedly. Consider using a relative unit or min/max constraints.
Apply this diff to improve the row height handling:
.rowBackground { background-color: var(--bs-white); - max-height: 120px; + max-height: min(120px, 8rem); overflow-y: auto; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/screens/SubTags/SubTags.module.css(0 hunks)src/style/app.module.css(4 hunks)
💤 Files with no reviewable changes (1)
- src/screens/SubTags/SubTags.module.css
🔇 Additional comments (2)
src/style/app.module.css (2)
773-811: Great implementation of layout shift prevention and accessibility!
The use of pseudo-elements to prevent layout shifts during hover/focus states is excellent. The focus states matching hover states ensures good keyboard navigation accessibility.
813-819: LGTM! Well-implemented scrollable container.
The scrollbar customization and use of CSS custom properties is clean and maintainable.
…/amitb0ra/talawa-admin into refactor/css-global-subtags-2526
|
@amitb0ra Please fix the failed tests. |
What kind of change does this PR introduce?
refactoring
Issue Number:
Fixes #2526
Did you add tests for your changes?
No
Snapshots/Videos:
There are some issues like: "Cannot find module '@testing-library/dom' from 'node_modules/@testing-library/react/dist/pure.js'" but that is not relevant here.
If relevant, did you update the documentation?
No
Summary
The goal is to convert the CSS file in this subdirectory and all the components related to this screen to use this new design pattern.
Does this PR introduce a breaking change?
No
Other information
No
Have you read the contributing guide?
Yes
Summary by CodeRabbit
Bug Fixes
New Features
Style