Occupancy bed tooltip fully visible in mobile view #9235
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request focus on enhancing the Changes
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (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: 0
🧹 Outside diff range and nitpick comments (1)
src/components/Facility/FacilityCard.tsx (1)
105-105: Consider simplifying responsive positioning logic.While the current positioning works, the multiple breakpoint-specific translations (
lg:-translate-x-1/2,xl:-translate-x-1/4) might lead to jumpy behavior during viewport resizing.Consider simplifying to a single, consistent translation that works across all viewports:
-<span className="tooltip-text tooltip-top absolute left-1/2 transform -translate-x-1/4 mb-1 z-10 lg:left-1/2 lg:-translate-x-1/2 xl:left-1/2 xl:-translate-x-1/4"> +<span className="tooltip-text tooltip-top absolute left-1/2 transform -translate-x-1/2 mb-1 z-10">This change:
- Uses
-translate-x-1/2consistently across all breakpoints- Removes unnecessary repetition of
left-1/2- Maintains proper centering regardless of viewport size
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Facility/FacilityCard.tsx(1 hunks)
🔇 Additional comments (2)
src/components/Facility/FacilityCard.tsx (2)
103-103: LGTM: Container positioning setup is correct.
The addition of the relative class to the occupancy badge container creates the necessary positioning context for the absolute-positioned tooltip.
103-107: Verify tooltip visibility in all scenarios.
The tooltip positioning looks correct, but let's verify it remains fully visible:
- When the facility name is very long
- When the occupancy ratio triggers the red background
- At the edges of the screen
✅ Verification successful
Tooltip implementation appears robust and well-positioned.
The tooltip implementation is well-designed with proper visibility handling:
- Global CSS styles handle tooltip positioning, transitions, and visibility
- The tooltip uses responsive positioning with different breakpoints (lg, xl)
- The z-index (z-10) ensures proper stacking
- The transform and translate properties ensure the tooltip stays centered
- The whitespace-nowrap class prevents text wrapping
- The pointer-events-none class prevents tooltip interference
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases covering tooltip visibility
rg -l "occupancy-badge|tooltip" --type=test
Length of output: 74
Script:
#!/bin/bash
# Search for test files related to FacilityCard component
fd "FacilityCard.*\.(test|spec)\.(ts|tsx|js|jsx)$"
# Search for any test files containing tooltip or occupancy related tests
rg -l "tooltip|occupancy" "**/*.(test|spec).(ts|tsx|js|jsx)"
# Check the actual component implementation for tooltip usage patterns
rg -A 3 "tooltip" "src/components/Facility/FacilityCard.tsx"
Length of output: 881
Script:
#!/bin/bash
# Let's check for any existing tooltip styles in CSS files
rg -l "tooltip" "**/*.css" "**/*.scss"
# Check for any tooltip components or utilities
fd "tooltip" -e tsx -e ts
# Look for any tooltip-related tests with correct glob pattern
rg -l "tooltip" $(fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts)
Length of output: 2756
Script:
#!/bin/bash
# Let's check the tooltip component implementation and styles
cat src/components/ui/tooltip.tsx
# Check the global styles for tooltips
cat src/style/CAREUI.css
# Look at the specific implementation context
rg -B 5 -A 10 "tooltip" src/components/Facility/FacilityCard.tsx
Length of output: 8494
There was a problem hiding this comment.
Let's switch to newer approach of showing tooltip using shadcn's tooltip component: https://github.com/ohcnetwork/care_fe/blob/develop/src/components/ui/tooltip.tsx
This shadcn tooltip component working well in desktop, but it is not working on mobile view how should I proceed? |
|
Could you push the changes so that we can take a look at it? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/components/Facility/FacilityCard.tsx (1)
108-136: Accessibility and performance improvementsConsider these additional enhancements:
- Add aria-label for better screen reader support
- Adjust delay duration for better mobile experience
- Consider touch event handling
Apply these changes for better accessibility and performance:
-<Tooltip delayDuration={0}> +<Tooltip delayDuration={150}> <TooltipTrigger asChild> <button data-test-id="occupancy-badge" + aria-label={t("occupancy_details")} className={`relative flex items-center gap-1 text-sm ${ (facility.patient_count || 0) / (facility.bed_count || 0) > 0.85 ? "justify-center rounded-md border border-red-600 bg-red-500 p-1 font-bold text-white" : "text-secondary-700" }`} + onTouchStart={(e) => { + // Prevent tooltip from staying open on scroll + e.currentTarget.blur(); + }} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Facility/FacilityCard.tsx(2 hunks)
🔇 Additional comments (2)
src/components/Facility/FacilityCard.tsx (2)
22-27: LGTM: Clean import of tooltip components
The imports are well-structured and include all necessary components from shadcn's tooltip package.
133-133: Verify translation key and tooltip usage consistency
Let's ensure the translation key exists and check for consistent tooltip usage across the codebase.
✅ Verification successful
Tooltip implementation and translation key are correctly used
The implementation is using the standardized shadcn/ui tooltip component from @/components/ui/tooltip.tsx which is based on Radix UI, and the translation key exists in the provided locale file. The tooltip usage is consistent with other parts of the codebase that import from the same tooltip component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the translation key exists in i18n files
echo "Checking for translation key 'live_patients_total_beds'..."
fd -e json . | xargs grep -l "live_patients_total_beds"
# Check for other tooltip implementations that might need similar updates
echo "Checking for other tooltip implementations..."
rg -l "tooltip" --type tsx --type jsx
# Check for shadcn tooltip usage patterns
echo "Checking for shadcn tooltip usage patterns..."
rg -l "TooltipContent|TooltipTrigger|TooltipProvider" --type tsx --type jsx
Length of output: 584
Script:
#!/bin/bash
# Let's check the content of the translation file to verify the key
echo "Checking content of translation file..."
cat public/locale/en.json
# Let's try a different approach to find tooltip implementations
echo "Checking for tooltip patterns..."
rg -t js -t jsx -t typescript -t tsx "Tooltip"
# Let's also check the specific component's tooltip implementation
echo "Checking FacilityCard tooltip implementation..."
cat src/components/Facility/FacilityCard.tsx
Length of output: 86663
Script:
#!/bin/bash
# Let's check for tooltip implementations in the codebase
echo "Checking for tooltip implementations..."
rg -t typescript -t typescriptreact "import.*tooltip" --no-filename
# Let's also check for translation key usage
echo "Checking for live_patients_total_beds translation key usage..."
rg -t typescript -t typescriptreact "live_patients_total_beds" --no-filename
Length of output: 440
Script:
#!/bin/bash
# Let's try a different approach to find tooltip implementations
echo "Checking for tooltip imports..."
rg "from ['\"].*tooltip" -g "*.tsx" -g "*.ts"
# Let's check the UI components directory for tooltip implementation
echo "Checking UI components directory..."
find . -type f -name "tooltip.tsx" -o -name "Tooltip.tsx" | xargs cat
# Let's check for the translation key usage
echo "Checking translation key usage..."
rg "live_patients_total_beds" -g "*.tsx" -g "*.ts"
Length of output: 1897
Could you please look into the changes |
|
I don't see any issues in this. What's the issue? |
In mobile view when we are clicking on the occupancy, the tooltip is appearing and disappearing quickly. |
|
@Rishith25 clear the merge conflict PR looks good for me, @rithviknishad @khavinshankar review it |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/ui/tooltip.tsx (1)
14-39: Consider enhancing mobile touch interactionWhile the current implementation works on mobile by using
onClick, it could be improved for better mobile UX:
- The tooltip might interfere with scrolling on touch devices
- No distinction between touch and mouse interactions
Consider this enhancement:
const TooltipComponent = React.forwardRef< React.ElementRef<typeof TooltipContent>, React.ComponentPropsWithoutRef<typeof TooltipPrimitive.Content> >(({ children, content, sideOffset = 4, className }, ref) => { const [open, setOpen] = React.useState(false); + const [isTouchDevice, setIsTouchDevice] = React.useState(false); + + React.useEffect(() => { + setIsTouchDevice('ontouchstart' in window); + }, []); return ( <TooltipProvider> - <Tooltip open={open} onOpenChange={setOpen} delayDuration={0}> + <Tooltip + open={open} + onOpenChange={setOpen} + delayDuration={isTouchDevice ? 0 : 200}> <TooltipTrigger asChild - onClick={() => setOpen(!open)} + onClick={() => isTouchDevice && setOpen(!open)} > {children} </TooltipTrigger> <TooltipContent ref={ref} sideOffset={sideOffset} className={cn( "z-50 overflow-hidden rounded-md bg-gray-900 px-3 py-1.5 text-xs text-gray-50 animate-in fade-in-0 zoom-in-95 data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=closed]:zoom-out-95 data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2 dark:bg-gray-50 dark:text-gray-900", className, )} > {content} </TooltipContent> </Tooltip> </TooltipProvider> ); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/Common/Sidebar/Sidebar.tsx(2 hunks)src/components/Facility/FacilityCard.tsx(2 hunks)src/components/ui/tooltip.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Facility/FacilityCard.tsx
🔇 Additional comments (3)
src/components/ui/tooltip.tsx (1)
12-13: LGTM: Clean re-export of Radix primitive
The direct re-export of TooltipPrimitive.Content simplifies the implementation by removing the previously unused state.
src/components/Common/Sidebar/Sidebar.tsx (2)
9-9: LGTM: Clean import of new tooltip components
The import has been simplified to only include the necessary components.
241-253: LGTM: Clean tooltip implementation with i18n support
The implementation:
- Uses the new TooltipComponent correctly
- Maintains internationalization for tooltip content
- Preserves existing styling and functionality
I have changed the tooltip component so could you please confirm is it fine or not. |
rithviknishad
added
changes required
cypress failed
@nihal467 @rithviknishad Can you please review it and all the test cases are passing. |
rithviknishad
removed
changes required
cypress failed
|
@Rishith25 Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌 |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
TooltipComponentfor better functionality.