FIXED: The assets configuration page lacks mobile responsiveness #9467
Conversation
WalkthroughThe pull request modifies the Changes
Assessment against linked issues
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. |
![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
🧹 Nitpick comments (1)
src/components/Assets/AssetType/HL7Monitor.tsx (1)
Line range hint
93-164: Overall mobile responsiveness implementation feedback.The responsive design implementation using Tailwind classes is good, but consider these architectural suggestions for better mobile UX:
- Consider implementing a mobile-first approach starting with full width on smaller screens
- Test the layout with different content lengths to ensure proper wrapping
- Verify that form inputs and buttons are touch-friendly (min 44px tap targets)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Assets/AssetType/HL7Monitor.tsx(4 hunks)
🔇 Additional comments (1)
src/components/Assets/AssetType/HL7Monitor.tsx (1)
Line range hint 154-162: Verify mobile responsiveness of VentilatorPatientVitalsMonitor.
While the component addition looks good, please ensure that the VentilatorPatientVitalsMonitor component itself is mobile-responsive.
✅ Verification successful
VentilatorPatientVitalsMonitor component is properly mobile-responsive
The component implements responsive design through:
- Flexbox layouts with
flex-colthat adjust toflex-rowon medium screens (md:) - Responsive text sizing (
text-4xltomd:text-6xl) - Mobile-first approach with proper breakpoints
- Responsive divisions and spacing using Tailwind's responsive modifiers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for responsive classes in the VentilatorPatientVitalsMonitor component
rg -A 5 "className" "src/components/VitalsMonitor/VentilatorPatientVitalsMonitor"
Length of output: 179
Script:
#!/bin/bash
# First, let's find the correct path for the VentilatorPatientVitalsMonitor component
fd "VentilatorPatientVitalsMonitor" --type f
# Then check for responsive design patterns in the found files
rg -i "className|style|@media|responsive|mobile|width|flex" $(fd "VentilatorPatientVitalsMonitor" --type f)
Length of output: 1743
|
Check the failing cypress test; should be related to the button changes. |
… to find element: #submit, but never found it.
|
@Jacobjeevan please check the updation |
|
@rithviknishad @Jacobjeevan @bodhish how to resolve these cypress errors
|
|
@rithviknishad can you please check the updation |
…onsive-configurePage
….com/modamaan/care_feAmaan into issues/9465/responsive-configurePage
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/VitalsMonitor/HL7PatientVitalsMonitor.tsx (2)
50-50: Consider improving small screen responsivenessThe current width setup (
w-[55%]) might be too narrow for small screens before hitting the medium breakpoint. Consider using a more flexible approach for smaller screens.- <div className="flex flex-col gap-1 rounded bg-[#020617] p-2 w-[55%] m-1 md:w-full md:m-0"> + <div className="flex flex-col gap-1 rounded bg-[#020617] p-2 w-full sm:w-[55%] m-1 md:w-full md:m-0">
Line range hint
50-207: Consider accessibility improvements for vital signsWhile the responsive layout looks good, consider adding the following accessibility improvements:
- Add
aria-labelattributes to vital sign sections for screen readers- Ensure sufficient color contrast for all vital sign values
- Consider adding
role="status"to dynamically updating valuesExample implementation for the ECG section:
<NonWaveformData label="ECG" attr={data.pulseRate?.value ? data.pulseRate : data.heartRate} className="text-green-400" + aria-label={`ECG ${data.pulseRate?.value || data.heartRate?.value || 'no reading'}`} + role="status" suffix={ <span className="animate-pulse font-sans text-red-500">❤️</span> } />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/VitalsMonitor/HL7PatientVitalsMonitor.tsx(3 hunks)
🔇 Additional comments (2)
src/components/VitalsMonitor/HL7PatientVitalsMonitor.tsx (2)
153-153: Well-implemented responsive layout
The use of flex-wrap with md:flex-nowrap is a good approach for handling responsive layout transitions between mobile and desktop views.
206-206: Verify absolute positioning behavior
While the responsive adjustments look good, the md:absolute positioning could cause layout issues if the parent container's height is not properly constrained. Please verify the following scenarios:
- Content overflow behavior when vital signs have long values
- Layout stability when switching between mobile and desktop views
- Container height consistency across different screen sizes
✅ Verification successful
Let me gather more information about the VitalsNonWaveformContent component and its layout structure.
Let me check the NonWaveformData component to understand the content structure and any potential overflow handling.
Based on the analysis of the code, I can now provide the final response:
Absolute positioning implementation is safe
The absolute positioning implementation is well-constrained and safe because:
- The parent container uses
relative flex flex-col md:flex-rowwhich properly establishes the positioning context - The
NonWaveformDatacomponent has a controlled layout with fixed text sizes (text-4xl/text-6xl) and consistent padding - The grid layout with
md:grid-cols-1ensures content stacks vertically on desktop - The
inset-y-0with the parent's relative positioning ensures the height is properly bounded
The responsive behavior will work correctly as:
- Mobile: Grid-based layout with controlled gaps
- Desktop: Height-constrained absolute positioning with single column
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for height constraints on parent containers
rg -A 2 "relative flex flex-col.*md:flex-row" src/
Length of output: 1148
Script:
#!/bin/bash
# Check VitalsNonWaveformContent implementation and its layout constraints
ast-grep --pattern 'const VitalsNonWaveformContent = $$$'
# Search for any height-related styles in the component
rg -B 2 -A 2 "VitalsNonWaveformContent" src/
Length of output: 3835
Script:
#!/bin/bash
# Check NonWaveformData implementation for content structure
ast-grep --pattern 'const NonWaveformData = $$$'
# Search for any max-height or overflow handling in related components
rg "max-h|overflow" src/components/VitalsMonitor/
Length of output: 3473
|
@Jacobjeevan HL7 monitor ventilator monitor |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/VitalsMonitor/VentilatorPatientVitalsMonitor.tsx (1)
28-28: Consider adopting a mobile-first approach with fluid width.The current implementation uses a fixed desktop-first approach which might cause layout issues. Consider:
- Following mobile-first principles by making mobile styles the default
- Using fluid width with max-width constraints instead of fixed percentage
- <div className="flex flex-col gap-1 rounded bg-[#020617] p-2 w-[55%] m-1 md:w-full md:m-0"> + <div className="flex flex-col gap-1 rounded bg-[#020617] p-2 w-full m-0 lg:max-w-[600px] lg:m-1">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/VitalsMonitor/VentilatorPatientVitalsMonitor.tsx(1 hunks)
🔇 Additional comments (2)
src/components/VitalsMonitor/VentilatorPatientVitalsMonitor.tsx (2)
Line range hint 33-65: Verify canvas responsiveness and layout stability.
While the flex-wrap addition improves responsiveness, please verify:
- Canvas elements scale properly on different screen sizes
- Layout remains stable when switching between online/offline states
✅ Verification successful
Let me gather more information about the canvas size handling and responsive design implementation.
Canvas responsiveness and layout stability are properly implemented.
The implementation shows robust handling of canvas responsiveness and layout stability:
- Canvas size is managed through
rendererConfig.sizewhich is obtained fromgetVitalsCanvasSizeAndDuration()and consistently applied to all canvas elements - The container uses proper responsive classes:
- Base container has
w-[55%]that switches tomd:w-fullon medium screens - Flex layouts adapt with
md:flex-rowandmd:flex-nowrapbreakpoints - Content visibility is controlled via
hiddenclass with proper state management throughisOnline
- Base container has
- Layout stability is maintained through:
- Absolute positioning of canvas elements with consistent sizing
- Proper state transitions between online/offline modes using
classNamesutility - Consistent spacing and positioning across different viewport sizes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for canvas size calculations and responsive handling
ast-grep --pattern 'const $_ = useVentilatorVitalsMonitor($_)' -A 10
rg -A 5 'waveformCanvas.size'
Length of output: 5926
Script:
#!/bin/bash
# Search for the hook implementation to understand canvas size handling
ast-grep --pattern 'function useVentilatorVitalsMonitor($_) {
$$$
}'
# Search for responsive classes and container styles
rg -A 2 'md:|sm:|lg:' src/components/VitalsMonitor/
Length of output: 17949
Line range hint 28-91: Address failing Cypress tests mentioned in PR comments.
The layout changes might affect element positioning that Cypress tests rely on. Consider:
- Updating Cypress selectors if they depend on specific layout structure
- Adding data-testid attributes for more reliable test selection
|
@modamaan the screen is not responsive in mobile screen |
|
@nihal467 i didnt get the real time data for testing |
|
@Jacobjeevan I logged into the |
Sorry, I meant dev-districtadmin. But anyways, don't worry about it. Issue is no longer relevant as we are removing/reworking assets 👍 |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Chores