Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Figma: People Page Violates The Figma Style Guide #3401

Closed

Conversation

abhassen44
Copy link

@abhassen44 abhassen44 commented Jan 23, 2025

What kind of change does this PR introduce?

bugfix, feature, refactoring

Issue Number:

Fixes #3171

Snapshots/Videos:

Screenshot 2025-01-23 213738

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

  • I have reviewed and addressed all critical issues flagged by CodeRabbit AI
  • I have implemented or provided justification for each non-critical suggestion
  • I have documented my reasoning in the PR comments where CodeRabbit AI suggestions were not implemented

Test Coverage

  • I have written tests for all new changes/features
  • I have verified that test coverage meets or exceeds 95%
  • I have run the test suite locally and all tests pass

Have you read the contributing guide?

Yes

Summary by CodeRabbit

Release Notes

  • Localization

    • Updated capitalization for "User" and "Profile" in English translations.
  • UI/UX Improvements

    • Enhanced button styles with improved hover and active states.
    • Added new left drawer organization component with responsive design.
    • Introduced sorting dropdown in the organization people screen.
    • Added a button for super admins to create organizations directly from the list view.
    • Refined organization people and event management screens.
  • Styling

    • Updated CSS variables and color schemes.
    • Improved component-specific styling for dropdowns and buttons.
    • Enhanced visual representation of the left drawer component.
  • Code Quality

    • Implemented type-safe parameter handling.
    • Introduced custom modal management hook.
    • Simplified component logic and state management.
  • Performance

    • Optimized rendering and interaction for various components.

Copy link
Contributor

coderabbitai bot commented Jan 23, 2025

Warning

Rate limit exceeded

@abhassen44 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 46 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

📥 Commits

Reviewing files that changed from the base of the PR and between 976e9f5 and e76cfec.

📒 Files selected for processing (3)
  • docs/docs/auto-docs/components/LeftDrawerOrg/LeftDrawerOrg/functions/default.md (1 hunks)
  • docs/docs/auto-docs/utils/recurrenceUtils/recurrenceConstants/variables/Days.md (0 hunks)
  • src/assets/css/app.css (7 hunks)

Walkthrough

This pull request encompasses multiple changes across various files in the Talawa Admin project, focusing on styling, translations, and component improvements. The modifications include updating translation files, refactoring CSS styles, enhancing component type safety, and standardizing UI elements. The changes aim to improve the overall user interface consistency, type safety, and adherence to the Figma design guidelines.

Changes

File Change Summary
public/locales/en/translation.json Updated capitalization of user and profile keys
src/assets/css/app.css Added .btn-success class and updated button styles
src/components/CheckIn/TableRow.spec.tsx Added TestInterfaceTableRowProps interface and refactored test cases
src/components/CheckIn/TableRow.tsx Modified PDF blob creation and success toast message handling
src/components/CollapsibleDropdown/CollapsibleDropdown.tsx Updated button and icon styling based on state
src/components/LeftDrawerOrg/LeftDrawerOrg.module.css Added new CSS classes and keyframe animations
src/components/LeftDrawerOrg/LeftDrawerOrg.tsx Updated component naming and styling logic
src/screens/EventManagement/EventManagement.tsx Added Params type for URL parameters
src/screens/OrgList/OrgList.tsx Modified search input and button styling
src/screens/OrganizationPeople/AddMember.tsx Introduced useModal hook and updated translations
src/screens/OrganizationPeople/OrganizationPeople.tsx Added sorting functionality and updated component
src/style/app.module.css Significant CSS variable and class updates

Assessment against linked issues

