Issues/#10955/inconsistency in nurse mobile view #10956
Conversation
…to display all statuses.
…to display all statuses.
WalkthroughThis pull request modifies the encounter status filtering logic and adjusts the layout styling. In the EncounterList component, the onClick event for encounter status tabs now toggles the filter: if the clicked status is already active, it clears the selection; otherwise, it sets the status to the chosen value. Additionally, the OrganizationLayout component has updated class names for its Page and Menubar elements to adjust the styling and overflow behavior without altering the underlying functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant E as EncounterList
U->>E: Click status tab (e.g. "planned")
E->>E: Check if current status equals clicked status
alt Status Matched
E->>E: Set status = undefined
else Status Not Matched
E->>E: Set status = clicked value
end
E->>E: Update query parameters
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
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.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/pages/Organization/components/OrganizationLayout.tsx (1)
109-109: Add a comment explaining the purpose of the fixed width.Setting a width of 95vw (95% of viewport width) is a good approach for mobile responsiveness, but adding a comment would help explain why this specific value was chosen.
- <Page title={`${org.name}`} className="w-[95vw]"> + <Page title={`${org.name}`} className="w-[95vw]" {/* Setting width to 95% of viewport to fix mobile view inconsistency */}>src/pages/Encounters/EncounterList.tsx (2)
522-534: Refactor opportunity to reduce code duplication in status filters.The same toggle pattern is repeated for each status. Consider extracting this logic into a helper function to improve maintainability.
+ // Add this helper function above the component or as a local function + const toggleStatus = (currentStatus: string | undefined, newStatus: string, + encounterClass: string | undefined, priority: string | undefined) => { + return { + ...{ encounter_class: encounterClass, priority }, + status: currentStatus === newStatus ? undefined : newStatus, + }; + }; - onClick={() => { - if (status === "in_progress") { - updateQuery({ - ...{ encounter_class: encounterClass, priority }, - status: undefined, - }); - } else { - updateQuery({ - ...{ encounter_class: encounterClass, priority }, - status: "in_progress", - }); - } - }} + onClick={() => updateQuery(toggleStatus(status, "in_progress", encounterClass, priority))}
562-574: Add analytics tracking for status filter usage.Consider adding analytics tracking to understand which status filters are most commonly used, which could inform future UI improvements.
This would require implementing an analytics tracking function and calling it within the click handler:
onClick={() => { if (status === "completed") { updateQuery({ ...{ encounter_class: encounterClass, priority }, status: undefined, }); + // trackFilterUsage("status", "completed", "unselect"); } else { updateQuery({ ...{ encounter_class: encounterClass, priority }, status: "completed", }); + // trackFilterUsage("status", "completed", "select"); } }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/pages/Encounters/EncounterList.tsx(1 hunks)src/pages/Organization/OrganizationView.tsx(1 hunks)src/pages/Organization/components/OrganizationLayout.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/pages/Organization/OrganizationView.tsx
🔇 Additional comments (4)
src/pages/Organization/components/OrganizationLayout.tsx (1)
112-112:✅ Verification successful
Consider performance implications of enabling both vertical and horizontal scrolling.
Changing from
overflow-x-autotooverflow-autoenables both horizontal and vertical scrolling. This is appropriate for mobile, but ensure it doesn't cause unexpected layout shifts.
🏁 Script executed:
#!/bin/bash # Check for other usages of overflow styles in the Menubar component rg -A 2 -B 2 "Menubar.*className.*overflow"Length of output: 547
Final Resolved Comment: Performance Implications on Scrolling
The usage of
overflow-autoon the<Menubar>is consistent with similar patterns in this component and appears to be an intentional choice to support mobile responsiveness. That said, please verify that enabling both horizontal and vertical scrolling does not inadvertently lead to layout shifts or performance issues—especially on larger screens or during device rotations.src/pages/Encounters/EncounterList.tsx (3)
502-514: Great implementation of toggle functionality for status filtering.This change improves the user experience by allowing users to toggle off a status filter, rather than only being able to switch between different statuses. This is particularly helpful on mobile devices.
542-554: Maintain the same code pattern as other status filters.This code follows the same pattern as the other status filters, which is good for consistency.
582-594:❓ Verification inconclusive
Ensure the query update is optimized for performance.
The status toggle implementation works well, but frequent updates might impact performance. Consider debouncing these updates if multiple filters can be changed in quick succession.
🏁 Script executed:
#!/bin/bash # Check how updateQuery is implemented to ensure it handles frequent updates well rg -A 10 "function useFilters" src/hooks/useFilters.ts rg -A 5 "updateQuery" src/hooks/useFilters.tsLength of output: 230
Action: Verify the
updateQueryImplementation for PerformanceThe status toggle logic is implemented correctly. However, to ensure overall performance, please verify that the underlying
updateQueryfunction efficiently handles rapid updates—especially in scenarios where multiple filters may change quickly. It might be beneficial to introduce debouncing if such rapid-fire updates are a risk. Since the expected implementation insrc/hooks/useFilters.tscould not be located, kindly confirm the actual implementation ofupdateQueryand consider performance optimizations if necessary.
| {/* Navigation */} | ||
| <div className="mt-4"> | ||
| <Menubar className="w-full h-full overflow-x-auto"> | ||
| <div className="mt-4 overflow-auto scrollbar-height: [1px]"> |
There was a problem hiding this comment.
Fix the CSS syntax error in scrollbar styling.
The scrollbar-height: [1px] appears to be an invalid CSS syntax. Scrollbar styling should use the correct CSS property.
- <div className="mt-4 overflow-auto scrollbar-height: [1px]">
+ <div className="mt-4 overflow-auto scrollbar-thin">If you're using Tailwind CSS, consider using their scrollbar plugin or a custom CSS class to properly style scrollbars.
📝 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.
| <div className="mt-4 overflow-auto scrollbar-height: [1px]"> | |
| <div className="mt-4 overflow-auto scrollbar-thin"> |
| value="in_progress" | ||
| className="data-[state=active]:bg-primary/10 data-[state=active]:text-primary" | ||
| onClick={() => | ||
| updateQuery({ | ||
| ...{ encounter_class: encounterClass, priority }, | ||
| status: "in_progress", | ||
| }) | ||
| } | ||
| onClick={() => { | ||
| if (status === "in_progress") { | ||
| updateQuery({ | ||
| ...{ encounter_class: encounterClass, priority }, | ||
| status: undefined, | ||
| }); | ||
| } else { | ||
| updateQuery({ | ||
| ...{ encounter_class: encounterClass, priority }, | ||
| status: "in_progress", | ||
| }); | ||
| } | ||
| }} | ||
| > | ||
| <CareIcon icon="l-spinner" className="mr-2 h-4 w-4" /> | ||
| {t("encounter_class__in_progress")} | ||
| </TabsTrigger> | ||
| <TabsTrigger | ||
| value="discharged" | ||
| className="data-[state=active]:bg-primary/10 data-[state=active]:text-primary" | ||
| onClick={() => | ||
| updateQuery({ | ||
| ...{ encounter_class: encounterClass, priority }, | ||
| status: "discharged", | ||
| }) | ||
| } | ||
| onClick={() => { | ||
| if (status === "discharged") { | ||
| updateQuery({ | ||
| ...{ encounter_class: encounterClass, priority }, | ||
| status: undefined, | ||
| }); | ||
| } else { | ||
| updateQuery({ | ||
| ...{ encounter_class: encounterClass, priority }, | ||
| status: "discharged", | ||
| }); | ||
| } | ||
| }} | ||
| > | ||
| <CareIcon icon="l-home" className="mr-2 h-4 w-4" /> | ||
| {t("discharge")} | ||
| </TabsTrigger> | ||
| <TabsTrigger | ||
| value="completed" | ||
| className="data-[state=active]:bg-primary/10 data-[state=active]:text-primary" | ||
| onClick={() => | ||
| updateQuery({ | ||
| ...{ encounter_class: encounterClass, priority }, | ||
| status: "completed", | ||
| }) | ||
| } | ||
| onClick={() => { | ||
| if (status === "completed") { | ||
| updateQuery({ | ||
| ...{ encounter_class: encounterClass, priority }, | ||
| status: undefined, | ||
| }); | ||
| } else { | ||
| updateQuery({ | ||
| ...{ encounter_class: encounterClass, priority }, | ||
| status: "completed", | ||
| }); | ||
| } | ||
| }} | ||
| > | ||
| <CareIcon icon="l-check" className="mr-2 h-4 w-4" /> | ||
| {t("completed")} | ||
| </TabsTrigger> | ||
| <TabsTrigger | ||
| value="cancelled" | ||
| className="data-[state=active]:bg-primary/10 data-[state=active]:text-primary" | ||
| onClick={() => | ||
| updateQuery({ | ||
| ...{ encounter_class: encounterClass, priority }, | ||
| status: "cancelled", | ||
| }) | ||
| } | ||
| onClick={() => { | ||
| if (status === "cancelled") { | ||
| updateQuery({ | ||
| ...{ encounter_class: encounterClass, priority }, | ||
| status: undefined, | ||
| }); | ||
| } else { | ||
| updateQuery({ | ||
| ...{ encounter_class: encounterClass, priority }, | ||
| status: "cancelled", | ||
| }); | ||
| } | ||
| }} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address desktop vs. mobile filter consistency.
The toggle behavior is implemented for desktop tabs but the mobile dropdown in lines 372-415 still uses the old approach. Consider implementing the same toggle behavior for the mobile dropdown to maintain consistency.
Look at the mobile implementation around line 375:
onValueChange={(value) => {
updateQuery({
...{ encounter_class: encounterClass, priority },
- status: value === "all" ? undefined : value,
+ status: value === "all" ? undefined : (status === value ? undefined : value),
});
}}📝 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.
| value="planned" | |
| className="data-[state=active]:bg-primary/10 data-[state=active]:text-primary" | |
| onClick={() => | |
| updateQuery({ | |
| ...{ encounter_class: encounterClass, priority }, | |
| status: "planned", | |
| }) | |
| } | |
| onClick={() => { | |
| if (status === "planned") { | |
| updateQuery({ | |
| ...{ encounter_class: encounterClass, priority }, | |
| status: undefined, | |
| }); | |
| } else { | |
| updateQuery({ | |
| ...{ encounter_class: encounterClass, priority }, | |
| status: "planned", | |
| }); | |
| } | |
| }} | |
| > | |
| <CareIcon icon="l-calender" className="mr-2 h-4 w-4" /> | |
| {t("encounter_status__planned")} | |
| </TabsTrigger> | |
| <TabsTrigger | |
| value="in_progress" | |
| className="data-[state=active]:bg-primary/10 data-[state=active]:text-primary" | |
| onClick={() => | |
| updateQuery({ | |
| ...{ encounter_class: encounterClass, priority }, | |
| status: "in_progress", | |
| }) | |
| } | |
| onClick={() => { | |
| if (status === "in_progress") { | |
| updateQuery({ | |
| ...{ encounter_class: encounterClass, priority }, | |
| status: undefined, | |
| }); | |
| } else { | |
| updateQuery({ | |
| ...{ encounter_class: encounterClass, priority }, | |
| status: "in_progress", | |
| }); | |
| } | |
| }} | |
| > | |
| <CareIcon icon="l-spinner" className="mr-2 h-4 w-4" /> | |
| {t("encounter_class__in_progress")} | |
| </TabsTrigger> | |
| <TabsTrigger | |
| value="discharged" | |
| className="data-[state=active]:bg-primary/10 data-[state=active]:text-primary" | |
| onClick={() => | |
| updateQuery({ | |
| ...{ encounter_class: encounterClass, priority }, | |
| status: "discharged", | |
| }) | |
| } | |
| onClick={() => { | |
| if (status === "discharged") { | |
| updateQuery({ | |
| ...{ encounter_class: encounterClass, priority }, | |
| status: undefined, | |
| }); | |
| } else { | |
| updateQuery({ | |
| ...{ encounter_class: encounterClass, priority }, | |
| status: "discharged", | |
| }); | |
| } | |
| }} | |
| > | |
| <CareIcon icon="l-home" className="mr-2 h-4 w-4" /> | |
| {t("discharge")} | |
| </TabsTrigger> | |
| <TabsTrigger | |
| value="completed" | |
| className="data-[state=active]:bg-primary/10 data-[state=active]:text-primary" | |
| onClick={() => | |
| updateQuery({ | |
| ...{ encounter_class: encounterClass, priority }, | |
| status: "completed", | |
| }) | |
| } | |
| onClick={() => { | |
| if (status === "completed") { | |
| updateQuery({ | |
| ...{ encounter_class: encounterClass, priority }, | |
| status: undefined, | |
| }); | |
| } else { | |
| updateQuery({ | |
| ...{ encounter_class: encounterClass, priority }, | |
| status: "completed", | |
| }); | |
| } | |
| }} | |
| > | |
| <CareIcon icon="l-check" className="mr-2 h-4 w-4" /> | |
| {t("completed")} | |
| </TabsTrigger> | |
| <TabsTrigger | |
| value="cancelled" | |
| className="data-[state=active]:bg-primary/10 data-[state=active]:text-primary" | |
| onClick={() => | |
| updateQuery({ | |
| ...{ encounter_class: encounterClass, priority }, | |
| status: "cancelled", | |
| }) | |
| } | |
| onClick={() => { | |
| if (status === "cancelled") { | |
| updateQuery({ | |
| ...{ encounter_class: encounterClass, priority }, | |
| status: undefined, | |
| }); | |
| } else { | |
| updateQuery({ | |
| ...{ encounter_class: encounterClass, priority }, | |
| status: "cancelled", | |
| }); | |
| } | |
| }} | |
| > | |
| <Select | |
| value={status || "all"} | |
| onValueChange={(value) => { | |
| updateQuery({ | |
| ...{ encounter_class: encounterClass, priority }, | |
| - status: value === "all" ? undefined : value, | |
| + status: value === "all" ? undefined : (status === value ? undefined : value), | |
| }); | |
| }} | |
| > | |
| {/* ...other mobile dropdown options... */} | |
| </Select> |
|
Hey @siddheshThorat04, I appreciate your enthusiasm in contributing! However, the issue hasn't been assigned or approved for you yet. It would be great if you could wait for confirmation before submitting a PR. Thanks for understanding! |
|
Ok, i thought if issue is created by own then no need to wait for it to be assigned. |
it's generally good practice to wait for an issue to be approved and assigned before working on it |
|
Ok I'll keep that in mind |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
video_7.mp4
Summary by CodeRabbit
New Features
Style