Fixed:Show more monitors in CNS on very large displays #9028
Conversation
WalkthroughThe changes in Changes
Assessment against linked issues
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 failed.
|
| const getDimension = useWindowDimensions(); | ||
|
|
||
| if (getDimension.width > 2080) { | ||
| PER_PAGE_LIMIT = 9; | ||
| } else { | ||
| PER_PAGE_LIMIT = 6; | ||
| } |
There was a problem hiding this comment.
use useBreakpoints hook instead.
| @@ -41,11 +40,19 @@ interface Props { | |||
| } | |||
|
|
|||
| export default function CentralNursingStation({ facilityId }: Props) { | |||
| let PER_PAGE_LIMIT; | |||
There was a problem hiding this comment.
| let PER_PAGE_LIMIT; |
| @@ -216,7 +223,7 @@ export default function CentralNursingStation({ facilityId }: Props) { | |||
| {t("no_vitals_present")} | |||
| </div> | |||
| ) : ( | |||
| <div className="3xl:grid-cols-3 mt-1 grid grid-cols-1 gap-1 lg:grid-cols-2"> | |||
| <div className="mt-1 grid grid-cols-1 gap-1 lg:grid-cols-2 min-[2080px]:grid-cols-3"> | |||
There was a problem hiding this comment.
avoid hardcoding abritrary pixel values. prefer rem values over pixel values as pixel values may behave differently for different zoom settings.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
src/components/Facility/CentralNursingStation.tsx (1)
226-226: Enhance grid layout for better responsiveness.The current implementation uses a fixed breakpoint for 3 columns. Consider using CSS Grid's auto-fit/auto-fill for more flexible layouts that automatically adjust based on available space.
Consider this alternative approach:
- <div className="mt-1 grid grid-cols-1 gap-1 lg:grid-cols-2 min-[2080px]:grid-cols-3"> + <div className="mt-1 grid gap-1 grid-cols-[repeat(auto-fit,minmax(min(100%,400px),1fr))]">This approach:
- Automatically adjusts columns based on container width
- Ensures monitors maintain readable dimensions
- Eliminates the need for fixed breakpoints
| const getDimension = useWindowDimensions(); | ||
|
|
||
| if (getDimension.width > 2080) { | ||
| PER_PAGE_LIMIT = 9; | ||
| } else { | ||
| PER_PAGE_LIMIT = 6; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve window dimension check implementation.
Several improvements can be made to the window dimension check:
- Extract the magic number 2080 as a named constant
- Consider DPI scaling for more accurate breakpoints
- Simplify the implementation
Apply these improvements:
+const LARGE_SCREEN_BREAKPOINT = 2080; // Consider moving to a constants file
+
- if (getDimension.width > 2080) {
- PER_PAGE_LIMIT = 9;
- } else {
- PER_PAGE_LIMIT = 6;
- }
+ // Consider using window.devicePixelRatio for DPI-aware breakpoints
+ const effectiveWidth = getDimension.width * window.devicePixelRatio;
+ const PER_PAGE_LIMIT = effectiveWidth > LARGE_SCREEN_BREAKPOINT ? 9 : 6;📝 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.
| const getDimension = useWindowDimensions(); | |
| if (getDimension.width > 2080) { | |
| PER_PAGE_LIMIT = 9; | |
| } else { | |
| PER_PAGE_LIMIT = 6; | |
| } | |
| const LARGE_SCREEN_BREAKPOINT = 2080; // Consider moving to a constants file | |
| const getDimension = useWindowDimensions(); | |
| // Consider using window.devicePixelRatio for DPI-aware breakpoints | |
| const effectiveWidth = getDimension.width * window.devicePixelRatio; | |
| const PER_PAGE_LIMIT = effectiveWidth > LARGE_SCREEN_BREAKPOINT ? 9 : 6; |
| let PER_PAGE_LIMIT; | ||
| const { t } = useTranslation(); | ||
| const [isFullscreen, setFullscreen] = useFullscreen(); | ||
| const { qParams, updateQuery, removeFilter, updatePage } = useFilters({ | ||
| limit: PER_PAGE_LIMIT, | ||
| }); |
There was a problem hiding this comment.
Fix variable initialization order issue.
The PER_PAGE_LIMIT is used in useFilters hook before being initialized. This could lead to undefined behavior as the value will be undefined during the first render.
Move the initialization before the hook usage:
- let PER_PAGE_LIMIT;
- const { t } = useTranslation();
- const [isFullscreen, setFullscreen] = useFullscreen();
- const { qParams, updateQuery, removeFilter, updatePage } = useFilters({
- limit: PER_PAGE_LIMIT,
- });
- const getDimension = useWindowDimensions();
+ const { t } = useTranslation();
+ const [isFullscreen, setFullscreen] = useFullscreen();
+ const getDimension = useWindowDimensions();
+ const PER_PAGE_LIMIT = getDimension.width > 2080 ? 9 : 6;
+ const { qParams, updateQuery, removeFilter, updatePage } = useFilters({
+ limit: PER_PAGE_LIMIT,
+ });📝 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.
| let PER_PAGE_LIMIT; | |
| const { t } = useTranslation(); | |
| const [isFullscreen, setFullscreen] = useFullscreen(); | |
| const { qParams, updateQuery, removeFilter, updatePage } = useFilters({ | |
| limit: PER_PAGE_LIMIT, | |
| }); | |
| const { t } = useTranslation(); | |
| const [isFullscreen, setFullscreen] = useFullscreen(); | |
| const getDimension = useWindowDimensions(); | |
| const PER_PAGE_LIMIT = getDimension.width > 2080 ? 9 : 6; | |
| const { qParams, updateQuery, removeFilter, updatePage } = useFilters({ | |
| limit: PER_PAGE_LIMIT, | |
| }); |
| @@ -23,12 +23,11 @@ import { getVitalsMonitorSocketUrl } from "@/components/VitalsMonitor/utils"; | |||
|
|
|||
| import useFilters from "@/hooks/useFilters"; | |||
| import useFullscreen from "@/hooks/useFullscreen"; | |||
| import useWindowDimensions from "@/hooks/useWindowDimensions"; | |||
There was a problem hiding this comment.
💡 Codebase verification
Window dimension hook needs throttling for performance optimization
The current implementation of useWindowDimensions directly attaches to the window resize event without any throttling mechanism, which can lead to excessive re-renders during window resizing. Add throttling to the resize event handler:
// Example modification
import { useEffect, useState } from "react";
import { throttle } from "lodash"; // or implement your own throttle
export default function useWindowDimensions() {
const [windowDimensions, setWindowDimensions] = useState(getWindowDimensions());
useEffect(() => {
const handleResize = throttle(() => {
setWindowDimensions(getWindowDimensions());
}, 200); // throttle to 200ms
window.addEventListener("resize", handleResize);
return () => window.removeEventListener("resize", handleResize);
}, []);
return windowDimensions;
}🔗 Analysis chain
Verify window dimension hook implementation.
The useWindowDimensions hook should implement proper event throttling to avoid performance issues during window resizing.
Let's verify the hook implementation:
Let me check the implementation of the getWindowDimensions function since it's used in the hook.
Let me try a different approach to find the complete implementation of the hook and any related functions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the useWindowDimensions hook implementation
rg -A 10 "useWindowDimensions" --type ts
Length of output: 8535
Script:
#!/bin/bash
# Check the getWindowDimensions function implementation
rg -A 5 "function getWindowDimensions" --type ts
Length of output: 50
Script:
#!/bin/bash
# Get the complete content of useWindowDimensions.ts
fd useWindowDimensions.ts --exec cat {}
Length of output: 786
|
Closing as duplicate of #9013 |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Improvements