-
Notifications
You must be signed in to change notification settings - Fork 649
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
Redesigned the dashboard for the user organisation page #11066
base: develop
Are you sure you want to change the base?
Redesigned the dashboard for the user organisation page #11066
Conversation
WalkthroughThis pull request updates three files. In the localization file, several new keys are added for dashboard tabs and personalized doctor greetings while some old keys are removed or modified. The User Dashboard component now features a tabbed interface that sorts organizations into associations and governance. It also incorporates state management for active tabs, conditional rendering of content, and updated interaction handlers for sign-out and profile redirection. A minor cosmetic change is made in the Cypress configuration. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant D as Dashboard Component
participant L as Logic Handler
U->>D: Clicks a specific tab
D->>D: Update activeTab state via useState
D->>L: Filter organizations (Associations vs Governance)
L-->>D: Returns filtered data
D-->>U: Renders selected tab content
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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: 1
🧹 Nitpick comments (3)
src/pages/UserDashboard.tsx (3)
126-143
: Consider enhancing accessibility for the tab navigation.While the tab implementation looks good visually, it lacks proper accessibility attributes.
Consider adding ARIA attributes for better accessibility:
<div className="flex border-b border-gray-200" role="tablist"> {tabs.map((tab) => ( <button key={tab} onClick={() => setActiveTab(tab)} + role="tab" + aria-selected={activeTab === tab} + aria-controls={`${tab.toLowerCase().replace(/\s+/g, '-')}-panel`} className={`px-4 py-2 text-md font-medium transition-all duration-75 ${ activeTab === tab ? "border-b-2 border-green-600 text-green-700" : "text-gray-500" }`} > {tab} </button> ))} </div>Then add corresponding attributes to the tab panels:
<div + id="my-facilities-panel" + role="tabpanel" + aria-labelledby="My Facilities" > {/* Content */} </div>
185-224
: Consider adding empty state message for no associations.While you check for
associations.length > 0
, there's no UI feedback when the list is empty.Add a message when no associations are available:
{activeTab === "Associations" && ( <section className="space-y-3 md:space-y-4"> <p className="text-sm text-gray-800 font-normal px-1"> {t("dashboard_tab_associations")} </p> + {associations.length === 0 ? ( + <div className="text-center py-6 text-gray-500"> + <p>{t("no_associations_found")}</p> + </div> + ) : ( <div className="grid gap-3 md:gap-4 grid-cols-1 sm:grid-cols-2 lg:grid-cols-3" data-cy="organization-list" > {associations.map((association) => ( // ... Existing code ... ))} </div> + )} </section> )}You'll need to add the
no_associations_found
key to your translations.
226-265
: Consider adding empty state message for no administrations.Similar to the associations section, there's no UI feedback when the administrations list is empty.
Apply the same pattern suggested for the associations section to provide feedback when no administrations are found.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json
(2 hunks)src/pages/UserDashboard.tsx
(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Redirect rules - care-ohc
- GitHub Check: Header rules - care-ohc
- GitHub Check: Pages changed - care-ohc
🔇 Additional comments (7)
src/pages/UserDashboard.tsx (5)
29-32
: Well-organized organization filtering logic!The clear separation of organizations by type improves code readability and the user experience by providing targeted content in each tab.
34-35
: Good state management for tabs implementation.Using useState for tracking the active tab and defining a tabs array provides a maintainable approach for the tabbed interface.
40-68
: Excellent responsive welcome section design.The redesigned welcome section with adaptive layout for different screen sizes (using flex-col/flex-row) and personalized greeting for doctors significantly improves the user experience.
119-123
: Great conditional doctor banner.The special banner for doctors adds helpful context for doctor accounts, improving the user experience through clear role identification.
146-183
: Well-implemented facilities tab content.The facilities section is well organized with proper responsive grid layout and consistent card designs. Good job using translation keys for the descriptions.
public/locale/en.json (2)
739-741
: Clear and descriptive dashboard tab explanations.These descriptive texts provide excellent context to help users understand the different organizational categories.
2581-2584
: Effective personalized doctor greetings.The addition of personalized greetings for doctors and a clear indication of doctor status enhance the user experience and align well with the UI changes in the dashboard.
Lets rename |
any other changes needed? @bodhish |
Added UI feedback when lists are empty, added accessibilty attributes and fixed some typos
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: 2
🧹 Nitpick comments (5)
src/pages/UserDashboard.tsx (5)
29-32
: Simplify the filter expressions by removing unnecessary fallback arrays.The
|| []
fallbacks after the filter operations are redundant sincefilter()
will always return an array (even if empty).- const associations = - organizations.filter((org) => org.org_type === "role") || []; - const governance = - organizations.filter((org) => org.org_type === "govt") || []; + const associations = organizations.filter((org) => org.org_type === "role"); + const governance = organizations.filter((org) => org.org_type === "govt");
151-301
: Refactor repeated tab content structure to reduce code duplication.The three tab sections (Facilities, Associations, Governance) follow identical patterns with only minor differences in data and labels. This creates significant code duplication that could be extracted into a reusable component or function.
You could refactor this to a more DRY approach like:
// Create a reusable function or component const renderTabContent = ( tabId, items, emptyMessage, description, renderLink ) => { return ( <section className="space-y-3 md:space-y-4" id={`my-${tabId.toLowerCase()}-panel`} role="tabpanel" aria-labelledby={tabId} > <p className="text-sm text-gray-800 font-normal px-1"> {description} </p> {items.length === 0 ? ( <div className="text-center py-6 text-gray-500"> <p>{emptyMessage}</p> </div> ) : ( <div className="grid gap-3 md:gap-4 grid-cols-1 sm:grid-cols-2 lg:grid-cols-3" data-cy={tabId === "My Facilities" ? "facility-list" : "organization-list"} > {items.map((item) => renderLink(item))} </div> )} </section> ); }; // Then in your component render: {activeTab === "My Facilities" && renderTabContent( "My Facilities", facilities, t("no_facilities_found"), t("dashboard_tab_facilities"), (facility) => ( <Link key={facility.id} href={`/facility/${facility.id}/overview`} > {/* Card content */} </Link> ) )} // Repeat similar pattern for other tabsThis would significantly reduce the code duplication while maintaining the same functionality.
271-271
: Rename variable to match tab terminology.In the Governance tab, you're using
administration
as the variable name, but the tab is called "Governance". This inconsistency could be confusing for future developers.- {governance.map((administration) => ( + {governance.map((governanceOrg) => (This aligns with the PR comment from user bodhish suggesting to rename "administration" to "governance".
34-35
: Consider using an enum or constants for tab names.Using hardcoded strings for tab identifiers can lead to errors if the same string is not used consistently throughout the code.
+ // Define tab constants to avoid typos and ensure consistency + const TAB_FACILITIES = "My Facilities"; + const TAB_ASSOCIATIONS = "Associations"; + const TAB_GOVERNANCE = "Governance"; + - const [activeTab, setActiveTab] = useState("My Facilities"); - const tabs = ["My Facilities", "Associations", "Governance"]; + const [activeTab, setActiveTab] = useState(TAB_FACILITIES); + const tabs = [TAB_FACILITIES, TAB_ASSOCIATIONS, TAB_GOVERNANCE]; // Then use these constants throughout the code: - {activeTab === "My Facilities" && ( + {activeTab === TAB_FACILITIES && (
129-146
: Enhance accessibility with explicit aria labels.The tabs implementation could benefit from additional accessibility attributes to make it fully ARIA compliant.
Consider adding:
- An
aria-label
to the tabs container- Explicit
id
attributes to each tab that match theiraria-controls
- <div className="flex border-b border-gray-200" role="tablist"> + <div + className="flex border-b border-gray-200" + role="tablist" + aria-label="Dashboard Sections" + > {tabs.map((tab) => ( <button key={tab} onClick={() => setActiveTab(tab)} role="tab" + id={`tab-${tab.toLowerCase().replace(/\s+/g, "-")}`} aria-selected={activeTab === tab} aria-controls={`${tab.toLowerCase().replace(/\s+/g, "-")}-panel`}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json
(4 hunks)src/pages/UserDashboard.tsx
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- public/locale/en.json
@Spandan-Mishra can you look into code rabbit comments |
@nihal467 should I make the changes mentioned by coderabbit and make another commit? |
If the suggestion makes sense yes |
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.
As per the Figma design, the Sign out
button is expected to move inside the kebab menu
.
cc: @vinutv
@nihal467 @Jacobjeevan @yash-learner |
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: 3
♻️ Duplicate comments (2)
src/pages/UserDashboard.tsx (2)
40-45
: 🛠️ Refactor suggestionApply localization to tab names.
The tab names are hardcoded in English. To maintain consistency with the rest of the application, these should use the translation function.
- const tabs = [ - DashboardTabs.TAB_FACILITIES, - DashboardTabs.TAB_ASSOCIATIONS, - DashboardTabs.TAB_GOVERNANCE, - ]; + const tabs = [ + { id: DashboardTabs.TAB_FACILITIES, label: t("dashboard_tab_facilities_title") }, + { id: DashboardTabs.TAB_ASSOCIATIONS, label: t("dashboard_tab_associations_title") }, + { id: DashboardTabs.TAB_GOVERNANCE, label: t("dashboard_tab_governance_title") } + ];Also update the tab rendering in the mapping function to use the new structure.
147-163
: 🛠️ Refactor suggestionUpdate tab rendering to use the localized tabs object.
When implementing the localized tabs suggested earlier, you'll need to update this mapping to use the new structure:
- {tabs.map((tab) => ( + {tabs.map((tab) => ( <button - key={tab} - onClick={() => setActiveTab(tab)} + key={tab.id} + onClick={() => setActiveTab(tab.id)} role="tab" - id={`${tab.toLowerCase().replace(/\s+/g, "-")}-tab`} - aria-selected={activeTab === tab} - aria-controls={`${tab.toLowerCase().replace(/\s+/g, "-")}-panel`} + id={`${tab.id.toLowerCase().replace(/\s+/g, "-")}-tab`} + aria-selected={activeTab === tab.id} + aria-controls={`${tab.id.toLowerCase().replace(/\s+/g, "-")}-panel`} className={`px-4 py-2 text-md font-medium transition-all duration-75 ${ - activeTab === tab + activeTab === tab.id ? "border-b-2 border-green-600 text-green-700" : "text-gray-500" }`} > - {tab} + {tab.label} </button> ))}
🧹 Nitpick comments (1)
src/pages/UserDashboard.tsx (1)
284-284
: Duplicate data-cy attributes across tabs.Both association and governance tabs use the same data-cy attribute "organization-list". This might cause confusion in tests that target these elements. Consider using distinct attributes like "association-list" and "governance-list".
- data-cy="organization-list" + data-cy="association-list" // And for governance: - data-cy="organization-list" + data-cy="governance-list"Also applies to: 234-234
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cypress/tsconfig.json
(1 hunks)src/pages/UserDashboard.tsx
(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- cypress/tsconfig.json
🔇 Additional comments (7)
src/pages/UserDashboard.tsx (7)
24-28
: Good addition of the DashboardTabs enum.Using an enum for tab identifiers is a clean approach that provides better type safety and makes future tab modifications easier to manage.
37-39
: Well-structured organization filtering.The separation of organizations into associations and governance based on their type is clear and maintainable.
132-136
: Good user type-based conditional rendering.The special banner for doctors is a nice personalized touch. The use of the translation key ensures consistency with the application's localization strategy.
110-129
: Great implementation of dropdown menu for the sign-out action.The dropdown menu pattern is a good UI improvement over the previous direct sign-out button. Using CareIcon as suggested in the previous review is also a good practice for consistency.
64-66
: Good user personalization.Nice addition of conditional rendering to show a different greeting for doctors. This enhances the personalized experience.
1-6
: Good update to imports.Adding CareIcon import is appropriate for using the icon library component as suggested in previous comments.
10-15
: Good addition of dropdown menu components.The dropdown menu imports are appropriate for the new menu implementation.
@Spandan-Mishra , do "Mark as Resolved" with comment or acknowledgement after changes are done or unnecessary ones by coderabbitai |
Resolved all useful as well as unnecessary changes @AdityaJ2305 |
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.
Adding for context. here is the discussion on slack regarding UI/UX of |
Made the design responsive for mobile and upda ted card design for mobiles. Introduced TabContent Component to reduce code redundancy
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
♻️ Duplicate comments (1)
src/pages/UserDashboard.tsx (1)
55-60
: 🛠️ Refactor suggestionApply localization to tab display names.
The tab names in the tabs array and in the JSX are hardcoded in English. This contradicts the internationalization approach used elsewhere in the application.
- const [activeTab, setActiveTab] = useState(DashboardTabs.TAB_FACILITIES); - const tabs = [ - DashboardTabs.TAB_FACILITIES, - DashboardTabs.TAB_ASSOCIATIONS, - DashboardTabs.TAB_GOVERNANCE, - ]; + const [activeTab, setActiveTab] = useState<DashboardTabs>(DashboardTabs.TAB_FACILITIES); + const tabsConfig = [ + { id: DashboardTabs.TAB_FACILITIES, label: t("dashboard_tab_facilities_title") }, + { id: DashboardTabs.TAB_ASSOCIATIONS, label: t("dashboard_tab_associations_title") }, + { id: DashboardTabs.TAB_GOVERNANCE, label: t("dashboard_tab_governance_title") } + ];Then update the tab rendering at lines 182-198 to use the translated labels:
- {tabs.map((tab) => ( + {tabsConfig.map(({ id, label }) => ( <button - key={tab} - onClick={() => setActiveTab(tab)} + key={id} + onClick={() => setActiveTab(id)} role="tab" - id={`${tab.toLowerCase().replace(/\s+/g, "-")}-tab`} - aria-selected={activeTab === tab} - aria-controls={`${tab.toLowerCase().replace(/\s+/g, "-")}-panel`} + id={`${id.toLowerCase().replace(/\s+/g, "-")}-tab`} + aria-selected={activeTab === id} + aria-controls={`${id.toLowerCase().replace(/\s+/g, "-")}-panel`} className={`px-4 py-2 text-md font-medium transition-all duration-75 ${ - activeTab === tab + activeTab === id ? "border-b-2 border-green-600 text-green-700" : "text-gray-500" }`} > - {tab} + {label} </button> ))}
🧹 Nitpick comments (2)
src/pages/UserDashboard.tsx (2)
332-332
: Consider adding responsive spacing.The grid layout for the items list could benefit from more responsive spacing between items on larger screens.
- className="grid gap-3 md:gap-4 grid-cols-1 sm:grid-cols-2 lg:grid-cols-3" + className="grid gap-3 md:gap-4 lg:gap-5 grid-cols-1 sm:grid-cols-2 lg:grid-cols-3"
231-231
: Add hover state feedback to improve UX.Consider adding a slight transformation effect on hover to provide better visual feedback to users when hovering over the cards.
In each Card component across the three tab sections:
- <Card className="transition-all hover:shadow-md hover:border-primary/20"> + <Card className="transition-all hover:shadow-md hover:border-primary/20 hover:translate-y-[-2px]">Also applies to: 266-266, 301-301
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/UserDashboard.tsx
(2 hunks)
🔇 Additional comments (6)
src/pages/UserDashboard.tsx (6)
165-172
: Good implementation of the doctor-specific banner.The conditional rendering for the doctor banner is well-implemented. It provides a personalized experience for users with the doctor role.
187-189
: Fix aria attributes for improved accessibility.For better accessibility, ensure that the aria-controls attribute points to the correct panel ID and add a meaningful aria-label to each tab.
- aria-controls={`${tab.toLowerCase().replace(/\s+/g, "-")}-panel`} + aria-controls={`${tab.toLowerCase()}-panel`} + aria-label={t(`dashboard_tab_${tab.toLowerCase()}_title`)}
310-339
: Good extraction of the TabContent component.The TabContent component is a good abstraction that reduces code duplication. It effectively handles the shared logic for each tab's content.
322-322
: Fix the aria-labelledby attribute reference.The aria-labelledby attribute should reference the ID of the corresponding tab button rather than the tabId itself.
- aria-labelledby={tabId} + aria-labelledby={`${tabId.replace('-panel', '')}-tab`}
125-164
: Well-implemented dropdown menu for actions.The dropdown menu implementation is clean and follows best practices. It provides a good mobile experience by showing the edit profile link only on small screens through the dropdown menu.
65-93
: Improved user welcome section with personalization.The welcome section has been significantly improved with personalized greetings for doctors and a more organized layout. The conditional rendering using the user's type is a good approach.
made the card mobile responsive and reduced code redundancy |
@Jacobjeevan made all the changes which were requested. Screen.Recording.2025-03-08.231620.mp4 |
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: 1
🧹 Nitpick comments (2)
src/pages/UserDashboard.tsx (2)
24-28
: Consider using i18n keys in enum values for consistency.The DashboardTabs enum uses hardcoded English strings which will need to be translated when displayed. Consider using translation key identifiers instead.
enum DashboardTabs { - TAB_FACILITIES = "Facilities", - TAB_ASSOCIATIONS = "Associations", - TAB_GOVERNANCE = "Governance", + TAB_FACILITIES = "dashboard_tab_facilities_title", + TAB_ASSOCIATIONS = "dashboard_tab_associations_title", + TAB_GOVERNANCE = "dashboard_tab_governance_title", }
171-173
: Improve accessibility with more descriptive IDs.The current ID generation might lead to issues with special characters. Consider using a more robust approach for generating IDs from the tab names.
- id={`${tab.toLowerCase().replace(/\s+/g, "-")}-tab`} - aria-selected={activeTab === tab} - aria-controls={`${tab.toLowerCase().replace(/\s+/g, "-")}-panel`} + id={`dashboard-tab-${tab.toLowerCase()}`} + aria-selected={activeTab === tab} + aria-controls={`dashboard-panel-${tab.toLowerCase()}`}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/UserDashboard.tsx
(2 hunks)
🔇 Additional comments (6)
src/pages/UserDashboard.tsx (6)
54-58
: Apply localization to tab names.The tab names are hardcoded in English. To maintain consistency with the rest of the application, these should use the translation function.
const tabs = [ DashboardTabs.TAB_FACILITIES, DashboardTabs.TAB_ASSOCIATIONS, DashboardTabs.TAB_GOVERNANCE, ]; // And later in the JSX: - {tab} + {t(tab)}Also applies to: 180-180
93-108
: Consider removing redundant admin dashboard button.Based on the PR comments, it was suggested to remove the admin dashboard button outside the dropdown menu. This button is still present in the desktop view while hidden in mobile view.
- {user.is_superuser && ( - <Button - variant="outline" - size="sm" - className="w-full sm:w-auto hidden sm:flex" - asChild - > - <Link - href="/admin/questionnaire" - className="gap-2 text-inherit flex items-center" - > - <User2Icon className="h-4 w-4" /> - {t("admin_dashboard")} - </Link> - </Button> - )}
306-306
: Fix the aria-labelledby attribute.The aria-labelledby attribute should reference an element ID, not the panel ID. Update it to match the corresponding tab ID.
- aria-labelledby={tabId} + aria-labelledby={`${tabId.replace('panel', 'tab')}`}
120-130
: Consider removing the admin dashboard link from the dropdown.Based on previous review comments, it was suggested to remove the admin dashboard from the dropdown as it's redundant. While you mentioned this is only visible in mobile view, maintaining two separate links for the same function can cause confusion and maintenance issues.
Would you like to keep both instances of the admin dashboard link, or consolidate to just one implementation?
151-155
: Great addition for personalized experience!The doctor-specific banner provides a personalized experience which enhances user engagement. The styling with indigo colors makes it stand out appropriately.
294-323
: Well-implemented reusable component!Great job extracting the tab content into a reusable component. This reduces code duplication and makes the component more maintainable.
Proposed Changes
Redesigned the user's organization page which was assigned to me in issue #11016.
Video which shows the changes made:
Screen.Recording.2025-03-05.220932.mp4
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit