-
-
Notifications
You must be signed in to change notification settings - Fork 893
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
Fix: Organization Dashboard Page Layout Fixes #3794
base: develop-postgres
Are you sure you want to change the base?
Fix: Organization Dashboard Page Layout Fixes #3794
Conversation
WalkthroughThis pull request updates documentation references for the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Button
participant Toast
User->>Button: Click event on button (e.g., view all events)
Button->>Toast: Invoke toast.success("Coming soon!")
Toast-->>User: Display "Coming soon!" notification
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (5)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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
|
There was a problem hiding this 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 (4)
src/style/app-fixed.module.css (3)
3437-3441
: .cardItem Layout Adjustments Review
The changes correctly update the layout by setting:
align-items: center;
(note: with row direction this vertically centers items; please update the inline comment if “left alignment” was not the intent)- Reduced padding to
0.75rem
and gap to1.5rem
, which aligns with the new design requirements.Suggestion: Update or remove the misleading inline comment ("Ensures left alignment") to avoid confusion since vertical centering is being applied.
3449-3454
: .CardItemImage Modifications Review
The changes lower the image height from a larger value to3.5rem
and replace the fixed width withmin-width: 3.5rem
, improving responsiveness.
However, the declarationborder: 3.25rem;
is unusual since a proper border needs a style and color. Please confirm if this value is intentional or if it requires a full border shorthand (e.g.border: 3.25rem solid currentColor;
).
4144-4165
: New.dropdownToggle
Class Implementation Review
A new class.dropdownToggle
has been added along with hover and pseudo-element styles. This block establishes:
- A flex container with zero bottom margin;
- A background image sourced from
/public/images/svg/angleDown.svg
with proper repeat and positioning;- A forced background color via
!important
;- Adjusted border properties (no left border and rounded top-right and bottom-right corners), and a defined text color.
On hover, the element applies a border while retaining no left border, and the pseudo-element adjustments remove top and bottom borders.Points to consider:
- Asset Path: Verify that the absolute path
/public/images/svg/angleDown.svg
is correct in the deployed environment. If assets are handled via a bundler, a relative path might be preferable.- Use of
!important
: Ensure that using!important
for background-color and hover border styles is necessary; overuse can reduce maintainability.src/screens/OrganizationDashboard/OrganizationDashboard.tsx (1)
488-490
: Fix typo in the test ID for the leaderboard button.There's a typo in the test ID for the leaderboard button:
viewAllLeadeboard
should beviewAllLeaderboard
. This might cause issues with test selection or future maintenance.- data-testid="viewAllLeadeboard" + data-testid="viewAllLeaderboard"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/docs/auto-docs/components/LeftDrawerOrg/LeftDrawerOrg/functions/default.md
(1 hunks)docs/docs/auto-docs/components/LeftDrawerOrg/LeftDrawerOrg/interfaces/InterfaceLeftDrawerProps.md
(1 hunks)docs/docs/auto-docs/components/OrganizationScreen/OrganizationScreen/functions/default.md
(1 hunks)src/components/ProfileDropdown/ProfileDropdown.tsx
(1 hunks)src/screens/OrganizationDashboard/OrganizationDashboard.tsx
(3 hunks)src/style/app-fixed.module.css
(4 hunks)
✅ Files skipped from review due to trivial changes (4)
- src/components/ProfileDropdown/ProfileDropdown.tsx
- docs/docs/auto-docs/components/OrganizationScreen/OrganizationScreen/functions/default.md
- docs/docs/auto-docs/components/LeftDrawerOrg/LeftDrawerOrg/functions/default.md
- docs/docs/auto-docs/components/LeftDrawerOrg/LeftDrawerOrg/interfaces/InterfaceLeftDrawerProps.md
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Application
🔇 Additional comments (5)
src/style/app-fixed.module.css (1)
3456-3459
: .cardItemtitle Font Size Update
The font size has been reduced to15px
, which matches the design specification outlined in the PR objectives.src/screens/OrganizationDashboard/OrganizationDashboard.tsx (4)
340-347
: Added user feedback for "View All Events" button - good addition.Adding the toast notification provides immediate feedback to users when clicking the button that doesn't navigate anywhere yet. This matches the PR objective of ensuring "View All" buttons display toast notifications.
388-390
: Appropriate replacement of commented navigation with toast notification.You've replaced a commented-out navigation action with a toast notification that provides immediate user feedback. This is consistent with the handling of other "View All" buttons.
438-446
: Ensure consistent implementation across all "View All" buttons.I notice this button already had a toast notification implementation before the changes. Good to see that your new implementations follow the same pattern, maintaining consistency throughout the UI.
27-27
: Good import addition for the toast functionality.The import of
toast
from 'react-toastify' is correctly placed with the other imports, following the project's import organization pattern.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #3794 +/- ##
====================================================
+ Coverage 86.57% 86.58% +0.01%
====================================================
Files 373 373
Lines 9226 9229 +3
Branches 1949 1949
====================================================
+ Hits 7987 7991 +4
Misses 867 867
+ Partials 372 371 -1 ☔ View full report in Codecov by Sentry. |
I am writing the test for the newly added onClick function. |
There was a problem hiding this 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 (2)
src/screens/OrganizationDashboard/OrganizationDashboard.spec.tsx (2)
118-120
: Typo in test ID for leaderboard elementThere's a typo in the test ID: 'viewAllLeadeboard' should be 'viewAllLeaderboard'.
- const viewLeaderBtn = screen.getByTestId('viewAllLeadeboard'); + const viewLeaderBtn = screen.getByTestId('viewAllLeaderboard');
105-129
: Consider separating button tests for better maintainabilityCurrently, all button click tests are combined in a single test case. Consider splitting these into separate test cases for better readability and maintainability. This would make it easier to identify which specific functionality is failing if a test fails.
it('shows success toast when clicking on membership requests view button', async () => { renderWithProviders({ mocks: MOCKS }); await waitFor(() => { expect( screen.getByTestId('viewAllMembershipRequests'), ).toBeInTheDocument(); }); const viewRequestsBtn = screen.getByTestId('viewAllMembershipRequests'); fireEvent.click(viewRequestsBtn); expect(toast.success).toHaveBeenCalledWith('Coming soon!'); + }); + + it('shows success toast when clicking on leaderboard view button', async () => { + renderWithProviders({ mocks: MOCKS }); + + await waitFor(() => { + expect(screen.getByTestId('viewAllLeaderboard')).toBeInTheDocument(); + }); - const viewLeaderBtn = screen.getByTestId('viewAllLeadeboard'); + const viewLeaderBtn = screen.getByTestId('viewAllLeaderboard'); fireEvent.click(viewLeaderBtn); expect(toast.success).toHaveBeenCalledWith('Coming soon!'); + }); + + it('navigates to events page when clicking view all events button', async () => { + renderWithProviders({ mocks: MOCKS }); + + await waitFor(() => { + expect(screen.getByTestId('viewAllEvents')).toBeInTheDocument(); + }); const viewEventsBtn = screen.getByTestId('viewAllEvents'); fireEvent.click(viewEventsBtn); expect(mockedNavigate).toHaveBeenCalledWith('/orgevents/orgId'); + }); + + it('navigates to posts page when clicking view all posts button', async () => { + renderWithProviders({ mocks: MOCKS }); + + await waitFor(() => { + expect(screen.getByTestId('viewAllPosts')).toBeInTheDocument(); + }); const viewPostBtn = screen.getByTestId('viewAllPosts'); fireEvent.click(viewPostBtn); expect(mockedNavigate).toHaveBeenCalledWith('/orgpost/orgId'); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/screens/OrganizationDashboard/OrganizationDashboard.spec.tsx
(1 hunks)src/screens/OrganizationDashboard/OrganizationDashboard.tsx
(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/screens/OrganizationDashboard/OrganizationDashboard.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Application
🔇 Additional comments (5)
src/screens/OrganizationDashboard/OrganizationDashboard.spec.tsx (5)
116-116
: Testing toast notification for membership requests view button - LGTM!This line correctly verifies that clicking the membership requests view button shows a success toast with the message "Coming soon!".
122-125
: Testing navigation for events view button - LGTM!This test correctly verifies the navigation to the events page when clicking the "View All Events" button.
126-129
: Testing navigation for posts view button - LGTM!This test correctly verifies the navigation to the posts page when clicking the "View All Posts" button.
122-125
:✅ Verification successful
Verify consistency with the companion component file
Make sure that the navigation to '/orgevents/orgId' in the test matches the actual implementation in the OrganizationDashboard.tsx component.
🏁 Script executed:
#!/bin/bash # Description: Look for the implementation of the viewAllEvents button in the OrganizationDashboard component # to verify that it navigates to the correct path # Find the event handler for viewAllEvents button rg -A 3 -B 3 "data-testid=\"viewAllEvents\"" src/screens/OrganizationDashboard/OrganizationDashboard.tsx # Also look for the onClick handler that might be navigating to orgevents path rg -A 3 -B 3 "/orgevents" src/screens/OrganizationDashboard/OrganizationDashboard.tsxLength of output: 723
Navigation Path Verified
The test expectation for navigating to
/orgevents/orgId
is consistent with the implementation in theOrganizationDashboard.tsx
component. The component constructs the navigation path usingeventsLink =
/orgevents/${orgId}, which, when
orgIdis set to
"orgId", produces the expected
/orgevents/orgId`.No changes are required.
126-128
:✅ Verification successful
Verify consistency with the companion component file
Make sure that the navigation to '/orgpost/orgId' in the test matches the actual implementation in the OrganizationDashboard.tsx component.
🏁 Script executed:
#!/bin/bash # Description: Look for the implementation of the viewAllPosts button in the OrganizationDashboard component # to verify that it navigates to the correct path # Find the event handler for viewAllPosts button rg -A 3 -B 3 "data-testid=\"viewAllPosts\"" src/screens/OrganizationDashboard/OrganizationDashboard.tsx # Also look for the onClick handler that might be navigating to orgpost path rg -A 3 -B 3 "/orgpost" src/screens/OrganizationDashboard/OrganizationDashboard.tsxLength of output: 716
Consistency Check: Navigation Route Verified
The implementation in
OrganizationDashboard.tsx
constructs the navigation path as:const postsLink = `/orgpost/${orgId}`;and the button's click handler correctly calls:
onClick={(): void => navigate(postsLink)}The test in
OrganizationDashboard.spec.tsx
verifies that clicking the button triggers navigation to'/orgpost/orgId'
. This is consistent—as long as the testing context assignsorgId
the literal"orgId"
value, both the component and the test are aligned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Please add translations for the messages
public/locales
- Add a video showing that the messages change when the language changes
- Add the appropriate tests
@palisadoes Translations added for the notification. Also test is added to check notification and routing. Screen.Recording.mp4 |
Patch coverage is 100%, to make the file test coverage to 100% another issue is raised. |
@palisadoes Translations added for the word "Tags" ![]() |
What kind of change does this PR introduce?
This PR will fix the Organization Dashboard Page Layout, fixes import of
src/style/app-fixed.module.css
.Also,
View All
buttons are now showing toast notification.Issue Number:
Fixes #3777
Snapshots/Videos:

If relevant, did you update the documentation?
N/A
Summary
Does this PR introduce a breaking change?
No
Checklist
CodeRabbit AI Review
Test Coverage
Other information
N/A
Have you read the contributing guide?
Yes
Summary by CodeRabbit