Figma: People Page Violates The Figma Style Guide #3408
Conversation
WalkthroughThis pull request focuses on updating the styling for buttons and CSS variables across multiple files. The changes primarily involve modifying color schemes in CSS files, specifically targeting the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (5)
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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #3408 +/- ##
===================================================
- Coverage 6.31% 6.30% -0.01%
===================================================
Files 315 315
Lines 8270 8274 +4
Branches 1859 1856 -3
===================================================
Hits 522 522
- Misses 7688 7692 +4
Partials 60 60
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![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
🧹 Nitpick comments (3)
src/screens/OrganizationPeople/OrganizationPeople.tsx (1)
452-453: Cell borders removed.
Eliminating cell borders can enhance the modern feel, but watch out for large data sets where visuals may blur row boundaries.src/screens/OrganizationPeople/AddMember.tsx (2)
451-451: Modal header style.
Consider verifying that the new class name.createUserModalHeaderis well-documented or tested, if relevant.
554-561: Button styling for user creation.
The inline style overrides are permissible but might be better consolidated into the global CSS to keep styling consistent.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/assets/css/app.css(1 hunks)src/screens/OrganizationPeople/AddMember.tsx(13 hunks)src/screens/OrganizationPeople/OrganizationPeople.tsx(7 hunks)src/style/app.module.css(5 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/screens/OrganizationPeople/AddMember.tsx
[warning] 113-116: src/screens/OrganizationPeople/AddMember.tsx#L113-L116
Added lines #L113 - L116 were not covered by tests
[warning] 118-118: src/screens/OrganizationPeople/AddMember.tsx#L118
Added line #L118 was not covered by tests
[warning] 124-124: src/screens/OrganizationPeople/AddMember.tsx#L124
Added line #L124 was not covered by tests
[warning] 142-142: src/screens/OrganizationPeople/AddMember.tsx#L142
Added line #L142 was not covered by tests
src/screens/OrganizationPeople/OrganizationPeople.tsx
[warning] 42-42: src/screens/OrganizationPeople/OrganizationPeople.tsx#L42
Added line #L42 was not covered by tests
[warning] 188-190: src/screens/OrganizationPeople/OrganizationPeople.tsx#L188-L190
Added lines #L188 - L190 were not covered by tests
[warning] 375-375: src/screens/OrganizationPeople/OrganizationPeople.tsx#L375
Added line #L375 was not covered by tests
[warning] 389-389: src/screens/OrganizationPeople/OrganizationPeople.tsx#L389
Added line #L389 was not covered by tests
[warning] 403-403: src/screens/OrganizationPeople/OrganizationPeople.tsx#L403
Added line #L403 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Application
🔇 Additional comments (22)
src/screens/OrganizationPeople/OrganizationPeople.tsx (9)
22-23: Imports look clean and purposeful.
No concerns here; these imports replace or supplement prior solutions neatly and make the code more self-contained.
178-192: Validate row numbering for paginated data.
This new column logic seems correct for local sorting, but be mindful if you move to server-side pagination, as row indices can become ambiguous. Also note lines 188-190 aren't covered by tests.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 188-190: src/screens/OrganizationPeople/OrganizationPeople.tsx#L188-L190
Added lines #L188 - L190 were not covered by tests
201-201: Profile image cell uses correct class name.
Looks consistent with the rest of the styling approach.
349-349: Confirm the test selector.
Thedata-testid="searchbtn"attribute is clear. Ensure you have a matching test to validate search button behavior.
356-410: Dropdown-based sorting approach is workable; consider removingdefaultChecked.
UsingdefaultCheckedon<Dropdown.Item>is unconventional. Rely on thestateto indicate active selection or integrate a highlighted class instead. Also, lines 375, 389, and 403 are flagged as uncovered by tests.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 375-375: src/screens/OrganizationPeople/OrganizationPeople.tsx#L375
Added line #L375 was not covered by tests
[warning] 389-389: src/screens/OrganizationPeople/OrganizationPeople.tsx#L389
Added line #L389 was not covered by tests
[warning] 403-403: src/screens/OrganizationPeople/OrganizationPeople.tsx#L403
Added line #L403 was not covered by tests
418-420: Ensure data presence checks.
When switching betweenmemberData,adminFilteredData, andusersData, guard against potential undefined objects. Otherwise, the condition is sound for controlling the DataGrid display.
439-459: DataGrid styling is consistent with Figma guidelines.
This lines up well with the Figma style guide, especially removing row borders and applying a custom hover color.
455-459: New hover color from CSS variable.
Applyingvar(--org-people-hover-blue)keeps the table theming consistent.
42-42: Add test coverage for extracting orgId from URL.
Line 42 is not covered by tests per static analysis. Consider adding a test that ensuresorgIdis successfully retrieved and used.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 42-42: src/screens/OrganizationPeople/OrganizationPeople.tsx#L42
Added line #L42 was not covered by testssrc/screens/OrganizationPeople/AddMember.tsx (8)
2-2: Added new icon import for searching.
Please confirm that the places using<Search />are tested so coverage for line #2 isn't flagged.
107-119: Custom hookuseModalis a clean abstraction.
This is a strong approach for reusability. Add test coverage around open/close transitions and initial state handling.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 113-116: src/screens/OrganizationPeople/AddMember.tsx#L113-L116
Added lines #L113 - L116 were not covered by tests
[warning] 118-118: src/screens/OrganizationPeople/AddMember.tsx#L118
Added line #L118 was not covered by tests
120-124: Destructured modal logic looks good.
No immediate issues, but be cautious about passing these modal handlers around. Consider verifying if any child components require stronger type definitions.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 124-124: src/screens/OrganizationPeople/AddMember.tsx#L124
Added line #L124 was not covered by tests
142-142: ConfirmorgIdusage is covered by tests.
Line 142 is flagged as uncovered code. Add a simple test verifying thatAddMemberproperly reads the URL param.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 142-142: src/screens/OrganizationPeople/AddMember.tsx#L142
Added line #L142 was not covered by tests
365-368: Translation for column headers is consistent.
UsingtranslateOrgPeoplealigns with the rest of the file. No concerns here.
410-410: Clear, descriptive link class name.
.membernameplus.subtleBlueGreyfosters a uniform style. Looks good.
449-449: Closing the modal withcloseCreateNewUserModal.
This direct approach is more explicit than toggling. Nice improvement for clarity.
Line range hint
643-643: Sync border color with new delete theme variable.
Replacing any hardcoded color withvar(--delete-theme-color)keeps the color scheme unified. Good move.src/style/app.module.css (5)
86-86: New variable--org-people-hover-blue.
A consistent naming scheme with your existing custom properties. Nicely done.
96-96: Introduced--delete-theme-color.
Centralizing the “danger” or “delete” color is excellent for brand consistency and maintainability.
99-99:--close-button-bgupdated to a direct color.
Confirm that removing the reference tovar(--white-color)doesn’t break existing usage.
643-643: Applyingvar(--delete-theme-color)in.removeButton.
Harmonizes with the new variable. Good consistency.
1472-1472:.eventsAttended, .membername { color: var(--black-color); }
Aligns the text color with the updated design. Just ensure this doesn’t clash with any theme-related text color.
|
|
Please fix the conflicting file |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/style/app.module.css (1)
98-100: Consider using CSS variable references for better maintainability.Instead of using direct color values, consider referencing existing CSS variables:
- --close-button-bg: #f8d6dc; + --close-button-bg: var(--delete-button-bg);The new
--input-text-colorvariable is well-placed and follows naming conventions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/style/app.module.css(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Application
🔇 Additional comments (4)
src/style/app.module.css (4)
85-85: LGTM! New CSS variable follows naming conventions.The new CSS variable
--org-people-hover-blueis well-named and appropriately placed within the root variables section.
95-95: LGTM! New CSS variable consolidates the danger color.The new CSS variable
--delete-theme-colorappropriately consolidates the Bootstrap danger color (#dc3545) for consistent usage across the application.
899-899: LGTM! Improved color consistency.The change to use
--delete-theme-colorfor the button border color improves consistency in the color system.
1809-1809: LGTM! Improved color consistency using theme variables.The change to use
--black-colorfor both.eventsAttendedand.membernameclasses improves theme consistency.
|
can you guild me how to fix test application |
|
|
i merged with upstream |
|
what should i do now |
|
Please ask the #talawa-projects slack channel. We recently did a major update that could be affecting this. |
|
Please fix the conflicting file |
|
|
Alright, I will review these changes and work on improving them. Could you please let me know which API branch is suitable for develop-postgres admin branch |
|
develop-postgres until further notice |
|
Please note my previous review above |
|
ok i keep in mind |
What kind of change does this PR introduce?
bugfix, feature, refactoring
Issue Number:
Fixes #3171
Snapshots/Videos:
If relevant, did you update the documentation?
no
Summary
it refactor mainly OraganisationPeople.tsx
Does this PR introduce a breaking change?
no
Checklist
CodeRabbit AI Review
Test Coverage
Have you read the contributing guide?
Yes
Summary by CodeRabbit
Release Notes
New Features
UI/UX Improvements
Style Changes
Performance