Objective Addressed Explanation
Conform to Figma Style Guide [#3171]
Consolidate CSS into src/style/app.module.css [#3171]
Use descriptive color variable names [#3171]
Remove HTML color codes except in app.module.css [#3171]

Possibly related issues

Possibly related PRs

  • Plugin and advertisement screen revamp #2006: The main PR modifies the public/locales/en/translation.json file, specifically changing keys related to the "Add On Store" section, which is similar to the changes made in the same file in this retrieved PR, where the title of the "Add On Store" section is changed to "Plugin Store".
  • Fix Misplaced Screen Title & Fix Layout Structure #2412: The main PR updates the public/locales/en/translation.json file, changing the title casing for keys related to user and profile, which is related to the changes in the same file in this retrieved PR, where translation keys are modified for consistency in the advertisement section.
  • changed color schema for the tag and action item screen #2417: The main PR modifies the public/locales/en/translation.json file, which is relevant to the changes in the same file in this retrieved PR, where new keys related to user tag management are added.

Suggested labels

ignore-sensitive-files-pr

Suggested reviewers

  • palisadoes

Poem

🐰 A Rabbit's Ode to Styling Delight
In lines of code, a transformation bright,
CSS variables dance with might,
Translations polished, components sleek,
A UI journey, no longer weak!
Hop, hop, hurray for clean design! 🎨


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

Our Pull Request Approval Process

Thanks for contributing!

Testing Your Code

Remember, your PRs won't be reviewed until these criteria are met:

  1. We don't merge PRs with poor code quality.
    1. Follow coding best practices such that CodeRabbit.ai approves your PR.
  2. We don't merge PRs with failed tests.
    1. When tests fail, click on the Details link to learn more.
    2. Write sufficient tests for your changes (CodeCov Patch Test). Your testing level must be better than the target threshold of the repository
    3. Tests may fail if you edit sensitive files. Ask to add the ignore-sensitive-files-pr label if the edits are necessary.
  3. We cannot merge PRs with conflicting files. These must be fixed.

Our policies make our code better.

Reviewers

Do not assign reviewers. Our Queue Monitors will review your PR and assign them.
When your PR has been assigned reviewers contact them to get your code reviewed and approved via:

  1. comments in this PR or
  2. our slack channel

Reviewing Your Code

Your reviewer(s) will have the following roles:

  1. arbitrators of future discussions with other contributors about the validity of your changes
  2. point of contact for evaluating the validity of your work
  3. person who verifies matching issues by others that should be closed.
  4. person who gives general guidance in fixing your tests

CONTRIBUTING.md

Read our CONTRIBUTING.md file. Most importantly:

  1. PRs with issues not assigned to you will be closed by the reviewer
  2. Fix the first comment in the PR so that each issue listed automatically closes

Other

  1. 🎯 Please be considerate of our volunteers' time. Contacting the person who assigned the reviewers is not advised unless they ask for your input. Do not @ the person who did the assignment otherwise.
  2. Read the CONTRIBUTING.md file make

@abhassen44
Copy link
Author

checks were failing not because of my files please check

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (16)
src/components/LeftDrawerOrg/LeftDrawerOrg.tsx (1)

177-183: Simplify boolean comparisons.

The strict equality comparison with true is redundant and can be simplified.

-variant={isActive === true ? 'success' : ''}
+variant={isActive ? 'success' : ''}
-backgroundColor: isActive === true ? '#EAEBEF' : '',
+backgroundColor: isActive ? '#EAEBEF' : '',
-isActive === true ? 'text-black' : 'text-secondary'
+isActive ? 'text-black' : 'text-secondary'
-isActive === true ? 'var(--bs-black)' : 'var(--bs-secondary)'
+isActive ? 'var(--bs-black)' : 'var(--bs-secondary)'

Also applies to: 189-191

src/components/LeftDrawerOrg/LeftDrawerOrg.module.css (3)

1-14: Consider using CSS variables for z-index values.

For better maintainability and consistency, consider defining z-index values as CSS variables.

+:root {
+  --drawer-z-index: 100;
+}

.leftDrawer {
-  z-index: 100;
+  z-index: var(--drawer-z-index);
}

.activeDrawer {
+  z-index: var(--drawer-z-index);
}

Also applies to: 25-40


184-285: Consider using CSS variables for breakpoints.

For better maintainability and reusability, consider defining breakpoints as CSS variables.

+:root {
+  --breakpoint-tablet: 900px;
+  --breakpoint-mobile: 820px;
+  --breakpoint-small: 650px;
+}

-@media (max-width: 1120px) {
+@media (max-width: var(--breakpoint-desktop)) {
  /* ... */
}

-@media (max-height: 900px) {
+@media (max-height: var(--breakpoint-tablet)) {
  /* ... */
}

287-383: Optimize animations for performance.

Consider using transform instead of animating left property for better performance. This will utilize GPU acceleration.

@keyframes goToLeftBigScreen {
  from {
-    left: 0;
+    transform: translateX(0);
  }

  to {
    opacity: 0.1;
-    left: calc(-300px - 2rem);
+    transform: translateX(calc(-300px - 2rem));
  }
}
src/components/CheckIn/TableRow.tsx (1)

75-81: Add cleanup and improve error handling.

Consider these improvements:

  1. Clean up the blob URL to prevent memory leaks
  2. Add error handling for window.open call
-      const blob = new Blob([pdf], { type: 'application/pdf' });
-      const url = URL.createObjectURL(blob);
-      window.open(url);
-      toast.success('PDF generated successfully!');
+      const blob = new Blob([pdf], { type: 'application/pdf' });
+      const url = URL.createObjectURL(blob);
+      const newWindow = window.open(url);
+      if (newWindow === null) {
+        throw new Error('Failed to open PDF in new window');
+      }
+      toast.success('PDF generated successfully!');
+      // Clean up the blob URL after the window is loaded
+      newWindow.onload = () => {
+        URL.revokeObjectURL(url);
+      };
src/components/CollapsibleDropdown/CollapsibleDropdown.tsx (2)

46-46: Use CSS variables for better maintainability.

Consider using CSS variables for the text and icon colors to maintain consistency and make future updates easier.

-        className={showDropdown ? 'text-black' : 'text-secondary'}
+        className={showDropdown ? 'var(--text-primary)' : 'var(--text-secondary)'}

-            fill={showDropdown ? 'var(--bs-black)' : 'var(--bs-secondary)'}
+            fill={showDropdown ? 'var(--icon-primary)' : 'var(--icon-secondary)'}

-          ${showDropdown ? 'var(--bs-black)' : 'var(--bs-secondary)'} 
+          ${showDropdown ? 'var(--icon-primary)' : 'var(--icon-secondary)'} 

Also applies to: 54-54, 60-60


77-77: Use consistent CSS variables for active state colors.

Use the same CSS variables as the main button for consistency.

-                        isActive === true ? 'text-black' : 'text-secondary'
+                        isActive === true ? 'var(--text-primary)' : 'var(--text-secondary)'

-                            isActive === true ? 'text-black' : 'text-secondary'
+                            isActive === true ? 'var(--icon-primary)' : 'var(--icon-secondary)'

Also applies to: 91-91

src/components/CheckIn/TableRow.spec.tsx (2)

135-162: Add test cases for other error scenarios.

Consider adding test cases for:

  1. Invalid PDF template
  2. Network errors during PDF generation
  3. Memory errors during blob creation

164-189: Add content verification to the PDF download test.

Consider adding verifications for:

  1. PDF content matches the expected template
  2. PDF metadata (e.g., title, author)
  3. PDF size constraints
src/screens/EventManagement/EventManagement.tsx (1)

108-111: Move type definition to a separate types file.

Consider moving the Params type to a separate types file (e.g., src/types/event.ts) for better organization and reusability.

+// src/types/event.ts
+export type EventParams = {
+  eventId: string;
+  orgId: string;
+};

// src/screens/EventManagement/EventManagement.tsx
+import type { EventParams } from 'types/event';

-  type Params = {
-    eventId: string;
-    orgId: string;
-  };
-  const { eventId, orgId } = useParams<Params>();
+  const { eventId, orgId } = useParams<EventParams>();
src/screens/OrgList/OrgList.tsx (1)

357-357: Consider using module-scoped class for input field styling.

Using a static class name 'bg-white' instead of a module-scoped class could lead to style conflicts. Consider using a module-scoped class from the styles module to maintain better style encapsulation.

-className={'bg-white'}
+className={styles.inputField}
src/style/app.module.css (3)

382-384: Remove commented out code and clean up margins.

The color changes for hover states look good, but there's unnecessary commented code that should be removed.

.dropdown {
  background-color: var(--bs-white) !important;
  min-width: 150px;
  border: 1px solid var(--davys-grey);
  color: var(--davys-grey) !important;
  position: relative;
  display: inline-block;
-  /* margin-top: 10px;
-  margin-bottom: 10px; */
-  /* margin-left:30px; */
}

Also applies to: 387-390, 403-407


525-539: Remove duplicate focus state for inputField.

The hover and focus states look good, but there's a duplicate focus state that should be removed.

.inputField {
  margin-top: 10px;
  margin-bottom: 10px;
  background-color: var(--bs-white);
}

.inputField:focus {
  background-color: var(--bs-white); /* Ensure no color change on focus */
}

.inputField:hover {
  background-color: var(--bs-white);
  box-shadow: var(--input-shadow-color);
}

- .inputField:focus {
-   background-color: var(--bs-white);
-   box-shadow: var(--input-shadow-color);
- }

Line range hint 581-590: Remove duplicate active state for searchButton.

The active state styles look good, but there's a duplicate block that should be consolidated.

.searchButton:active {
  transform: scale(0.95);
  background-color: var(--search-button-bg) !important;
  border-color: var(--search-button-bg) !important;
  box-shadow: 0px 4px 10px var(--search-button-shadow);
}

- .searchButton:active {
-   transform: scale(0.95);
-   background-color: var(--search-button-bg) !important;
-   border-color: transparent !important;
- }
src/screens/OrganizationPeople/AddMember.tsx (2)

107-124: Great refactoring with the useModal hook!

The custom hook nicely encapsulates modal state management logic, improving code reusability and maintainability.

Consider adding TypeScript generics to support modals with different state types:

-function useModal(initialState = false): {
+function useModal<T = boolean>(initialState: T): {
-  isOpen: boolean;
+  isOpen: T;
-  open: () => void;
+  open: () => void;
   close: () => void;
   toggle: () => void;
 }

554-561: Move inline styles to CSS modules.

Consider moving the inline styles to the CSS module for better maintainability and consistency with the project's CSS strategy.

-style={{
-  border: '1px solid var(--dropdown-border-color)',
-  width: '100%',
-}}
+className={`${styles.colorPrimary} ${styles.borderNone} ${styles.createButton}`}

Add to your CSS module:

.createButton {
  border: 1px solid var(--dropdown-border-color);
  width: 100%;
}
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ece035f and 460a02b.

📒 Files selected for processing (12)
  • public/locales/en/translation.json (1 hunks)
  • src/assets/css/app.css (7 hunks)
  • src/components/CheckIn/TableRow.spec.tsx (4 hunks)
  • src/components/CheckIn/TableRow.tsx (1 hunks)
  • src/components/CollapsibleDropdown/CollapsibleDropdown.tsx (3 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 (45 hunks)
✅ Files skipped from review due to trivial changes (1)
  • public/locales/en/translation.json
🧰 Additional context used
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
src/components/CheckIn/TableRow.spec.tsx

[failure] 16-16:
Interface name TableRowProps must have one of the following prefixes: Interface, TestInterface

🪛 GitHub Actions: PR Workflow
src/components/CheckIn/TableRow.spec.tsx

[error] 16-16: Interface name 'TableRowProps' must have one of the following prefixes: Interface, TestInterface

src/assets/css/app.css

[warning] File ignored because of a matching ignore pattern. Use "--no-ignore" to override

🪛 Biome (1.9.4)
src/style/app.module.css

[error] 672-672: 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] 673-673: 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)

src/assets/css/app.css

[error] 81-81: Unexpected value or character.

Expected one of:

(parse)

🔇 Additional comments (23)
src/components/LeftDrawerOrg/LeftDrawerOrg.tsx (1)

124-129: LGTM: Well-implemented loading state.

Good use of semantic HTML with proper accessibility attributes and shimmer effect for the loading state.

src/screens/OrganizationPeople/OrganizationPeople.tsx (8)

22-23: LGTM!

The imports are correctly added and align with the new UI components being used.


42-42: LGTM!

The type annotation for useParams enhances type safety by explicitly defining the expected parameter type.


178-192: LGTM!

The row number column implementation is well-designed:

  • Maintains correct row numbers even after sorting using getSortedRowIds()
  • Properly configured with appropriate styling and alignment

201-201: LGTM!

Good improvement in styling implementation:

  • Moved styles to CSS classes for better maintainability
  • Follows best practices by avoiding inline styles

Also applies to: 217-217


220-223: LGTM!

Good improvement in fallback image handling:

  • Uses Avatar component for a better fallback solution
  • Props are correctly passed to the component

327-327: LGTM!

Added data-testid attribute to improve testability.


334-388: LGTM!

Well-implemented dropdown component:

  • Provides better UX with standard dropdown behavior
  • Properly configured with necessary attributes and event handlers
  • Includes data-testid attributes for testing

417-435: LGTM!

Good improvements in DataGrid styling:

  • Enhanced visual appearance with proper border handling
  • Uses CSS variables for consistent theming
src/screens/OrgList/OrgList.tsx (2)

366-366: LGTM!

Good improvement in class naming:

  • Uses a more specific class name for the search button
  • Follows better naming convention by being component-specific

Line range hint 385-395: LGTM!

Well-implemented create organization button:

  • Correctly checks for superAdmin permission
  • Proper button styling and icon spacing
src/style/app.module.css (6)

98-98: LGTM! Shadow and color variable improvements.

The changes improve the visual consistency and depth perception:

  • Table row background now reuses the dropdown hover color for consistency
  • Input shadow uses a layered effect for better depth perception

Also applies to: 151-152


5516-5519: LGTM! Calendar grid improvement.

The calendar grid now properly handles months that span 6 weeks by increasing the grid template rows.


3700-3711: LGTM! Improved left drawer styling and animations.

The left drawer changes improve the user experience with:

  • Better positioning and width calculations
  • Smooth transitions for animations
  • Consistent use of CSS variables

3609-3611: LGTM! Icon wrapper active state.

The active state for icons provides clear visual feedback using white fill color.


1765-1772: LGTM! Enhanced button hover effects.

The button hover states now provide better visual feedback with:

  • Card shadow effect
  • White background color

1107-1108: LGTM! Toggle button hover effect.

The toggle button hover state uses the light green color variable consistently.

src/assets/css/app.css (3)

2481-2483: LGTM: Form control focus styles updated for consistency

The focus state styles have been updated to use consistent colors that align with the style guide.

🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] File ignored because of a matching ignore pattern. Use "--no-ignore" to override


6112-6112: LGTM: Close button focus shadow matches form controls

The close button focus shadow has been updated to maintain consistency with form control focus states.

🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] File ignored because of a matching ignore pattern. Use "--no-ignore" to override


14079-14110: LGTM: Success button styles updated per design

The success button styles have been updated to match the Figma style guide:

  • Text color set to #707070
  • Hover/active states use #e0eaf6 background
  • Added smooth transition for SVG icon colors
🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] File ignored because of a matching ignore pattern. Use "--no-ignore" to override

src/screens/OrganizationPeople/AddMember.tsx (3)

142-142: Good type safety improvement!

Adding the type parameter to useParams helps catch potential issues at compile time.


365-368: Good translation key organization!

Using translateOrgPeople for organization-related translations improves consistency and maintainability.


39-40: Good use of CSS variables!

Using CSS variables for colors improves theme consistency and maintainability.

src/components/CheckIn/TableRow.spec.tsx Outdated Show resolved Hide resolved
src/style/app.module.css Outdated Show resolved Hide resolved
src/assets/css/app.css Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
src/style/app.module.css (1)

670-675: ⚠️ Potential issue

Fix invalid CSS properties in profileImage class.

The CSS properties need to be hyphenated.

Apply this diff to fix the properties:

.profileImage {
  display: flex;
-  justify-content: center;
-  align-items: center;
+  justify-content: center;
+  align-items: center;
  height: 100%;
  width: 100%;
}
🧹 Nitpick comments (7)
src/components/CheckIn/TableRow.spec.tsx (3)

Line range hint 102-133: Use the helper function for consistency.

This test doesn't use the renderWithProviders helper function like other tests. For consistency and maintainability, consider using the helper function here as well.

-    const { findByText } = render(
-      <BrowserRouter>
-        <LocalizationProvider dateAdapter={AdapterDayjs}>
-          <MockedProvider addTypename={false} mocks={checkInMutationUnsuccess}>
-            <Provider store={store}>
-              <I18nextProvider i18n={i18nForTest}>
-                <ToastContainer />
-                <TableRow {...props} />
-              </I18nextProvider>
-            </Provider>
-          </MockedProvider>
-        </LocalizationProvider>
-      </BrowserRouter>,
-    );
+    const { findByText } = renderWithProviders(props);

Line range hint 72-100: Enhance mock cleanup consistency.

The mock cleanup for URL.createObjectURL and window.open is inconsistent across tests. Consider adding cleanup in the beforeEach or afterEach block to ensure a clean state for each test.

 describe('Testing TableRow component', () => {
   beforeEach(() => {
     vi.clearAllMocks();
+    global.URL.createObjectURL = vi.fn(() => 'mockURL');
+    global.window.open = vi.fn();
   });
+
+  afterEach(() => {
+    vi.restoreAllMocks();
+  });

Also applies to: 135-162, 164-189


53-70: Add assertions for component state changes.

The check-in test could be enhanced by verifying the component's state changes after clicking the button. Consider adding assertions for loading state or disabled state of the button during the check-in process.

     expect(await findByText('Check In')).toBeInTheDocument();
+    const button = await findByText('Check In');
+    expect(button).not.toBeDisabled();
     fireEvent.click(await findByText('Check In'));
+    expect(button).toBeDisabled();
     expect(await findByText('Checked in successfully')).toBeInTheDocument();
+    expect(props.refetch).toHaveBeenCalled();
src/components/LeftDrawerOrg/LeftDrawerOrg.tsx (3)

124-129: Add accessibility attributes to the loading button.

While the structural change is good, the loading button should include proper accessibility attributes to improve user experience for screen readers.

 <>
   <button
     className={`${styles.profileContainer} shimmer`}
     data-testid="orgBtn"
+    aria-label="Loading organization information"
+    aria-busy="true"
+    disabled
   />
 </>

177-183: Optimize active state styling.

The current implementation has redundant strict equality checks and separated style properties.

-variant={isActive === true ? 'success' : ''}
-style={{
-  backgroundColor: isActive === true ? '#EAEBEF' : '',
-}}
-className={`${
-  isActive === true ? 'text-black' : 'text-secondary'
-}`}
+variant={isActive ? 'success' : undefined}
+className={`${isActive ? 'text-black bg-light' : 'text-secondary'}`}

189-191: Simplify icon fill condition.

The strict equality check is redundant for a boolean value.

-fill={
-  isActive === true
-    ? 'var(--bs-black)'
-    : 'var(--bs-secondary)'
-}
+fill={isActive ? 'var(--bs-black)' : 'var(--bs-secondary)'}
src/style/app.module.css (1)

Line range hint 5798-5825: Use CSS variables for breakpoint values in media queries.

Replace hardcoded pixel values with the defined breakpoint variables for better maintainability.

Apply this pattern to media queries:

-@media (max-width: 520px) {
+@media (max-width: var(--breakpoint-mobile)) {
  .calendar__header {
    display: flex;
    flex-direction: column;
    align-items: stretch;
    gap: 10px;
  }
  /* ... */
}
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 460a02b and 8a0854d.

📒 Files selected for processing (4)
  • src/assets/css/app.css (7 hunks)
  • src/components/CheckIn/TableRow.spec.tsx (4 hunks)
  • src/components/LeftDrawerOrg/LeftDrawerOrg.tsx (6 hunks)
  • src/style/app.module.css (45 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/assets/css/app.css

[error] 81-81: Unexpected value or character.

Expected one of:

(parse)

🔇 Additional comments (12)
src/components/CheckIn/TableRow.spec.tsx (2)

16-28: Interface naming convention has been fixed correctly.

The interface name now follows the required prefix convention as specified in the previous review.


Line range hint 35-52: Well-structured test helper function.

Good job extracting the common rendering logic into a reusable helper function. This reduces code duplication and improves maintainability.

src/components/LeftDrawerOrg/LeftDrawerOrg.tsx (2)

14-14: LGTM! Good migration to CSS modules.

The change from global CSS to CSS modules improves style encapsulation and reduces the risk of style conflicts.


34-34: Component naming convention fixed.

The component has been properly renamed to follow React's PascalCase naming convention.

src/style/app.module.css (5)

525-539: LGTM! Enhanced input field interaction states.

The input field hover and focus states now provide better visual feedback with consistent shadow effects.


664-667: LGTM! Consistent table header styling.

The table header styles align well with the new color scheme while maintaining proper visual hierarchy.


5457-5460: LGTM! Enhanced calendar layout and spacing.

The calendar styles have been improved with better grid structure and consistent spacing.

Also applies to: 5466-5466, 5473-5477, 5487-5491


1765-1772: LGTM! Improved button hover interactions.

The button hover states now provide better visual feedback with smooth transitions and consistent shadows.


3609-3612: LGTM! Consistent icon active state.

The icon fill color on active state aligns with the overall color scheme.

src/assets/css/app.css (3)

2481-2483: LGTM! Form control focus state aligns with Figma style guide.

The updated focus state colors create a cohesive visual hierarchy with the blue-gray theme.


3508-3516: LGTM! Primary button styles match Figma design.

The updated button colors provide better visual hierarchy and contrast.


6112-6112: LGTM! Close button focus state maintains consistency.

The focus shadow color matches other interactive elements, creating a cohesive experience.

src/style/app.module.css Show resolved Hide resolved
src/assets/css/app.css Outdated Show resolved Hide resolved
@palisadoes
Copy link
Contributor

Please fix the conflicting files

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🔭 Outside diff range comments (1)
src/style/app.module.css (1)

Line range hint 1382-1387: Fix color contrast ratio for accessibility.

The white text on light green background (#31bb6b) has a contrast ratio of 2.73:1, which fails WCAG 2.1 Level AA requirements (minimum 4.5:1 for normal text, 3:1 for large text).

Consider using a darker shade of green or adjusting the text color to meet accessibility standards. For example:

.customcell {
-  background-color: var(--light-green) !important;
+  background-color: var(--dark-emerald-green) !important;
  color: var(--bs-white) !important;
  font-size: medium !important;
  font-weight: 500 !important;
  padding-top: 10px !important;
  padding-bottom: 10px !important;
}
🧹 Nitpick comments (1)
src/style/app.module.css (1)

90-95: LGTM! CSS variables enhance design system consistency.

The changes improve the design system by:

  1. Adding semantic variables for button states
  2. Using consistent color variables for table headers
  3. Enhancing shadow definition for better depth perception

Consider grouping related variables together (e.g., all button-related variables) and adding comments to document their intended usage.

Also applies to: 158-159, 201-204

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8a0854d and 364bdfc.

📒 Files selected for processing (1)
  • src/style/app.module.css (51 hunks)
🔇 Additional comments (10)
src/style/app.module.css (10)

404-407: LGTM! Enhanced dropdown accessibility and interactivity.

The changes improve dropdown behavior by:

  1. Adding proper focus and hover states
  2. Using semantic color variables
  3. Maintaining consistent styling across states

Also applies to: 420-424


546-560: LGTM! Enhanced input field interactivity.

The changes improve form field UX by:

  1. Adding consistent hover and focus states
  2. Maintaining background color across states
  3. Adding subtle shadow effects for depth

5014-5015: LGTM! Improved calendar grid layout and visual hierarchy.

The changes enhance the calendar component by:

  1. Using a consistent grid layout
  2. Adding clear visual distinction for days outside the current month

Also applies to: 5017-5018


3647-3649: LGTM! Added icon active state feedback.

The changes improve icon interactivity by adding visual feedback when active.


1788-1795: LGTM! Enhanced button hover and focus states.

The changes improve button interactivity by:

  1. Adding consistent hover and focus states
  2. Using subtle shadow effects
  3. Maintaining background color consistency

2872-2875: LGTM! Added consistent text styling class.

The addition of .blueText class promotes styling consistency across the application.


3738-3749: LGTM! Enhanced left drawer layout and styling.

The changes improve the left drawer by:

  1. Using consistent width calculations
  2. Implementing proper positioning
  3. Using semantic variables for colors
  4. Adding smooth transitions

1130-1132: LGTM! Added toggle button hover feedback.

The changes improve button interactivity by adding a color change on hover.


861-864: LGTM! Improved layout flexibility.

The removal of fixed height constraint allows for more dynamic content.


2001-2012: LGTM! Enhanced checkbox button accessibility.

The changes improve checkbox interaction by:

  1. Adding proper hover and focus states
  2. Using semantic color variables
  3. Implementing consistent styling

Copy link

codecov bot commented Jan 23, 2025

Codecov Report

Attention: Patch coverage is 0% with 28 lines in your changes missing coverage. Please review.

Project coverage is 7.92%. Comparing base (a8f2b25) to head (e76cfec).

Files with missing lines Patch % Lines
src/screens/OrganizationPeople/AddMember.tsx 0.00% 8 Missing ⚠️
.../screens/OrganizationPeople/OrganizationPeople.tsx 0.00% 8 Missing ⚠️
...onents/CollapsibleDropdown/CollapsibleDropdown.tsx 0.00% 5 Missing ⚠️
src/components/LeftDrawerOrg/LeftDrawerOrg.tsx 0.00% 5 Missing ⚠️
src/components/CheckIn/TableRow.tsx 0.00% 1 Missing ⚠️
src/screens/EventManagement/EventManagement.tsx 0.00% 1 Missing ⚠️
Additional details and impacted files
@@                 Coverage Diff                 @@
##           develop-postgres   #3401      +/-   ##
===================================================
- Coverage              7.93%   7.92%   -0.01%     
===================================================
  Files                   312     312              
  Lines                  8131    8135       +4     
  Branches               1815    1811       -4     
===================================================
  Hits                    645     645              
- Misses                 7419    7423       +4     
  Partials                 67      67              
Flag Coverage Δ
jest 7.92% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
src/assets/css/app.css (3)

2481-2483: Align form control focus state with brand colors.

The focus state uses a muted blue color (#98b3dd) which is inconsistent with the primary brand color (#31bb6b) used elsewhere in the application.

Consider using the primary brand color for better visual consistency:

.form-control:focus {
  color: var(--bs-body-color);
  background-color: #f2f2f2;
-  border-color: #98b3dd;
+  border-color: #98ddb5;
  outline: 0;
-  box-shadow: 0 0 0 0.25rem rgba(155, 174, 211, 0.5);
+  box-shadow: 0 0 0 0.25rem rgba(49, 187, 107, 0.25);
}
🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] Code style issues found. Run Prettier with --write to fix formatting issues.


6112-6112: Align close button focus state with brand colors.

The close button focus state uses the same muted blue color as form controls, which is inconsistent with the brand identity.

Consider using the primary brand color:

-  --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(49, 187, 107, 0.25);
🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] Code style issues found. Run Prettier with --write to fix formatting issues.


Line range hint 1-14110: Fix code formatting issues.

The pipeline indicates formatting inconsistencies that can be fixed using Prettier.

Run Prettier to automatically fix the formatting:

prettier --write src/assets/css/app.css
🧰 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

📥 Commits

Reviewing files that changed from the base of the PR and between 364bdfc and 976e9f5.

📒 Files selected for processing (1)
  • src/assets/css/app.css (7 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/assets/css/app.css

[error] 81-81: Unexpected value or character.

Expected one of:

(parse)

🪛 GitHub Actions: PR Workflow
src/assets/css/app.css

[warning] Code style issues found. Run Prettier with --write to fix formatting issues.

🔇 Additional comments (1)
src/assets/css/app.css (1)

14079-14110: 🛠️ Refactor suggestion

Improve success button initial state visibility.

The success button uses a gray text color (#707070) which reduces its visual prominence and may affect user experience.

Consider using white text color for better contrast and visual hierarchy:

.btn-success {
-  color: #707070;
+  color: #fff;
}

.btn-success:hover,
.btn-success:active {
  background-color: #2aa65d !important;
}

.btn-success:active svg path {
  fill: #ffffff !important;
  transition: fill 0.32s !important;
}

.btn-success .text-secondary {
  transition: color 0.32s !important;
}

Likely invalid or redundant comment.

🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] Code style issues found. Run Prettier with --write to fix formatting issues.

coderabbitai[bot]
coderabbitai bot previously approved these changes Jan 23, 2025
@palisadoes
Copy link
Contributor

I have to close this again. There are too many files submitted that are not related to src/screens/OrganizationPeople/OrganizationPeople.tsx

Please do the following

  1. Save your work in abhassen44:ref/css3
  2. Checkout develop-postgres
  3. Do a hard reset git reset upstream/develop-postgres --hard
  4. Create a new local branch that is not abhassen44:ref/css3
  5. Update only src/screens/OrganizationPeople/OrganizationPeople.tsx and the CSS
  6. Submit a new PR

This PR #3401 will overwrite the work of many others who have contributed time and effort to a better UI/UX

Closing

@palisadoes palisadoes closed this Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants