Use shadcn Tooltip Components across FacilityCard #9317
Conversation
|
Warning Rate limit exceeded@nihal467 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 12 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes in Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
rajku-dev
changed the title
✅ 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: 2
🧹 Outside diff range and nitpick comments (1)
src/components/Facility/FacilityCard.tsx (1)
229-236: Add ARIA attributes for better accessibility.The tooltip implementation looks good and aligns with the PR objectives. However, consider enhancing accessibility by adding ARIA attributes.
<ButtonV2 id="facility-notify" ghost border - className="h-[38px] tooltip" + className="h-[38px] tooltip" + aria-label="Notify facility" + role="button" onClick={(_) => setNotifyModalFor(facility.id)} >
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/components/Facility/FacilityCard.tsx (3)
2-2: Fix import formattingAdd a space after the comma in the raviger import.
-import { Link,navigate } from "raviger"; +import { Link, navigate } from "raviger";🧰 Tools
🪛 eslint
[error] 2-2: Insert
·(prettier/prettier)
226-239: Improve Button component implementation
- The variant prop syntax is inconsistent with other props. Remove curly braces.
- The
role="button"is unnecessary as the Button component already provides this semantically.<Button id="facility-notify" - variant={"outline"} + variant="outline" className="h-[38px] tooltip" onClick={(_) => setNotifyModalFor(facility.id)} aria-label={t("notify")} - role="button" >Apply similar changes to the other Button components.
Also applies to: 241-256, 257-266
226-239: Enhance tooltip accessibility and consistencyThe tooltip implementation could be improved:
- Make aria-label more descriptive than just "notify"
- Consider adding tooltips to the View CNS button for consistency
<Button id="facility-notify" variant="outline" className="h-[38px] tooltip" onClick={(_) => setNotifyModalFor(facility.id)} - aria-label={t("notify")} + aria-label={t("notify_facility", { facility: facility.name })} >Would you like me to help implement tooltips for the View CNS button as well?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
public/locale/en.json(2 hunks)src/components/Facility/FacilityCard.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- public/locale/en.json
🧰 Additional context used
🪛 eslint
src/components/Facility/FacilityCard.tsx
[error] 2-2: Insert ·
(prettier/prettier)
[error] 4-5: Delete ";⏎import·{·Button·}·from·"@/components/ui/button
(prettier/prettier)
[error] 258-258: Replace ·navigate(/patients?facility=${facility.id}) with ⏎························navigate(/patients?facility=${facility.id})⏎······················
(prettier/prettier)
🔇 Additional comments (1)
src/components/Facility/FacilityCard.tsx (1)
226-266: Verify tooltip visibility on small viewports
The implementation aligns with the PR objectives for adding tooltips on screens < 768px. Let's verify the tooltip visibility and styling:
✅ Verification successful
Tooltip implementation is consistent and properly accessible
The implementation in FacilityCard.tsx follows the established tooltip pattern across the codebase:
- Tooltips are consistently implemented using
tooltipandtooltip-text tooltip-topclasses - Responsive visibility is properly handled with
sm:hiddenandmd:hiddenclasses - Accessibility is maintained with
aria-labelattributes on buttons - The pattern matches other components like AssetsList, DoctorVideoSlideover, and MedicineAdministrationSheet
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent tooltip implementation across components
# Check for tooltip class usage
echo "Checking tooltip class implementation..."
rg "tooltip-text tooltip-top" --type tsx
# Check for responsive classes
echo "Checking responsive visibility..."
rg "(sm|md):hidden.*tooltip" --type tsx
# Check for aria-label usage
echo "Checking accessibility attributes..."
rg "aria-label=.*" --type tsx
Length of output: 451
Script:
#!/bin/bash
# Description: Check for consistent tooltip implementation across components
# Check for tooltip class usage
echo "Checking tooltip class implementation..."
rg "tooltip-text tooltip-top"
# Check for responsive classes
echo "Checking responsive visibility..."
rg "(sm|md):hidden.*tooltip"
# Check for aria-label usage in Button components
echo "Checking accessibility attributes..."
rg "aria-label.*tooltip"
# Check for other tooltip implementations
echo "Checking other tooltip patterns..."
rg "className=.*tooltip"
Length of output: 15712
🧰 Tools
🪛 eslint
[error] 258-258: Replace ·navigate(/patients?facility=${facility.id}) with ⏎························navigate(/patients?facility=${facility.id})⏎······················
(prettier/prettier)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/components/Facility/FacilityCard.tsx (2)
2-5: Clean up import statementsThe import statements need cleanup:
- Fix spacing after raviger import
- Optimize ButtonV2 import since we only use Cancel and Submit
-import { Link,navigate } from "raviger"; +import { Link, navigate } from "raviger"; -import ButtonV2, { Cancel, Submit } from "@/components/Common/ButtonV2"; +import { Cancel, Submit } from "@/components/Common/ButtonV2";🧰 Tools
🪛 eslint
[error] 2-2: Insert
·(prettier/prettier)
[error] 4-5: Delete
";⏎import·{·Button·}·from·"@/components/ui/button(prettier/prettier)
231-232: Remove redundant button role attributeThe
role="button"attribute is redundant when using the Button component as it already has the semantic meaning of a button.-aria-label={t("notify")} -role="button" +aria-label={t("notify")}Also applies to: 246-247, 261-262
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Facility/FacilityCard.tsx(2 hunks)
🧰 Additional context used
🪛 eslint
src/components/Facility/FacilityCard.tsx
[error] 2-2: Insert ·
(prettier/prettier)
[error] 4-5: Delete ";⏎import·{·Button·}·from·"@/components/ui/button
(prettier/prettier)
[error] 258-258: Replace ·navigate(/patients?facility=${facility.id}) with ⏎························navigate(/patients?facility=${facility.id})⏎······················
(prettier/prettier)
🔇 Additional comments (1)
src/components/Facility/FacilityCard.tsx (1)
235-238: LGTM: Tooltip implementation meets requirements
The tooltip implementation correctly addresses the PR objective by:
- Showing tooltips only on mobile view (below 768px)
- Using proper translations for button labels
- Maintaining accessibility with aria-labels
Also applies to: 250-254
|
@Jacobjeevan UI after changes |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/components/Facility/FacilityCard.tsx (3)
2-2: Fix import formattingMinor formatting issues in the import statements need to be addressed.
-import { Link,navigate } from "raviger"; +import { Link, navigate } from "raviger"; -import { Button } from "@/components/ui/button"; +import { Button } from "@/components/ui/button";Also applies to: 5-5
🧰 Tools
🪛 eslint
[error] 2-2: Insert
·(prettier/prettier)
226-241: Fix Button variant syntax and approve accessibility improvementsThe accessibility improvements with
aria-labeland tooltips are good additions. However, the button variant syntax needs to be updated.<Button id="facility-notify" - variant={"outline_primary"} + variant="outline-primary" className="h-[38px] tooltip" onClick={(_) => setNotifyModalFor(facility.id)} aria-label={t("notify")} role="button" >🧰 Tools
🪛 eslint
[error] 235-237: Replace
⏎··························{t("notify")}⏎························with{t("notify")}(prettier/prettier)
243-268: Fix Button variant syntax for remaining buttonsThe accessibility improvements are good, but the variant syntax needs to be updated for consistency.
<Button onClick={() => navigate(`/facility/${facility.id}`)} id="facility-details" - variant={"outline_primary"} + variant="outline-primary" className="tooltip h-[38px]" aria-label={t("view_facility")} role="button" > <Button onClick={() => navigate(`/patients?facility=${facility.id}`)} id="facility-patients" - variant={"outline_primary"} + variant="outline-primary" aria-label={t("view_patients")} role="button" >🧰 Tools
🪛 eslint
[error] 260-260: Replace
·navigate(/patients?facility=${facility.id})with⏎························navigate(/patients?facility=${facility.id})⏎······················(prettier/prettier)
[error] 267-267: Delete
··(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Facility/FacilityCard.tsx(2 hunks)
🧰 Additional context used
🪛 eslint
src/components/Facility/FacilityCard.tsx
[error] 2-2: Insert ·
(prettier/prettier)
[error] 4-5: Delete ";⏎import·{·Button·}·from·"@/components/ui/button
(prettier/prettier)
[error] 235-237: Replace ⏎··························{t("notify")}⏎························ with {t("notify")}
(prettier/prettier)
[error] 260-260: Replace ·navigate(/patients?facility=${facility.id}) with ⏎························navigate(/patients?facility=${facility.id})⏎······················
(prettier/prettier)
[error] 267-267: Delete ··
(prettier/prettier)
🔇 Additional comments (1)
src/components/Facility/FacilityCard.tsx (1)
226-268: Implementation successfully meets PR objectives
The changes effectively address the PR objectives by:
- Adding tooltips for buttons on small viewports
- Enhancing accessibility with aria-labels
- Successfully migrating to the new Button component
The implementation is well-thought-out and includes additional accessibility improvements beyond the original requirements.
🧰 Tools
🪛 eslint
[error] 235-237: Replace ⏎··························{t("notify")}⏎························ with {t("notify")}
(prettier/prettier)
[error] 260-260: Replace ·navigate(/patients?facility=${facility.id}) with ⏎························navigate(/patients?facility=${facility.id})⏎······················
(prettier/prettier)
[error] 267-267: Delete ··
(prettier/prettier)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/components/Facility/FacilityCard.tsx (1)
2-5: Fix import formattingThe imports need proper spacing and ordering.
-import { Link,navigate } from "raviger"; +import { Link, navigate } from "raviger"; + import { useTranslation } from "react-i18next"; -import { Button } from "@/components/ui/button"; +import { Button } from "@/components/ui/button";🧰 Tools
🪛 eslint
[error] 2-2: Insert
·(prettier/prettier)
[error] 4-5: Delete
";⏎import·{·Button·}·from·"@/components/ui/button(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Facility/FacilityCard.tsx(3 hunks)
🧰 Additional context used
🪛 eslint
src/components/Facility/FacilityCard.tsx
[error] 2-2: Insert ·
(prettier/prettier)
[error] 4-5: Delete ";⏎import·{·Button·}·from·"@/components/ui/button
(prettier/prettier)
[error] 8-8: Insert on";⏎⏎import·{·Button·}·from·"@/components/ui/butt
(prettier/prettier)
[error] 234-236: Replace ⏎··························{t("notify")}⏎························ with {t("notify")}
(prettier/prettier)
[error] 259-259: Replace ·navigate(/patients?facility=${facility.id}) with ⏎························navigate(/patients?facility=${facility.id})⏎······················
(prettier/prettier)
[error] 266-266: Delete ··
(prettier/prettier)
🔇 Additional comments (2)
src/components/Facility/FacilityCard.tsx (2)
116-127: Fix Button variant syntax
The variant prop should use a hyphen instead of an underscore, following the component's convention.
-variant={"outline_primary"}
+variant="outline-primary"225-240: LGTM! Great accessibility improvements
The changes effectively address the PR objectives by:
- Adding tooltips for small viewports
- Including proper aria-labels and roles
- Maintaining consistent translations
However, there's one minor improvement needed:
-variant={"outline_primary"}
+variant="outline-primary"Also applies to: 242-257, 258-267
🧰 Tools
🪛 eslint
[error] 234-236: Replace ⏎··························{t("notify")}⏎························ with {t("notify")}
(prettier/prettier)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/components/Facility/FacilityCard.tsx (3)
226-239: Clean up onClick handler and fix variant syntaxTwo minor improvements needed:
<Button id="facility-notify" - variant={"outline_primary"} + variant="outline-primary" className="h-[38px] tooltip" - onClick={(_) => setNotifyModalFor(facility.id)} + onClick={() => setNotifyModalFor(facility.id)} aria-label={t("notify")} role="button" >
241-256: Fix variant syntaxThe translation key fix looks good, but the variant syntax needs to be updated.
<Button onClick={() => navigate(`/facility/${facility.id}`)} id="facility-details" - variant={"outline_primary"} + variant="outline-primary" className="tooltip h-[38px]" aria-label={t("view_facility")} role="button" >
257-268: Simplify onClick and fix variant syntaxThe onClick handler can be simplified, and the variant syntax needs to be updated.
<Button - onClick={() => - navigate(`/patients?facility=${facility.id}`) - } + onClick={() => navigate(`/patients?facility=${facility.id}`)} id="facility-patients" - variant={"outline_primary"} + variant="outline-primary" aria-label={t("view_patients")} role="button" >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Facility/FacilityCard.tsx(3 hunks)
🔇 Additional comments (2)
src/components/Facility/FacilityCard.tsx (2)
2-2: LGTM: Import changes are correct
The imports have been properly updated to support the migration from ButtonV2 to Button component while maintaining the necessary Cancel and Submit components.
Also applies to: 9-10, 12-12
Line range hint 117-268: LGTM: Successfully implements tooltips and improves accessibility
The changes effectively address the PR objectives by:
- Adding tooltips for buttons on small viewports
- Improving accessibility with aria-labels and roles
- Successfully migrating to the new Button component
| <Button | ||
| id="view-cns-button" | ||
| href={`/facility/${facility.id}/cns`} | ||
| border | ||
| ghost | ||
| onClick={() => navigate(`/facility/${facility.id}/cns`)} | ||
| variant={"outline_primary"} | ||
| className="mt-2 sm:mt-0" | ||
| > | ||
| <CareIcon | ||
| icon="l-monitor-heart-rate" | ||
| className="text-lg" | ||
| /> | ||
| <span>{t("view_cns")}</span> | ||
| </ButtonV2> | ||
| </Button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add accessibility attributes and fix variant syntax
The View CNS button should follow the same accessibility pattern as other buttons in the component.
<Button
id="view-cns-button"
onClick={() => navigate(`/facility/${facility.id}/cns`)}
- variant={"outline_primary"}
+ variant="outline-primary"
className="mt-2 sm:mt-0"
+ aria-label={t("view_cns")}
+ role="button"
>📝 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.
| <Button | |
| id="view-cns-button" | |
| href={`/facility/${facility.id}/cns`} | |
| border | |
| ghost | |
| onClick={() => navigate(`/facility/${facility.id}/cns`)} | |
| variant={"outline_primary"} | |
| className="mt-2 sm:mt-0" | |
| > | |
| <CareIcon | |
| icon="l-monitor-heart-rate" | |
| className="text-lg" | |
| /> | |
| <span>{t("view_cns")}</span> | |
| </ButtonV2> | |
| </Button> | |
| <Button | |
| id="view-cns-button" | |
| onClick={() => navigate(`/facility/${facility.id}/cns`)} | |
| variant="outline-primary" | |
| className="mt-2 sm:mt-0" | |
| aria-label={t("view_cns")} | |
| role="button" | |
| > | |
| <CareIcon | |
| icon="l-monitor-heart-rate" | |
| className="text-lg" | |
| /> | |
| <span>{t("view_cns")}</span> | |
| </Button> |
|
LGTM |
used |
rithviknishad
added
needs testing
and removed
Deploy-Failed
rajku-dev
changed the title
rajku-dev
changed the title
|
@rithviknishad, @Jacobjeevan, If there are any remaining changes to be made here, please let me know. Thank you! |
|
@nihal467 would be doing the QA when it's marked for "needs testing" and once he gives us the "tested" label, we are good to merge |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/components/Facility/FacilityCard.tsx (2)
9-13: Simplify imports by using the index barrel patternConsider consolidating the UI component imports to reduce import statements and improve maintainability.
-import { Button } from "@/components/ui/button"; -import { TooltipComponent, TooltipProvider } from "@/components/ui/tooltip"; +import { Button, TooltipComponent, TooltipProvider } from "@/components/ui";
107-107: Consolidate tooltip styling using Tailwind classesThe tooltip styling appears inconsistent across different tooltips in the file. Consider extracting common styles into a shared class.
-className="text-sm px-3 py-3 max-w-[300px]" +className="tooltip-common-style"Add this to your Tailwind CSS config:
.tooltip-common-style { @apply text-sm px-3 py-3 max-w-[300px]; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Facility/FacilityCard.tsx(4 hunks)
🔇 Additional comments (2)
src/components/Facility/FacilityCard.tsx (2)
128-141: Fix variant syntax and improve accessibility
Previous review comments about fixing variant syntax and improving accessibility are still applicable.
239-307: Verify tooltip behavior on mobile devices
The PR objectives mention adding tooltips for screens < 768px. Let's verify the implementation meets this requirement.
✅ Verification successful
Let me gather more specific information about the tooltip implementation.
Based on the gathered information, I can now provide the final response:
Tooltip implementation correctly follows mobile-first design
The implementation correctly shows tooltips only on small screens (< 768px) and replaces them with inline text on larger screens through:
- Tooltips have
sm:hiddenclass, making them visible only on mobile - Button text has
hidden sm:inlineclass, showing text directly on larger screens - This pattern is consistently applied across all three buttons (notify, view facility, view patients)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check tooltip visibility conditions in the codebase
# Check if tooltips are consistently shown only on small screens
echo "Checking tooltip visibility conditions..."
rg -A 2 "TooltipComponent" | rg "className.*sm:hidden"
# Check if there are any other tooltip implementations that might need similar updates
echo "Checking other tooltip implementations..."
rg -A 5 "tooltip.*className"
Length of output: 718
Script:
#!/bin/bash
# Check the TooltipComponent implementation and its usage
echo "Checking TooltipComponent implementation..."
ast-grep --pattern 'const TooltipComponent = $_'
# Check for sm:hidden and sm:inline patterns in the component
echo "Checking responsive patterns..."
rg "sm:(hidden|inline)" -B 2 -A 2
# Check if there are other similar card components with tooltips
echo "Checking other card components..."
fd "Card\.tsx$" --exec rg -l "TooltipComponent"
Length of output: 12395
| <TooltipProvider> | ||
| <TooltipComponent | ||
| content={t("view_facility")} | ||
| side="top" | ||
| className="sm:hidden text-sm px-3 py-3 max-w-[300px]" | ||
| > | ||
| <Button | ||
| id="facility-details" | ||
| variant={"outline_primary"} | ||
| className="tooltip h-[38px]" | ||
| aria-label={t("view_facility")} | ||
| asChild | ||
| > | ||
| <Link href={`/facility/${facility.id}`}> | ||
| <CareIcon icon="l-hospital" className="text-lg" /> | ||
| <span className="hidden sm:inline"> | ||
| {t("view_facility")} | ||
| </span> | ||
| </Link> | ||
| </Button> | ||
| </TooltipComponent> | ||
| </TooltipProvider> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consistent improvements needed for View Facility and View Patients buttons
Both buttons have similar issues that need to be addressed:
- Remove duplicate tooltip className
- Fix variant syntax
- Use consistent class naming for hidden text
// For both buttons:
-className="sm:hidden text-sm px-3 py-3 max-w-[300px]"
+className="tooltip-common-style"
-variant={"outline_primary"}
+variant="outline-primary"
-className="tooltip h-[38px]"
+className="h-[38px]"
-<span className="hidden sm:inline">
+<span className="hidden sm:inline-block">Also applies to: 283-307
|
LGTM |
|
@rajku-dev 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
UI changes
Summary by CodeRabbit
New Features
Bug Fixes