Figma: People Page Violates The Figma Style Guide #3367
Conversation
…ation/talawa-admin into develop-postgres
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (3)
src/components/CheckIn/TableRow.tsx (1)
Line range hint
76-82: Consider error handling improvements for PDF generation.The PDF generation logic could be improved in several ways:
- Add type checking for the PDF blob
- Handle potential URL.createObjectURL failures
- Clean up the created URL object to prevent memory leaks
Here's the suggested implementation:
- const blob = new Blob([pdf], { type: 'application/pdf' }); - // istanbul ignore next - const url = URL.createObjectURL(blob); - // istanbul ignore next - window.open(url); - // istanbul ignore next - toast.success('PDF generated successfully!'); + const blob = new Blob([pdf], { type: 'application/pdf' }); + const url = URL.createObjectURL(blob); + try { + const newWindow = window.open(url); + if (!newWindow) { + throw new Error('Failed to open PDF in new window'); + } + toast.success('PDF generated successfully!'); + } finally { + // Clean up the URL object to prevent memory leaks + URL.revokeObjectURL(url); + }🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] Contains code coverage disable statement. Remove the disable statement and add appropriate tests.
src/style/app.module.css (2)
Line range hint
97-138: Align color system with Figma style guide.The color variables are inconsistently defined, mixing direct color values with variable references. This violates the style guide's color system.
- --tablerow-bg-color: var(--dropdown-hover-color); + --tablerow-bg-color: var(--grey-bg-color); - --table-head-bg: var(--blue-subtle, var(--blue-color)); + --table-head-bg: var(--blue-subtle); - --table-header-color: var(--bs-white, var(--bs-gray-300)); + --table-header-color: var(--bs-white);
Line range hint
5275-5584: Fix calendar styles and unknown CSS properties.
- Calendar styles use hardcoded colors instead of design system variables
- Static analysis found unknown properties
justifycontentandalignitems.calendar { - background: var(--grey-bg-color); + background: var(--subtle-blue-grey); } .profileImage { display: flex; - justifycontent: center; - alignitems: center; + justify-content: center; + align-items: center; height: 100%; width: 100%; }
🧹 Nitpick comments (6)
src/components/LeftDrawerOrg/LeftDrawerOrg.tsx (1)
124-129: Improve accessibility of loading state button.The loading state button lacks proper accessibility attributes. Add appropriate ARIA attributes to improve screen reader support.
Apply these improvements:
<button className={`${styles.profileContainer} shimmer`} data-testid="orgBtn" + aria-label="Loading organization information" + aria-busy="true" + disabled />src/components/LeftDrawerOrg/LeftDrawerOrg.module.css (4)
1-40: Consider standardizing animation durations and improving responsiveness.The drawer animations use inconsistent durations (0.5s for transition vs 0.4s for animation). Additionally, the fixed width calculation
calc(300px + 2rem)may cause overflow on smaller screens..leftDrawer { - width: calc(300px + 2rem); + width: min(calc(300px + 2rem), 90vw); transition: 0.4s; /* Standardize to 0.4s */ } .activeDrawer { - width: calc(300px + 2rem); + width: min(calc(300px + 2rem), 90vw); animation: comeToRightBigScreen 0.4s ease-in-out; }
128-182: Improve cross-browser text overflow handling.The text overflow properties only use -webkit prefixes. Add standard properties for better cross-browser support.
.leftDrawer .profileContainer .profileText .primaryText { overflow: hidden; display: -webkit-box; -webkit-line-clamp: 2; -webkit-box-orient: vertical; + display: box; + line-clamp: 2; + box-orient: vertical; word-wrap: break-word; white-space: normal; }
184-281: Standardize responsive breakpoints.The media queries use inconsistent breakpoints (1120px, 900px, 650px) which may cause unexpected behavior at certain screen sizes. Consider using standard breakpoints.
- @media (max-width: 1120px) + @media (max-width: 1200px) /* xl */ - @media (max-height: 900px) + @media (max-height: 992px) /* lg */ - @media (max-height: 650px) + @media (max-height: 768px) /* md */
287-383: Optimize animation performance.The animations could benefit from performance optimizations using transform instead of left property, and adding will-change hint.
@keyframes goToLeftBigScreen { from { - left: 0; + transform: translateX(0); } to { opacity: 0.1; - left: calc(-300px - 2rem); + transform: translateX(calc(-300px - 2rem)); } } .leftDrawer { + will-change: transform; /* existing properties */ }src/assets/css/app.css (1)
3536-3549: Consider differentiating hover and active states.While the success button styles match the Figma guide, using the same background color (#eaebef) for both hover and active states may not provide enough visual feedback to users.
Consider using slightly different shades for hover and active states to improve user experience:
--bs-btn-bg: #eaebef; --bs-btn-hover-bg: #eaebef; - --bs-btn-active-bg: #eaebef; + --bs-btn-active-bg: #dcdee3;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
.husky/pre-commit(1 hunks)package.json(1 hunks)public/locales/en/translation.json(1 hunks)src/assets/css/app.css(7 hunks)src/components/CheckIn/TableRow.tsx(1 hunks)src/components/CollapsibleDropdown/CollapsibleDropdown.tsx(3 hunks)src/components/LeftDrawer/LeftDrawer.tsx(2 hunks)src/components/LeftDrawerOrg/LeftDrawerOrg.module.css(1 hunks)src/components/LeftDrawerOrg/LeftDrawerOrg.tsx(4 hunks)src/screens/EventManagement/EventManagement.tsx(1 hunks)src/screens/OrgList/OrgList.tsx(3 hunks)src/screens/OrganizationPeople/AddMember.tsx(13 hunks)src/screens/OrganizationPeople/OrganizationPeople.tsx(7 hunks)src/style/app.module.css(42 hunks)
✅ Files skipped from review due to trivial changes (3)
- .husky/pre-commit
- src/components/CollapsibleDropdown/CollapsibleDropdown.tsx
- public/locales/en/translation.json
🧰 Additional context used
🪛 GitHub Actions: PR Workflow
src/components/CheckIn/TableRow.tsx
[error] Contains code coverage disable statement. Remove the disable statement and add appropriate tests.
package.json
[error] Unauthorized modification or deletion of sensitive file
src/screens/OrganizationPeople/AddMember.tsx
[error] 2-2: 'Check' is defined but never used
src/components/LeftDrawerOrg/LeftDrawerOrg.module.css
[warning] File ignored because of a matching ignore pattern. Use "--no-ignore" to override
src/assets/css/app.css
[warning] File ignored because of a matching ignore pattern. Use "--no-ignore" to override
🪛 eslint
src/screens/OrganizationPeople/AddMember.tsx
[error] 2-2: 'Check' is defined but never used.
(@typescript-eslint/no-unused-vars)
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
src/screens/OrganizationPeople/AddMember.tsx
[failure] 2-2:
'Check' is defined but never used
🪛 Biome (1.9.4)
src/style/app.module.css
[error] 599-599: Unknown property is not allowed.
See CSS Specifications and browser specific properties for more details.
To resolve this issue, replace the unknown property with a valid CSS property.
(lint/correctness/noUnknownProperty)
[error] 600-600: Unknown property is not allowed.
See CSS Specifications and browser specific properties for more details.
To resolve this issue, replace the unknown property with a valid CSS property.
(lint/correctness/noUnknownProperty)
🔇 Additional comments (13)
src/components/CheckIn/TableRow.tsx (1)
Line range hint
76-82: Remove code coverage disable statements and add tests.The
istanbul ignore nextstatements are preventing code coverage for critical PDF generation logic. These statements should be removed, and appropriate tests should be added to cover this functionality.Let's verify the presence of tests for this component:
🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] Contains code coverage disable statement. Remove the disable statement and add appropriate tests.
src/components/LeftDrawerOrg/LeftDrawerOrg.tsx (2)
177-183: LGTM! Style consistency improvements.The button styling changes maintain consistency with the application's new color scheme.
189-192: LGTM! Icon color consistency.The icon color updates align with the new color scheme using 'var(--bs-black)' for active states.
src/screens/OrganizationPeople/OrganizationPeople.tsx (4)
178-192: LGTM! Row numbering implementation maintains correct order during sorting.The implementation correctly uses
getSortedRowIds()to ensure row numbers stay accurate even when the table is sorted.
201-201: LGTM! Improved code maintainability.Moved inline styles to CSS classes and simplified the Avatar component usage, making the code more maintainable.
Also applies to: 217-223
334-388: LGTM! Enhanced accessibility and testability.The dropdown implementation includes proper aria-labels and data-testid attributes, improving accessibility and testing capabilities.
417-435: LGTM! Styling aligns with Figma Style Guide.The DataGrid styling changes improve visual consistency by:
- Adding proper border and border-radius
- Removing unnecessary internal borders
- Adding consistent margin
src/screens/OrganizationPeople/AddMember.tsx (2)
107-124: LGTM! Well-implemented custom hook.The
useModalhook follows React best practices:
- Uses
useCallbackto memoize functions- Provides a clean API for modal state management
365-368: LGTM! Consistent translation key usage.Updated translation keys align with the changes in the translation files.
src/screens/OrgList/OrgList.tsx (1)
357-357: LGTM! Styling aligns with Figma Style Guide.Updated class names for consistent styling across the application.
Also applies to: 366-366
package.json (1)
Line range hint
1-186: Unauthorized modification of package.json detected.The pipeline indicates that modifications to package.json are not authorized. Please ensure you have the necessary permissions and review from the maintainers before modifying this file.
🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] Unauthorized modification or deletion of sensitive file
src/assets/css/app.css (2)
2481-2483: LGTM! Focus state colors updated.The form control focus state colors have been updated to match the Figma style guide.
3502-3510: LGTM! Primary button styles aligned with Figma.The primary button styles have been updated to match the Figma style guide:
- Background colors updated for different states
- Unnecessary border styles removed
There was a problem hiding this comment.
- You have edited files that are out of scope of the issue.
- In hindsight, I forgot that I made a suggestion for the left drawer. That should be excluded, but you have also made changes to the Organization, Checkin, Event Management pages and more.
Please make updates.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Let me know when this is ready for review. |
|
can you tell me how to resolve this code covarage issue |
|
as seen in the screenshot src/components/CheckIn/TableRow.tsx has coverage disable statements example: /* istanbul ignore else */ remove those and add necessary tests |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/components/CheckIn/TableRow.spec.tsx (1)
Line range hint
21-35: Add proper TypeScript types to the helper function.The
renderWithProvidersfunction improves code reuse, but it needs better type safety:-const renderWithProviders = (props: any) => +interface TableRowProps { + data: { + id: string; + name: string; + userId: string; + checkIn: { _id: string; time: string; } | null; + eventId: string; + }; + refetch: () => void; +} + +const renderWithProviders = (props: TableRowProps): ReturnType<typeof render> =>🧰 Tools
🪛 eslint
[error] 21-21: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 21-21: Missing return type on function.
(@typescript-eslint/explicit-function-return-type)
🧹 Nitpick comments (7)
src/components/CheckIn/TableRow.spec.tsx (2)
38-47: Reduce test data duplication using test fixtures.The same test data structure is repeated across multiple tests. Consider extracting it into a shared fixture:
const createTestProps = (overrides = {}) => ({ data: { id: '123', name: 'John Doe', userId: 'user123', checkIn: null, eventId: 'event123', }, refetch: vi.fn(), ...overrides, }); // Usage in tests: const props = createTestProps(); // Or with overrides: const props = createTestProps({ data: { ...createTestProps().data, checkIn: { _id: '123', time: '12:00:00' }, }, });Also applies to: 89-95, 122-131, 152-163
Line range hint
16-176: Add missing test cases for edge scenarios.The test suite could benefit from additional test cases:
- Test with invalid/malformed check-in data
- Test with empty user name
- Test PDF generation with special characters in the name
- Test concurrent check-in attempts
Would you like me to help implement these test cases?
src/style/app.module.css (3)
121-122: Consider splitting complex shadow values into separate variables.The shadow definition is complex and could be hard to maintain. Consider splitting it into separate variables for better reusability and maintainability.
:root { - --input-shadow-color: 4px 4px 10px -1px rgb(168, 168, 168, 0.7), - /* Shadow on bottom-right */ 2px 2px 10px -1px rgba(169, 169, 169, 0.7); + --shadow-offset-primary: 4px 4px 10px -1px; + --shadow-offset-secondary: 2px 2px 10px -1px; + --shadow-color: rgba(168, 168, 168, 0.7); + --input-shadow-color: var(--shadow-offset-primary) var(--shadow-color), + var(--shadow-offset-secondary) var(--shadow-color); }
5332-5336: Consider dynamic grid rows for calendar layout.The calendar grid is fixed to 6 rows which might not handle edge cases where a month spans across more weeks. Consider making the grid rows dynamic.
.calendar__days { display: grid; grid-template-columns: repeat(7, minmax(0, 1fr)); - grid-template-rows: repeat(6, 1fr); + grid-auto-rows: 1fr; }
5637-5642: Use CSS variables for breakpoints in media queries.The media queries use hardcoded pixel values. Consider using the defined breakpoint variables (
--breakpoint-mobile,--breakpoint-tablet,--breakpoint-desktop) for consistency.- @media (max-width: 520px) { + @media (max-width: var(--breakpoint-mobile)) { /* .input { width: 100%; } .createButton { margin: 0 auto; width: 100%; } */ }src/assets/css/app.css (2)
2481-2483: Consider accessibility in form focus styles.The light blue focus styles may not provide sufficient contrast for accessibility. Consider using a darker shade or increasing the opacity.
- border-color: #98b3dd; + border-color: #4a75c4; - box-shadow: 0 0 0 0.25rem rgba(155, 174, 211, 0.5); + box-shadow: 0 0 0 0.25rem rgba(74, 117, 196, 0.5);
6106-6106: Maintain consistency in focus shadow colors.The button close focus shadow should match the form control focus shadow for visual consistency.
- --bs-btn-close-focus-shadow: 0 0 0 0.25rem rgba(155, 174, 211, 0.5); + --bs-btn-close-focus-shadow: 0 0 0 0.25rem rgba(74, 117, 196, 0.5);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/assets/css/app.css(7 hunks)src/components/CheckIn/TableRow.spec.tsx(4 hunks)src/components/CheckIn/TableRow.tsx(1 hunks)src/screens/OrganizationPeople/AddMember.tsx(13 hunks)src/style/app.module.css(42 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/CheckIn/TableRow.tsx
- src/screens/OrganizationPeople/AddMember.tsx
🧰 Additional context used
🪛 eslint
src/components/CheckIn/TableRow.spec.tsx
[error] 21-21: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 21-21: Missing return type on function.
(@typescript-eslint/explicit-function-return-type)
🪛 Biome (1.9.4)
src/assets/css/app.css
[error] 81-81: Unexpected value or character.
Expected one of:
(parse)
src/style/app.module.css
[error] 599-599: Unknown property is not allowed.
See CSS Specifications and browser specific properties for more details.
To resolve this issue, replace the unknown property with a valid CSS property.
(lint/correctness/noUnknownProperty)
[error] 600-600: Unknown property is not allowed.
See CSS Specifications and browser specific properties for more details.
To resolve this issue, replace the unknown property with a valid CSS property.
(lint/correctness/noUnknownProperty)
🔇 Additional comments (3)
src/components/CheckIn/TableRow.spec.tsx (1)
16-19: Well-structured test suite with proper setup and cleanup!The test suite follows best practices with clear organization, proper mock cleanup, and descriptive test cases.
src/assets/css/app.css (2)
81-81:⚠️ Potential issueFix CSS variable syntax error.
The CSS variable declaration is missing a colon after the variable name.
Apply this diff to fix the syntax:
- --bs-body-color 'Liberation Mono', 'Courier New', monospace; + --bs-body-color: 'Liberation Mono', 'Courier New', monospace;Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 81-81: Unexpected value or character.
Expected one of:
(parse)
14073-14104: 🛠️ Refactor suggestionReview button success styles for accessibility and consistency.
The button success styles have several potential issues:
- The gray text color (#707070) may not provide sufficient contrast against the background
- The hover/active state transitions could be simplified
- The use of !important flags should be minimized
Consider these improvements:
.btn-success { - color: #707070; + color: #2c5f2d; } .btn-success:hover, .btn-success:active { - background-color: #e0eaf6 !important; + background-color: #e0eaf6; } .btn-success:active svg path { - fill: #ffffff !important; - transition: fill 0.32s !important; + fill: #ffffff; + transition: fill 0.32s; } .btn-success .text-secondary { - transition: color 0.32s !important; + transition: color 0.32s; }Likely invalid or redundant comment.
| .profileImage { | ||
| display: flex; | ||
| justifycontent: center; | ||
| alignitems: center; | ||
| height: 100%; | ||
| width: 100%; |
There was a problem hiding this comment.
Fix invalid CSS properties in .profileImage class.
The properties justifycontent and alignitems are invalid. They should be hyphenated as justify-content and align-items respectively.
.profileImage {
display: flex;
- justifycontent: center;
- alignitems: center;
+ justify-content: center;
+ align-items: center;
height: 100%;
width: 100%;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .profileImage { | |
| display: flex; | |
| justifycontent: center; | |
| alignitems: center; | |
| height: 100%; | |
| width: 100%; | |
| .profileImage { | |
| display: flex; | |
| justify-content: center; | |
| align-items: center; | |
| height: 100%; | |
| width: 100%; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 599-599: Unknown property is not allowed.
See CSS Specifications and browser specific properties for more details.
To resolve this issue, replace the unknown property with a valid CSS property.
(lint/correctness/noUnknownProperty)
[error] 600-600: Unknown property is not allowed.
See CSS Specifications and browser specific properties for more details.
To resolve this issue, replace the unknown property with a valid CSS property.
(lint/correctness/noUnknownProperty)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/style/app.module.css (5)
136-137: Improve shadow definition readability.The shadow definition spans multiple lines with inline comments that make it harder to read. Consider using a more concise format.
- --input-shadow-color: 4px 4px 10px -1px rgb(168, 168, 168, 0.7), - /* Shadow on bottom-right */ 2px 2px 10px -1px rgba(169, 169, 169, 0.7); /* Lighter shadow on bottom-right */ + --input-shadow-color: + 4px 4px 10px -1px rgb(168, 168, 168, 0.7), + 2px 2px 10px -1px rgba(169, 169, 169, 0.7);🧰 Tools
🪛 GitHub Actions: PR Workflow
[warning] Code style issues found. Run Prettier with --write to fix formatting issues.
4836-4840: Improve calendar grid layout definition.The calendar grid layout is defined with hardcoded values. Consider using CSS custom properties for better maintainability.
.calendar__days { display: grid; grid-template-columns: repeat(7, minmax(0, 1fr)); - grid-template-rows: repeat(6, 1fr); + grid-template-rows: repeat(var(--calendar-rows, 6), 1fr); }🧰 Tools
🪛 GitHub Actions: PR Workflow
[warning] Code style issues found. Run Prettier with --write to fix formatting issues.
341-343: Remove commented out margin properties.The code contains commented out margin properties that should be removed to maintain clean code.
.dropdown { background-color: var(--bs-white) !important; min-width: 150px; border: 1px solid var(--brown-color); color: var(--brown-color) !important; position: relative; display: inline-block; - /* margin-top: 10px; - margin-bottom: 10px; */ - /* margin-left:30px; */ }🧰 Tools
🪛 GitHub Actions: PR Workflow
[warning] Code style issues found. Run Prettier with --write to fix formatting issues.
479-479: Remove commented out box-shadow property.The code contains a commented out box-shadow property that should be removed.
.inputField { margin-top: 10px; margin-bottom: 10px; background-color: var(--bs-white); - /* box-shadow: var(--input-shadow-color); */ }Also applies to: 480-480
🧰 Tools
🪛 GitHub Actions: PR Workflow
[warning] Code style issues found. Run Prettier with --write to fix formatting issues.
5680-5680: Remove commented out width properties.The code contains commented out width properties that should be removed.
- /* .input { - width: 100%; - } - .createButton { - margin: 0 auto; - width: 100%; - } */Also applies to: 5685-5685
🧰 Tools
🪛 GitHub Actions: PR Workflow
[warning] Code style issues found. Run Prettier with --write to fix formatting issues.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/style/app.module.css(43 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/style/app.module.css
[error] 628-628: Unknown property is not allowed.
See CSS Specifications and browser specific properties for more details.
To resolve this issue, replace the unknown property with a valid CSS property.
(lint/correctness/noUnknownProperty)
[error] 629-629: Unknown property is not allowed.
See CSS Specifications and browser specific properties for more details.
To resolve this issue, replace the unknown property with a valid CSS property.
(lint/correctness/noUnknownProperty)
🪛 GitHub Actions: PR Workflow
src/style/app.module.css
[warning] Code style issues found. Run Prettier with --write to fix formatting issues.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (2)
src/style/app.module.css (2)
1721-1728: Standardize button hover/focus states.Button hover/focus states use inconsistent shadow values. Align with Figma style guide specifications.
🧰 Tools
🪛 GitHub Actions: PR Workflow
[warning] Code style issues found. Run Prettier with --write to fix formatting issues.
626-631:⚠️ Potential issueFix invalid CSS properties in
.profileImageclass.The properties
justifycontentandalignitemsare invalid. They should be hyphenated asjustify-contentandalign-itemsrespectively..profileImage { display: flex; - justifycontent: center; - alignitems: center; + justify-content: center; + align-items: center; height: 100%; width: 100%; }Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 628-628: Unknown property is not allowed.
See CSS Specifications and browser specific properties for more details.
To resolve this issue, replace the unknown property with a valid CSS property.(lint/correctness/noUnknownProperty)
[error] 629-629: Unknown property is not allowed.
See CSS Specifications and browser specific properties for more details.
To resolve this issue, replace the unknown property with a valid CSS property.(lint/correctness/noUnknownProperty)
🪛 GitHub Actions: PR Workflow
[warning] Code style issues found. Run Prettier with --write to fix formatting issues.
|
What kind of change does this PR introduce?
bugfix, feature, refactoring
Issue Number:
Fixes #3171
Snapshots/Videos:
2025-01-20.22-28-35.mp4
If relevant, did you update the documentation?
no
Summary
it refacors design of menu and components of menu
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
Localization
UI/UX Improvements
Code Quality
Testing