#3174 Action Item Pages Violate The Figma Style Guide #3372
Conversation
WalkthroughThis pull request introduces updates to theme color configuration and error handling improvements in various components. The primary color is now dynamically retrieved from CSS variables instead of being hardcoded. Additionally, a check for the presence of the root container in the DOM has been added to prevent runtime errors. Styling changes include class name updates for buttons and a shift from inline styles to CSS module-based styling for the Changes
Assessment against linked issues
Possibly related issues
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 (1)
src/style/app.module.css (1)
487-489: Remove commented code.The commented out
margin-leftshould be removed to keep the code clean..createButton { background-color: var(--grey-bg-color) !important; color: var(--black-color) !important; margin-top: 10px; - /* margin-left: 5px; */ border: 1px solid var(--brown-color); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/index.tsx(2 hunks)src/screens/OrganizationActionItems/ItemDeleteModal.tsx(1 hunks)src/screens/OrganizationActionItems/ItemViewModal.tsx(1 hunks)src/screens/OrganizationActionItems/OrganizationActionItems.tsx(1 hunks)src/style/app.module.css(6 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/screens/OrganizationActionItems/ItemViewModal.tsx
- src/screens/OrganizationActionItems/ItemDeleteModal.tsx
- src/screens/OrganizationActionItems/OrganizationActionItems.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Application
🔇 Additional comments (5)
src/index.tsx (2)
41-41: LGTM! Color update aligns with Figma style guide.The primary color change from
#31bb6bto#1778F2matches the design specifications.
142-144: LGTM! Good error handling for root container.The added check prevents potential runtime errors by validating the root container's existence before mounting the React application.
src/style/app.module.css (3)
165-165: LGTM! Good use of CSS variable for consistent shadows.The introduction of
--drop-shadowvariable centralizes shadow styling and improves maintainability.
337-341: LGTM! Clean implementation of close button styles.The close button styles are well-organized and use CSS variables for colors, maintaining consistency with the design system.
363-363: LGTM! Consistent use of drop shadow variable.The box shadow styles consistently use the new
--drop-shadowvariable across different elements (dropdown, button hover, input focus), maintaining visual consistency.Also applies to: 496-496, 519-519
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #3372 +/- ##
=====================================================
+ Coverage 7.92% 89.67% +81.74%
=====================================================
Files 312 335 +23
Lines 8135 8642 +507
Branches 1817 1922 +105
=====================================================
+ Hits 645 7750 +7105
+ Misses 7423 631 -6792
- Partials 67 261 +194
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
- When you hover over the icons in the options column, the icons change color. There should only be a shadow.
- When you click on the icons, they turn green. There should be no color change.
- At the end of the video the unchecked box has a green border. There should be none
src/screens/OrganizationActionItems/OrganizationActionItems.tsx
Outdated
Show resolved
Hide resolved
src/screens/OrganizationActionItems/OrganizationActionItems.tsx
Outdated
Show resolved
Hide resolved
src/screens/OrganizationActionItems/OrganizationActionItems.tsx
Outdated
Show resolved
Hide resolved
Fixed the icon size.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/style/app.module.css (1)
337-341: Consider using existing color variables for consistency.The
.closeButtonclass uses the raw variable--remove-button-colordirectly. For better maintainability and consistency with the codebase's color system, consider using semantic color variables..closeButton { - color: var(--remove-button-color); + color: var(--delete-button-color); border: none; - background-color: var(--white-color); + background-color: var(--bs-white); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/style/app.module.css(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Application
🔇 Additional comments (3)
src/style/app.module.css (3)
165-165: Good addition of a reusable shadow variable.The addition of
--drop-shadowas a CSS variable promotes consistency and maintainability by centralizing the shadow definition.
363-365: Good use of the new shadow variable.The
.dropdownclass now uses the centralized--drop-shadowvariable, improving consistency with other shadow effects.
518-520: Good use of the new shadow variable and transition.The
.inputField:focusstyles now use the centralized--drop-shadowvariable and include a smooth transition effect for better UX.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/style/app.module.css (2)
337-341: Consider adding hover and focus states to the.closeButtonclass.While the base styling is good, interactive states would improve user experience and accessibility.
.closeButton { color: var(--remove-button-color); border: none; background-color: var(--white-color); + transition: opacity 0.2s ease; + } + + .closeButton:hover { + opacity: 0.8; + } + + .closeButton:focus { + outline: 2px solid var(--remove-button-color); + outline-offset: 2px; }
641-643: Consider adding responsive border-radius for.dataGridContainer.The current fixed border-radius might look too large on smaller screens.
.dataGridContainer { - border-radius: 20px; + border-radius: clamp(12px, 2vw, 20px); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/index.tsx(2 hunks)src/screens/OrganizationActionItems/OrganizationActionItems.tsx(1 hunks)src/style/app.module.css(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/screens/OrganizationActionItems/OrganizationActionItems.tsx
- src/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Application
🔇 Additional comments (2)
src/style/app.module.css (2)
165-165: Good addition of the--drop-shadowCSS variable!Centralizing the shadow value as a CSS variable promotes consistency and makes it easier to maintain shadow effects across the application.
363-365: Good implementation of the new shadow variable!The consistent use of
var(--drop-shadow)across different components improves visual consistency and maintainability.Also applies to: 486-495, 518-521
Action.Items.fixes.mov |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/style/app.module.css (3)
479-484: Consider adding transition for smoother hover effect.The actionItemDeleteButton styles are good, but could benefit from a smooth transition.
.actionItemDeleteButton { background-color: var(--delete-button-bg) !important; color: var(--delete-button-color) !important; + transition: box-shadow 0.2s ease-in-out; }
645-647: Add overflow handling for dataGridContainer.The dataGridContainer should handle overflow gracefully to prevent content from breaking the layout.
.dataGridContainer { border-radius: 20px; + overflow: hidden; }
2047-2054: Enhance focus styles for better accessibility.While the hover and focus styles are good, consider adding a visible focus outline for better keyboard navigation.
.checkboxButton input:focus { outline: none; box-shadow: none; + outline: 2px solid var(--bs-primary); + outline-offset: 2px; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/style/app.module.css(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Application
🔇 Additional comments (2)
src/style/app.module.css (2)
165-165: Good addition of a reusable shadow variable!The
--drop-shadowCSS variable promotes consistency across the application and makes shadow maintenance easier.
337-341: LGTM: Clean implementation of closeButton styles.The styles follow the Figma guide with appropriate color variables and clear structure.
Action.Items.fixes.movFixed the checkbox green outline and removed the cell border |
|
Please fix the failing test and conflicting file |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/style/app.module.css (1)
97-104: 🛠️ Refactor suggestionUse semantic variable names for colors.
The new CSS variables should use semantic names that describe their function rather than their color. This helps with future dark mode support.
- --info-button-bg: var(--light-blue); - --info-button-border: var(--brown-color); - --info-button-text: var(--brown-color); + --info-button-bg: var(--primary-action-bg); + --info-button-border: var(--primary-border-color); + --info-button-text: var(--primary-text-color);
🧹 Nitpick comments (1)
src/style/app.module.css (1)
533-538: Avoid using !important declarations.The
!importantdeclarations on the action item delete button should be avoided as they make styles harder to maintain and override..actionItemDeleteButton { - background-color: var(--delete-button-bg) !important; - color: var(--delete-button-color) !important; + background-color: var(--delete-button-bg); + color: var(--delete-button-color); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/style/app.module.css(11 hunks)src/utils/StaticMockLink.spec.ts(1 hunks)
🔇 Additional comments (3)
src/utils/StaticMockLink.spec.ts (1)
717-717: Verify the timing change rationale.The minimum expected delay time has been reduced from 50ms to 40ms. Please confirm if this change is due to:
- Addressing flaky tests that were failing intermittently with the 50ms threshold
- Matching changes in the actual implementation's timing behavior
✅ Verification successful
Confirmed: Test reliability improvement
The change from 50ms to 40ms threshold makes the test more reliable while still validating the 50ms delay configuration. This provides a 10ms buffer to prevent flaky test failures due to timing precision issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for delay-related configurations in the codebase echo "Searching for delay configurations..." rg -A 5 "delay.*=.*[45]0" --type tsLength of output: 122
Script:
#!/bin/bash # Get the test context and related delay configurations echo "Searching for the test context..." rg -B 10 -A 2 "expect\(elapsed\)\.toBeGreaterThanOrEqual" src/utils/StaticMockLink.spec.ts echo -e "\nSearching for delay-related configurations in StaticMockLink files..." rg "delay|timeout|wait" src/utils/StaticMockLink* echo -e "\nSearching for timing constants..." rg "const.*=.*\d+.*ms" src/utils/StaticMockLink*Length of output: 1494
src/style/app.module.css (2)
574-578: LGTM! Input field focus styles follow best practices.The input field focus styles properly handle accessibility by providing clear visual feedback.
2093-2104: LGTM! Checkbox styles follow design guidelines.The checkbox styles align with the Figma design guide, providing consistent visual feedback for hover and focus states.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/style/app.module.css (4)
416-418: Avoid using !important flags in CSS.The !important flags make styles harder to maintain and override. Consider using more specific selectors instead.
- box-shadow: var(--drop-shadow); - border: 1px solid var(--brown-color) !important; - color: var(--dropdown-color) !important; + .dropdown:is(:hover, :focus, :active, :focus-visible, .show, :disabled, :checked) { + box-shadow: var(--drop-shadow); + border: 1px solid var(--brown-color); + color: var(--dropdown-color); + }
533-538: Remove !important flags from action item delete button.The !important flags should be avoided. Use more specific selectors to achieve the same effect.
-.actionItemDeleteButton { - background-color: var(--delete-button-bg) !important; - color: var(--delete-button-color) !important; -} +button.actionItemDeleteButton { + background-color: var(--delete-button-bg); + color: var(--delete-button-color); +}
671-673: Remove !important flags from info button.The !important flags should be avoided. Use more specific selectors to achieve the same effect.
- background-color: var(--info-button-bg) !important; - border-color: var(--info-button-border) !important; +button.infoButton { + background-color: var(--info-button-bg); + border-color: var(--info-button-border); +}Also applies to: 679-680
700-703: Remove !important flag from row background.The !important flag should be avoided. Use more specific selectors to achieve the same effect.
- background-color: var(--row-background) !important; +tr.rowBackground { + background-color: var(--row-background); +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/style/app.module.css(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Application
🔇 Additional comments (6)
src/style/app.module.css (6)
95-102: Improve semantic naming of CSS variables.Some variables reference color names (e.g.,
--brown-color) instead of their functional purpose. This makes it harder to maintain a consistent theme and implement features like dark mode.Consider renaming variables to describe their function rather than their color:
- --info-button-bg: var(--light-blue); - --info-button-border: var(--brown-color); - --info-button-text: var(--brown-color); + --info-button-bg: var(--secondary-action-bg); + --info-button-border: var(--border-default); + --info-button-text: var(--text-default);
Line range hint
164-215: LGTM! Well-structured CSS variables.The variables use semantic naming and consistent units, making them maintainable and reusable across the application.
387-391: LGTM! Clean button implementation.The close button styles are well-structured, using CSS variables and following consistent naming conventions.
544-544: LGTM! Consistent button styling.The create button styles use appropriate spacing and consistent shadow effects.
Also applies to: 552-553
574-578: LGTM! Well-structured input field styles.The input field styles use appropriate transitions and consistent CSS variables.
2093-2104: LGTM! Accessible checkbox button styles.The checkbox button styles follow accessibility best practices with appropriate focus states and use consistent CSS variables.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/index.tsx(2 hunks)src/screens/OrganizationActionItems/ItemDeleteModal.tsx(1 hunks)src/screens/OrganizationActionItems/ItemViewModal.tsx(1 hunks)src/screens/OrganizationActionItems/OrganizationActionItems.tsx(1 hunks)src/style/app.module.css(10 hunks)src/utils/StaticMockLink.spec.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/index.tsx
[warning] 145-145: src/index.tsx#L145
Added line #L145 was not covered by tests
[warning] 147-147: src/index.tsx#L147
Added line #L147 was not covered by tests
🔇 Additional comments (10)
src/screens/OrganizationActionItems/ItemDeleteModal.tsx (1)
65-65: LGTM! Style update aligns with Figma guide.The class name change from
modalCloseBtntocloseButtonmaintains functionality while improving style consistency.src/index.tsx (2)
41-43: LGTM! Using CSS variables improves theme maintainability.The change from hardcoded color to CSS variable aligns with the style guide and supports future dark mode implementation.
144-147: Add test coverage for root container error handling.The new error handling for missing root container improves robustness but lacks test coverage.
Would you like me to help generate test cases for this error handling scenario?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 145-145: src/index.tsx#L145
Added line #L145 was not covered by tests
[warning] 147-147: src/index.tsx#L147
Added line #L147 was not covered by testssrc/screens/OrganizationActionItems/ItemViewModal.tsx (1)
60-60: LGTM! Consistent style update across modals.The class name change maintains styling consistency with ItemDeleteModal component.
src/style/app.module.css (6)
95-102: LGTM! Good use of semantic color variables.The new CSS variables follow best practices by using semantic names that describe their function rather than their color value, which will help with future theming support.
533-538: LGTM! Proper use of !important for delete button styles.The use of
!importantis justified here to ensure consistent styling for critical delete actions.
544-544: LGTM! Consistent hover effects added.Good addition of drop shadow on hover for better visual feedback.
Also applies to: 552-553
574-578: LGTM! Improved input field focus styles.The focus styles provide clear visual feedback with a consistent shadow effect and smooth transition.
671-673: LGTM! Consistent info button styling.The info button styles properly use CSS variables and maintain consistency with other buttons.
Also applies to: 679-680
2093-2104: LGTM! Enhanced checkbox styling with proper states.Good implementation of hover and focus states for checkboxes using CSS variables.
Resolved the failing test and fixed the conflicting file. |
Apologies for overwriting the changes. I appreciate everyone's efforts and will fix this ASAP. Thank you for pointing it out! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/style/app.module.css (5)
532-537: Consider alternatives to!important.While the styles work, using
!importantis generally discouraged as it makes styles harder to maintain and override. Consider increasing specificity through class names or restructuring the CSS cascade.-.actionItemDeleteButton { - background-color: var(--delete-button-bg) !important; - color: var(--delete-button-color) !important; -} +/* Increase specificity instead of using !important */ +.actionItems .actionItemDeleteButton, +[data-theme="default"] .actionItemDeleteButton { + background-color: var(--delete-button-bg); + color: var(--delete-button-color); +}
Line range hint
543-552: Remove commented out code.Remove the commented out
margin-leftproperty as it adds noise to the codebase. If the property is needed in the future, it can be retrieved from version control..createButton { background-color: var(--grey-bg-color) !important; color: var(--black-color) !important; margin-top: 10px; - /* margin-left: 5px; */ border: 1px solid var(--davys-grey); }
573-577: Enhance input field focus styles.The focus styles provide good user feedback with transitions, but consider:
- Avoiding
!importantby increasing selector specificity- Adding
outline-offsetfor better visibility- Using
outline-colorinstead ofbox-shadowfor better accessibility.inputField:focus { - border: 0.1px solid var(--input-field-border) !important; - background-color: var(--input-field-bg) !important; - box-shadow: var(--drop-shadow); + border: 0.1px solid var(--input-field-border); + background-color: var(--input-field-bg); + outline: 2px solid var(--input-field-border); + outline-offset: 2px; outline: none; transition: box-shadow 0.2s ease; }
670-672: Improve info button styles.The hover effects provide good user feedback, but consider:
- Avoiding
!importantby increasing selector specificity- Adding focus styles for keyboard navigation
.infoButton { - background-color: var(--search-button-bg) !important; + background-color: var(--search-button-bg); border-color: var(--search-button-border); - color: var(--davys-grey) !important; + color: var(--davys-grey); } .infoButton:hover { background-color: var(--light-blue-color); border-color: var(--search-button-border); - color: var(--info-button-text) !important; + color: var(--info-button-text); box-shadow: var(--drop-shadow); } +.infoButton:focus-visible { + outline: 2px solid var(--info-button-text); + outline-offset: 2px; +}Also applies to: 680-681
701-701: Simplify row background style.Consider removing the
!importantflag by increasing selector specificity or restructuring the CSS cascade.-.rowBackground { - background-color: var(--row-background) !important; - max-height: 120px; - overflow-y: auto; -} +/* Increase specificity to avoid !important */ +.table .rowBackground, +[data-theme="default"] .rowBackground { + background-color: var(--row-background); + max-height: 120px; + overflow-y: auto; +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/style/app.module.css(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Deployment to https://docs-admin.talawa.io
🔇 Additional comments (2)
src/style/app.module.css (2)
95-100: Well-structured semantic CSS variables!The new CSS variables follow good naming conventions by describing their function rather than their color value. This makes the code more maintainable and supports future theming changes.
2094-2105: Well-structured checkbox styles!The checkbox styles are well-organized with:
- Clear state management (checked, hover, focus)
- Semantic CSS variables for colors
- Good user feedback through visual changes
- Proper focus handling for accessibility
palisadoes
merged commit 07740e5
into
PalisadoesFoundation:develop-postgres
What kind of change does this PR introduce?
This PR updates the styling of the Action Items page to align with the Figma design specifications.
Issue Number:
Fixes #3174
Snapshots/Videos:
Action.Items.mp4
If relevant, did you update the documentation?
Summary
This PR resolves issues related to the design mismatch and ensures the page adheres to the design guidelines.
Does this PR introduce a breaking change?
No
Checklist
CodeRabbit AI Review
Test Coverage
Other information
Have you read the contributing guide?
Yes
Summary by CodeRabbit
Release Notes
Style Updates
.closeButtonclass for consistent modal button styling.Bug Fixes
Chores