Added scroll and responsiveness to the plots page #9815
Conversation
WalkthroughThe pull request enhances the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/pages/Encounters/tabs/EncounterPlotsTab.tsx (1)
46-54: Enhance scroll implementation for better UX and accessibilityWhile the horizontal scroll implementation works, consider these improvements:
- Add
role="tablist"andaria-orientation="horizontal"for better accessibility- Use
overflow-x-autoinstead ofoverflow-x-scrollto hide scrollbar when unnecessary- Consider adding scroll indicators or navigation buttons for better UX
- <div className="overflow-x-scroll w-full"> + <div + className="overflow-x-auto w-full relative" + role="tablist" + aria-orientation="horizontal" + >
|
|
||
| {data.map((tab) => ( | ||
| <TabsContent key={tab.id} value={tab.id}> | ||
| <ObservationVisualizer | ||
| patientId={props.patient.id} | ||
| codeGroups={tab.groups} | ||
| gridCols={2} | ||
| gridCols={window.innerWidth >= 768 ? 2 : 1} |
There was a problem hiding this comment.
Fix critical issues with window dimension handling
The direct usage of window.innerWidth can cause several issues:
- SSR hydration mismatches
- No handling of window resize events
- Potential performance impact from frequent re-renders
Consider using a proper window resize hook implementation:
// hooks/useWindowSize.ts
import { useState, useEffect } from 'react';
import debounce from 'lodash/debounce';
export function useWindowSize() {
const [width, setWidth] = useState(0);
useEffect(() => {
// Initialize on client-side
setWidth(window.innerWidth);
const handleResize = debounce(() => {
setWidth(window.innerWidth);
}, 250);
window.addEventListener('resize', handleResize);
return () => window.removeEventListener('resize', handleResize);
}, []);
return width;
}Then update the component:
+ import { useWindowSize } from '@/hooks/useWindowSize';
export const EncounterPlotsTab = (props: EncounterTabProps) => {
+ const windowWidth = useWindowSize();
// ...
<ObservationVisualizer
patientId={props.patient.id}
codeGroups={tab.groups}
- gridCols={window.innerWidth >= 768 ? 2 : 1}
+ gridCols={windowWidth >= 768 ? 2 : 1}
/>Alternatively, consider using CSS Grid with media queries if the grid layout doesn't need to be controlled programmatically.
| @@ -21,6 +23,8 @@ export const EncounterPlotsTab = (props: EncounterTabProps) => { | |||
| const { t } = useTranslation(); | |||
| const [qParams, setQParams] = useQueryParams<QueryParams>(); | |||
|
|
|||
| const { width } = useWindowDimensions(); | |||
There was a problem hiding this comment.
use useBreakpoints instead for standard window sizes.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/pages/Encounters/tabs/EncounterPlotsTab.tsx (2)
50-58: Consider adding scroll affordance indicatorsThe horizontal scrolling implementation is clean and performant. However, users might not realize there are more tabs if they're hidden.
Consider adding visual indicators when tabs overflow:
-<div className="overflow-x-scroll w-full"> +<div className="relative w-full"> + <div className="overflow-x-scroll w-full"> <TabsList> {data.map((tab) => ( <TabsTrigger key={tab.id} value={tab.id}> {tab.name} </TabsTrigger> ))} </TabsList> + </div> + <div className="absolute right-0 top-0 bottom-0 w-8 bg-gradient-to-l from-background to-transparent pointer-events-none" + style={{ opacity: isScrollable ? 1 : 0 }} /> </div>
65-65: Consider using CSS Grid for responsive layoutWhile the current implementation works, consider using CSS Grid with media queries if the grid layout doesn't need to be controlled programmatically.
Example implementation:
-gridCols={width >= 768 ? 2 : 1} +className="grid grid-cols-1 md:grid-cols-2 gap-4"This would:
- Remove the dependency on JavaScript for layout
- Improve performance by handling responsiveness in CSS
- Work during SSR without hydration issues
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Encounters/tabs/EncounterPlotsTab.tsx(3 hunks)
🔇 Additional comments (1)
src/pages/Encounters/tabs/EncounterPlotsTab.tsx (1)
26-26: Clean hook usage!Good job avoiding direct window.innerWidth access and using a custom hook instead.
| @@ -11,6 +11,8 @@ import { | |||
| } from "@/components/Common/Charts/ObservationChart"; | |||
| import Loading from "@/components/Common/Loading"; | |||
|
|
|||
| import useWindowDimensions from "@/hooks/useWindowDimensions"; | |||
There was a problem hiding this comment.
💡 Codebase verification
Add SSR safety check and debounce resize events
The current implementation of useWindowDimensions has two issues:
- Potential SSR hydration mismatches due to direct window access during initialization
- Missing debouncing for resize events which can impact performance
Suggested improvements:
- Add SSR safety check: Initialize state as
undefinedand update in useEffect - Add debouncing to resize handler using lodash.debounce or similar utility
🔗 Analysis chain
Verify the window dimensions hook implementation
The implementation of useWindowDimensions should properly handle:
- SSR hydration mismatches
- Window resize events with debouncing
- Cleanup of event listeners
Let's verify the hook implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the useWindowDimensions hook implementation
ast-grep --pattern 'export $_ function useWindowDimensions() {
$$$
}'
Length of output: 73
Script:
#!/bin/bash
# First, find the hook file
fd "useWindowDimensions" --type f
# Then try to find the implementation with different patterns
echo "=== Named export pattern ==="
ast-grep --pattern 'export const useWindowDimensions = $_'
echo "=== Default export pattern ==="
ast-grep --pattern 'export default function $_() {
$$$
}'
echo "=== Alternative: Search with ripgrep for more context ==="
rg "useWindowDimensions" -A 10
Length of output: 66367
Script:
#!/bin/bash
# Get the complete implementation of useWindowDimensions hook
cat src/hooks/useWindowDimensions.ts
Length of output: 781
| @@ -21,6 +23,8 @@ export const EncounterPlotsTab = (props: EncounterTabProps) => { | |||
| const { t } = useTranslation(); | |||
| const [qParams, setQParams] = useQueryParams<QueryParams>(); | |||
|
|
|||
| const isSmallDevice = useBreakpoints({ default: true, md: false }); | |||
There was a problem hiding this comment.
| const isSmallDevice = useBreakpoints({ default: true, md: false }); | |
| const plotColumns = useBreakpoints({ default: 1, md: 2 }); |
|
|
||
| {data.map((tab) => ( | ||
| <TabsContent key={tab.id} value={tab.id}> | ||
| <ObservationVisualizer | ||
| patientId={props.patient.id} | ||
| encounterId={props.encounter.id} | ||
| codeGroups={tab.groups} | ||
| gridCols={2} | ||
| gridCols={isSmallDevice ? 1 : 2} |
There was a problem hiding this comment.
| gridCols={isSmallDevice ? 1 : 2} | |
| gridCols={plotColumns} |
…nt-of-plots-and-overflowing
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/pages/Encounters/tabs/EncounterPlotsTab.tsx (1)
50-58: Consider enhancing the scroll UXWhile the horizontal scrolling implementation works, consider these UX improvements:
- Add smooth scrolling behavior
- Add visual indicators for overflow
- Enhance keyboard navigation accessibility
- <div className="overflow-x-scroll w-full"> + <div className="overflow-x-auto w-full scroll-smooth relative"> + <div className="absolute right-0 top-0 bottom-0 w-8 bg-gradient-to-l from-background to-transparent pointer-events-none" aria-hidden="true" /> <TabsList>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Encounters/tabs/EncounterPlotsTab.tsx(3 hunks)
🔇 Additional comments (2)
src/pages/Encounters/tabs/EncounterPlotsTab.tsx (2)
14-15: LGTM! Good choice using useBreakpointsThe implementation correctly uses the
useBreakpointshook for responsive behavior, which is a better approach than direct window dimension handling. This addresses the previous review comments and provides a more maintainable solution.Also applies to: 26-27
66-66: LGTM! Clean responsive implementationThe grid columns implementation effectively handles responsiveness using the
isSmallDevicebreakpoint. This is a clean and maintainable approach that aligns well with the PR objectives.
…ng' of https://github.com/Utkarsh-Anandani/care_fe into issues/ohcnetwork#9788/Alignment-of-plots-and-overflowing
| onValueChange={(value) => setQParams({ plot: value })} | ||
| > |
There was a problem hiding this comment.
instead of adding a div with className, we can simply add className to the parent Tabs component ryt?
There was a problem hiding this comment.
Yeah, I tried that but it gives scroll to the whole component including the plots idk if that behavior was wanted, looks weird to me...
There was a problem hiding this comment.
oh my bad. Tried on TabsList? lots of other usages seems to be working fine with TabsList
There was a problem hiding this comment.
Yeah, it gives a vertical scroll but not a horizontal scroll
There was a problem hiding this comment.
are you sure? it's working fine for me
There was a problem hiding this comment.
<TabsList className="overflow-x-scroll">
You're using this className?
<TabsList className="overflow-auto w-full">
using this it cuts out at left
There was a problem hiding this comment.
like cuts out means it just overflows at left side
are you sure? it's working fine for me
what classes are you using for it to work??
|
Combined with PR #9782 |
|
closed |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit