Replaced ButtonV2's with Button in entire codebase #9736
Conversation
WalkthroughThis pull request introduces a comprehensive refactoring of button components across the codebase, replacing the custom 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 (
|
✅ 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 (30)
src/hooks/useFileManager.tsx (3)
311-320: Consider using a destructive variant for the irreversible “proceed” action.
Archiving a file is irreversible, so it may be clearer to use a “danger” (destructive) variant or styling to emphasize the consequence. Verify with your design team if the “primary” variant is acceptable for destructive actions.- <Button type="submit" variant="primary" disabled={archiving}> + <Button type="submit" variant="danger" disabled={archiving}>
397-403: Remove unused parameter to keep code concise.
The arrow function’s parameter_is not used. Omitting it clarifies that the event object is unnecessary.- onClick={(_) => setArchiveDialogueOpen(null)} + onClick={() => setArchiveDialogueOpen(null)}
444-461: Surface a loading indicator in addition to disabling the button.
You’re already disabling the submit button whileeditingis true, which is great. However, consider showing a small spinner or loading text to improve user feedback when the renaming process might be slow.src/pages/FacilityOrganization/FacilityOrganizationIndex.tsx (2)
65-65: Enhance reusability for this repeated container style
This change effectively ensures that the creation button is centered on smaller screens and right-aligned on medium screens and above. However, notice that the same Tailwind classes (i.e.,"flex justify-center md:justify-end mt-2 mb-4") appear again at line 93. To adhere to the DRY principle, consider extracting these shared container styles into a reusable layout component or a utility variable.
93-93: Maintain consistent layout approach
Similar to line 65, this line ensures a responsive alignment for the creation button. Reusing these shared utility classes across different sections is a good approach, but it might be cleaner to define them in a single place. This helps with maintenance and keeps the layout consistent across the app.src/components/Common/Menu.tsx (1)
104-116: Validate overlapping color variantsThe new variants (e.g.
dangeranddestructive) share the same color token. If that's intentional, documenting the distinction between them will help maintain clarity and consistency across the design system. Otherwise, consider assigning a unique style.src/CAREUI/display/Callout.tsx (1)
18-32: Centralize variant class definitions.
Extracting variant classes into a dictionary is a clean approach; if multiple components need these same variant definitions, consider moving them to a shared theme file to avoid duplication.src/CAREUI/interactive/Zoom.tsx (2)
72-75: Consider using the “icon” size variant
Since this button’s content is purely an icon, using the “icon” size variant (if available) could simplify styling and maintain consistent UI spacing across the application.-<Button - disabled={props.disabled} - variant="ghost" - className="p-2.5 rounded-full" - onClick={ctx.zoomIn} -> +<Button + disabled={props.disabled} + variant="ghost" + size="icon" + onClick={ctx.zoomIn} +>
83-86: Apply the same icon button approach
Similarly consider the “icon” size variant for the zoom-out button to maintain consistency.src/components/Patient/PatientConsentRecordBlock.tsx (1)
91-100: Avoid needing a full page reload
While this logic works fine, relying onwindow.location.reload()could be disruptive to user experience. Consider using local state or re-fetching data to update the UI without a full reload.src/components/ui/button.tsx (2)
7-19: Document new variants
Consider adding short docstrings or comments for each variant inButtonVariantto improve readability and consistency, especially for newer team members.
72-74: Provide a brief description forlabel
Adding a short docstring or TSDoc comment for the optionallabelprop would clarify how it should be used in conjunction with the button’s children.src/components/Users/UserAvatar.tsx (1)
104-119: Tooltip-based disable is user-friendly, but consider accessibility
Wrapping a disabled button in a tooltip is a good UI pattern, but consider ensuring that screen readers announce why it’s disabled (e.g., viaaria-disabledor anaria-describedbyattribute referencing the tooltip content).src/components/Patient/PatientDetailsTab/ResourceRequests.tsx (1)
63-63: Consider specifying a more uniform naming convention for variants.
It might be beneficial to adopt a standardized naming convention for button variants, such asoutlinevs.outline_primary, to make your system more predictable for future changes. You could renameoutline_primaryto something likeoutlinePrimaryor simplyoutlinefor clarity.src/CAREUI/display/PopupModal.tsx (2)
49-51: Ensure consistent labeling or user feedback upon save.
Everything appears correct for the save action. Consider providing immediate feedback or a loading state in the UI if the save operation might be lengthy.
153-155: Add loading or disabled feedback if necessary.
Similar to the mobile view, consider providing the user with a loading indicator or a disabled state if the submit operation involves backend calls.src/components/Form/Form.tsx (1)
151-159: Review the submit workflow.
Overall usage is fine. Consider further enhancements, such as disabling the button once clicked or showing a spinner to indicate submission status if the form submission is asynchronous.src/components/Common/Pagination.tsx (1)
174-185: Consider accessibility for tooltip text on the button.When using the tooltip class with visually hidden or dynamic text, ensure users who rely on screen readers can access this tooltip content. One approach is to include accessible labels/aria attributes.
src/components/Common/UpdatableApp.tsx (1)
139-141: Localize the button text.Currently, the strings
"Updating..."and"Update"do not appear to uset()for localization. Consider localizing these strings for consistency with other text in this file.src/components/Auth/ResetPassword.tsx (1)
177-190: Remove redundant onClick handler for the submit button.Because the button is already of type
"submit"and your form has anonSubmithandler, it is not strictly necessary to callhandleSubmitagain withinonClick(). This minor cleanup can help reduce duplication.<Button variant="primary" type="submit" - onClick={(e) => handleSubmit(e)} > <span>{t("reset")}</span> </Button>src/components/Files/CameraCaptureDialog.tsx (2)
167-186: Retake / Submit Buttons RealignmentUsing the same primary styling for both "Retake" and "Submit" keeps them visually consistent. Consider whether one action might require a different variant (e.g., outline or secondary) based on UX guidelines if “Submit” should stand out more than “Retake.”
233-250: Retake / Submit Buttons for Larger ScreensConsistent usage of
variant="primary"for both retake and submit. Please confirm with design if a secondary variant is desired for retake to keep the emphasis on submit.src/components/Patient/PatientConsentRecords.tsx (1)
7-7: Ensure correct import path.
It seems there's a double slash in@/components//ui/button. Consider removing the extra slash to maintain consistency and avoid potential import resolution issues.- import { Button } from "@/components//ui/button"; + import { Button } from "@/components/ui/button";src/components/Common/ExcelViewer.tsx (1)
81-90: Ensure the anchor text color matches the button's variant.
Inside the<Button>component, you’re including<a className="text-white">that depends on a parent style context. If you switch button variants or theming in the future, the explicit text color in the anchor tag might conflict with the overall Button styling.<Button> - <a href={downloadURL} className="text-white" download={selectedFile.name}> + <a href={downloadURL} download={selectedFile.name}> ... </a> </Button>src/components/Files/FileUpload.tsx (1)
296-319: Consider extracting loading logic into a separate component or HOC.
While the loading logic is functional, it might be cleaner to encapsulate the icon + opacity toggling to keep the code more readable.src/components/Common/FilePreviewDialog.tsx (1)
214-223: Suggest removing the inline color class on anchor tag.
Similar to the pattern in other files, you can rely on the button’s variant to handle text color.<Button variant="primary"> - <a href={downloadURL} className="text-white" download="..."> + <a href={downloadURL} download="..."> ... </a> </Button>src/components/Patient/PatientHome.tsx (2)
Line range hint
164-181: Commented-out block containing the new Button.
If this block is no longer needed, consider removing it to keep the codebase clean.
Line range hint
253-267: Commented-out volunteer assignment code.
Same note as above. If it’s not relevant to this PR, removing it or placing it in a dedicated feature branch might reduce confusion.src/components/Common/AvatarEditModal.tsx (1)
259-259: Consider using the new<Button>component instead of a styled<label>.
In many places, you are styling a<label>like a button. If feasible, this could be converted to an actual<Button>component for consistency and a better user experience. If not, ensure the styles match your design system and that user interactions are handled properly for accessibility.src/components/Files/FileBlock.tsx (1)
Line range hint
124-133: Reevaluate variant choice for archiving.
Using"primary"for an archived file might be slightly confusing since archiving often implies caution or a destructive-like action. Consider"warning"or"destructive"to more clearly convey that it’s a permanent/semi-permanent action.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
public/locale/en.json(1 hunks)src/CAREUI/display/Callout.tsx(2 hunks)src/CAREUI/display/Chip.tsx(0 hunks)src/CAREUI/display/PopupModal.tsx(3 hunks)src/CAREUI/interactive/FiltersSlideover.tsx(1 hunks)src/CAREUI/interactive/Zoom.tsx(2 hunks)src/CAREUI/misc/PaginatedList.tsx(3 hunks)src/Utils/AuthorizeFor.tsx(1 hunks)src/components/Auth/ResetPassword.tsx(2 hunks)src/components/Common/AuthorizedButton.tsx(1 hunks)src/components/Common/AvatarEditModal.tsx(6 hunks)src/components/Common/ButtonV2.tsx(0 hunks)src/components/Common/ConfirmDialog.tsx(2 hunks)src/components/Common/ExcelFIleDragAndDrop.tsx(4 hunks)src/components/Common/ExcelViewer.tsx(4 hunks)src/components/Common/FilePreviewDialog.tsx(9 hunks)src/components/Common/Menu.tsx(3 hunks)src/components/Common/Pagination.tsx(2 hunks)src/components/Common/UpdatableApp.tsx(2 hunks)src/components/Facility/FacilityHome.tsx(1 hunks)src/components/Files/CameraCaptureDialog.tsx(4 hunks)src/components/Files/FileBlock.tsx(3 hunks)src/components/Files/FileUpload.tsx(2 hunks)src/components/Form/Form.tsx(2 hunks)src/components/Patient/PatientConsentRecordBlock.tsx(2 hunks)src/components/Patient/PatientConsentRecords.tsx(4 hunks)src/components/Patient/PatientDetailsTab/PatientUsers.tsx(1 hunks)src/components/Patient/PatientDetailsTab/ResourceRequests.tsx(1 hunks)src/components/Patient/PatientDetailsTab/patientUpdates.tsx(1 hunks)src/components/Patient/PatientHome.tsx(4 hunks)src/components/Patient/PatientIndex.tsx(1 hunks)src/components/Resource/ResourceCommentSection.tsx(2 hunks)src/components/Resource/ResourceCreate.tsx(2 hunks)src/components/Resource/ResourceDetailsUpdate.tsx(2 hunks)src/components/Resource/ResourceList.tsx(2 hunks)src/components/Users/ConfirmFacilityModal.tsx(1 hunks)src/components/Users/UnlinkFacilityDialog.tsx(1 hunks)src/components/Users/UserAvatar.tsx(2 hunks)src/components/Users/UserDeleteDialog.tsx(1 hunks)src/components/Users/UserResetPassword.tsx(2 hunks)src/components/Users/UserSummary.tsx(2 hunks)src/components/ui/button.tsx(3 hunks)src/hooks/useFileManager.tsx(4 hunks)src/pages/Encounters/tabs/EncounterFeedTab.tsx(2 hunks)src/pages/FacilityOrganization/FacilityOrganizationIndex.tsx(2 hunks)
💤 Files with no reviewable changes (2)
- src/CAREUI/display/Chip.tsx
- src/components/Common/ButtonV2.tsx
✅ Files skipped from review due to trivial changes (5)
- src/components/Patient/PatientIndex.tsx
- src/components/Users/ConfirmFacilityModal.tsx
- src/components/Users/UserDeleteDialog.tsx
- src/components/Users/UnlinkFacilityDialog.tsx
- src/pages/Encounters/tabs/EncounterFeedTab.tsx
🧰 Additional context used
📓 Learnings (3)
src/components/Patient/PatientDetailsTab/ResourceRequests.tsx (1)
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#9327
File: src/components/Resource/ResourceDetails.tsx:88-94
Timestamp: 2024-12-12T19:27:20.394Z
Learning: Prefer using the primary `Button` component with the `variant="primary"` prop instead of `ButtonV2` for consistency across the application.
src/components/Users/UserSummary.tsx (1)
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#9080
File: src/components/Users/UserSummary.tsx:53-71
Timestamp: 2024-11-22T12:04:39.044Z
Learning: In the `UserSummaryTab` component (`src/components/Users/UserSummary.tsx`), when there is an error during user deletion, the delete dialog is intentionally closed to prevent the user from immediately trying to delete the user again.
src/components/Resource/ResourceCommentSection.tsx (1)
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#9327
File: src/components/Resource/ResourceDetails.tsx:88-94
Timestamp: 2024-12-12T19:27:20.394Z
Learning: Prefer using the primary `Button` component with the `variant="primary"` prop instead of `ButtonV2` for consistency across the application.
🔇 Additional comments (66)
src/components/Facility/FacilityHome.tsx (1)
195-195: Consistent use of thedestructivevariant.Switching from
"danger"to"destructive"aligns with the new design system’s semantics. Please verify that all corresponding styles are appropriately applied throughout the codebase.src/hooks/useFileManager.tsx (2)
2-2: Consider verifying translation key availability in i18n files.
Importingtfromi18nextis a good approach for localization. However, ensure that all new translation keys (e.g.,"cancel","proceed") are present in the translation files and appropriately localized.
7-8: Use of standard Button component is well-aligned with the PR objectives.
Replacing custom button implementations with the newButtoncomponent helps maintain consistency across the application. Good job!src/components/Common/Menu.tsx (3)
52-52: Maintain consistent icon sizingThis condition leverages
"xs"to set a smaller text size, which is fine. However, ensure it remains consistent with the rest of your Tailwind config (e.g., if you override base text sizes somewhere else).
122-122: No functional changes detectedThis added blank line does not introduce any functional changes.
6-6: Aligned import statement with newButtoncomponentThis change aligns with the PR objective of replacing
ButtonV2references and is consistent with the updated imports throughout the codebase.Run this script to ensure references to
ButtonV2have been fully removed from the codebase:✅ Verification successful
Migration to new Button component is complete
The verification results show that:
- No references to
ButtonV2exist in the codebase (case-insensitive search)- All Button imports are consistently using the new path
@/components/ui/button- The import in Menu.tsx (
ButtonSize, ButtonVariant) aligns with the new button component's types🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for any remaining references to ButtonV2 rg 'ButtonV2'Length of output: 13
Script:
#!/bin/bash # Let's verify the migration from ButtonV2 to Button using multiple approaches # 1. Check for any files containing ButtonV2 (case-insensitive) rg -i "buttonv2" # 2. Check for Button-related imports to understand the migration pattern rg "import.*Button.*from" # 3. Look for Button type/interface definitions ast-grep --pattern 'interface Button$_' ast-grep --pattern 'type Button$_' # 4. Check specifically in the components directory fd . src/components -t f -e tsx -e ts --exec rg -l "Button"Length of output: 17001
src/components/Common/AuthorizedButton.tsx (2)
9-11: Acknowledge the effective type composition.
UsingOmit<AuthorizedElementProps, "authorizeFor"> & ButtonPropsis a neat way to ensure thatauthorizeForis required while retaining all otherButtonProps.
13-23: Validate the disabled state logic.
Thedisabledprop here is overridden by the negation ofisAuthorized. This can be appropriate if the button should never be enabled for unauthorized users, but confirm that there's no scenario where you might want fine-grained control over the disabled state independent ofisAuthorized.src/components/Common/ConfirmDialog.tsx (2)
1-1: Check usage of ButtonVariant.
Now thatButtonVariantis imported from the new@/components/ui/button, ensure that other references to button variants in the existing codebase are consistently updated to avoid confusion.
34-45: Form submission consideration when using type="submit".
If this dialog is rendered within a<form>, a click on the confirm button will trigger a form submission, potentially conflicting with custom logic. Verify this behavior is intentional. If you only need a user acknowledgment, consider usingtype="button.src/Utils/AuthorizeFor.tsx (1)
22-27: Confirm consistency of documentation example.
Your example for<AuthorizedButton />references(role) => !role.includes('ReadOnly')andAuthorizeFor.Admins. Confirm these are valid patterns or define them in your doc for clarity.src/CAREUI/display/Callout.tsx (1)
52-59: Ensure visual consistency between container and badge.
The container usesvariantClasses[variant], and the badge usesbadgeVariantClasses[variant]. Verify that the chosen color pairings for each variant (e.g., “destructive” outer with “destructive” badge) are aligned with your design system.src/CAREUI/interactive/Zoom.tsx (1)
5-5: Confirm design consistency and variant usage
Using theghostvariant from the newButtoncomponent here is valid, but ensure that this variant aligns with the overall design system.Would you like a verification script to scan for other potential variant mismatches in the codebase?
src/components/ui/button.tsx (1)
46-49: Review the color palette for “warning” and “alert”
Double-check that these color classes meet accessibility (contrast) requirements and align with your design guidelines.src/components/Users/UserAvatar.tsx (1)
120-127: Good usage of the new Button
The direct usage ofvariant="white"here is consistent with the file’s design. No further issues observed.src/CAREUI/display/PopupModal.tsx (2)
45-47: Button usage looks good.
Switching to the newButtoncomponent withvariant="outline"is consistent with your UI library’s approach. No issues detected here.
149-151: Good use of the new Button.
This is consistent with the usage seen in the mobile view. Looks good.src/CAREUI/misc/PaginatedList.tsx (1)
Line range hint
126-137: Refactored to the new Button looks great.
The code properly handles disabled states and visually indicates loading with a spinning icon. This is a good pattern for asynchronous operations.src/components/Form/Form.tsx (1)
146-148: Good move to the new Button.
Replacing the oldButtonV2withButtonandvariant="outline"is aligned with your refactor objective.src/components/Patient/PatientDetailsTab/patientUpdates.tsx (1)
27-30: No concerns regarding the Button variant or icon usage.The transition to
Buttonwith theoutline_primaryvariant and the localized text plus icon is properly implemented. This is consistent with the new styling conventions and supports localization throught(...). Nice work!src/components/Common/Pagination.tsx (1)
5-5: Import transition to new Button is straightforward.The import statement cleanly replaces the old button reference with the new
Button, aligning with the PR's objective.src/components/Auth/ResetPassword.tsx (1)
5-6: The new Button import is correctly applied.You have successfully replaced the old component with the new
Button, following the PR’s objective.src/components/Users/UserResetPassword.tsx (2)
6-7: Import Replacement Looks GoodReplacing
ButtonV2withButtonfrom@/components/ui/buttonis consistent with the PR objective of standardizing button usage.
195-204: Consistent Variant UsageUsing
variant="outline_primary"aligns with the rest of the app’s design system. Just verify that this variant is available and styled as expected in the newButtoncomponent.src/components/Files/CameraCaptureDialog.tsx (7)
7-8: Import Statement Updated SuccessfullyThe import from
@/components/ui/buttonis in line with the overall replacement ofButtonV2. No issues noted.
138-144: Improved Clarity for Camera Switch ButtonUsing
variant="primary"for the switch camera button maintains visual consistency for a primary action on small screens.
153-161: Capture Button TransitionThis update correctly replaces the old button with the new
Buttoncomponent and a clear label. Functionality remains intact.
Line range hint
192-202: Close Button Outline VariantApplying
variant="outline"for the close button is a good choice to differentiate from primary actions.
208-211: Laptop Switch Camera Button CheckEnsure the camera-switch logic remains consistent and that it’s not hidden when it’s needed. Currently, it’s guarded by
isLaptopScreen. Verify that users on certain screen sizes can still access this feature.
219-227: Capture Action IconThe inclusion of the capture icon via
<CareIcon icon="l-capture" />is an effective visual cue. The code looks correct.
256-265: Close Button with Extended LabelUsing
${t("close")} ${t("camera")}as a combined label is fine if it aligns with the localized user interface guidelines.src/components/Resource/ResourceDetailsUpdate.tsx (3)
1-1: Translation Import CheckBringing in
tfromi18nextsupports localized strings. No issues noted.
7-8: Button Import Aligned with Project GoalsSwitching from
ButtonV2to the newButtonis in line with the PR objective. Looks good.
285-290: Cancel / Submit Button ReplacementThe replacement is correctly adopting
variant="outline"for “cancel” andvariant="primary"for “submit.” This approach preserves a clear visual distinction between these actions.src/components/Common/ExcelFIleDragAndDrop.tsx (3)
7-8: Button Import Successfully IntegratedThe import of
Buttonfrom@/components/ui/buttonaligns with the PR objective of removingButtonV2.
Line range hint
252-265: Close Button ImplementationReplacing a dedicated “Cancel” component with the new
Buttoninoutlinevariant is consistent with the codebase transition.
Line range hint
266-283: Submit Button EnhancementUsing a spinner icon for the loading state and a clear “Import” label clarifies user flow. Ensuring disabled state when no file is selected is a nice usability touch.
src/components/Patient/PatientConsentRecords.tsx (3)
2-2: Import usage confirmed.
Bringing inLoader2fromlucide-reactis consistent with similar references across the codebase and appears correct.
130-130: Destructive variant usage.
Thevariant="destructive"property correctly conveys the high-impact nature of the action. This is consistent with good UX, aligning the color and styling with destructive intents.
Line range hint
181-219: Refine file upload button block.
- Good job setting
variant="primary"for the upload button andvariant="destructive"for the delete button to distinguish actions clearly.- The condition
fileUpload.uploading || (newConsent.type === 2 && newConsent.patient_code_status === 0)elegantly disables the button during uploads or invalid status; consider adding a quick inline comment clarifying the significant use case for clarity.- Great implementation of spinner feedback (
Loader2) when uploading.Overall, the logic is consistent with best practices.
src/components/Patient/PatientDetailsTab/PatientUsers.tsx (1)
104-104: Use of "outline_primary" variant.
Theoutline_primaryvariant is consistent with a visually distinctive style while still appearing less forceful than a filled button. Ensure that theming is well-documented and consistent across other components.src/components/Resource/ResourceCreate.tsx (2)
9-9: Updated import path for Button.
This change aligns with the standardizedButtonusage across the codebase.
320-325: Replaced custom Cancel/Submit with standardized Button.
Replacing the custom components with a standardizedButtonpromotes consistency. Both the “Cancel” and “Submit” actions are clearly defined by their respective variants (outlineandprimary). This is correct and intuitive.src/components/Resource/ResourceList.tsx (3)
7-7: Consolidated imports for Badge and Button.
Importing bothBadgeandButtonfrom the appropriateuifolder is consistent with your approach to unify UI components.
93-103: Using secondary variant for "TRANSPORTATION TO BE ARRANGED".
The approach helps visually distinguish the special status. The icon and uppercase label provide clear user feedback.
108-124: Dynamic variant for "APPROVED" status.
Switching betweenprimaryorsecondaryfor theBadgebased on the resource status is a neat way to highlight approvals differently. The color-coded approach (e.g.,bg-sky-200for “APPROVED”) is user-friendly.src/components/Common/ExcelViewer.tsx (2)
6-7: Good import changes.
The import ofButtonfrom@/components/ui/buttonlooks correct.
266-268: Well-structured button usage.
The “Close” and “Import” buttons both handle localized text. This usage is consistent with the new button structure. Good job verifying the type (type="button"vstype="submit").Also applies to: 269-281
src/components/Files/FileUpload.tsx (1)
8-9: Successful import of the new Button.
This aligns with the goal of replacingButtonV2withButton.src/components/Common/FilePreviewDialog.tsx (6)
17-18: Import path changes look solid.
Using the newButtonfrom@/components/ui/buttonand updating theFileUploadModelpath improves consistency across the project.Also applies to: 22-22
225-227: Consistent usage of “close” button.
Localizing the label and usingtype="button"is correct.
Line range hint
232-243: Keyboard accessibility is good.
ProvidingArrowLeftinonKeyDownis a nice detail for keyboard navigation.
Line range hint
285-296: Next file button for multi-file preview.
Logic and constraints look correct.
Line range hint
337-351: Zooming feature is well-structured.
The approach for disabling the zoom control when reaching bounds is properly handled.
Line range hint
372-386: Page navigation for PDF is well-handled.
Incrementing/decrementing page numbers with boundary checks ensures no out-of-range errors.src/components/Common/AvatarEditModal.tsx (4)
13-13: Well done replacing the import fromButtonV2toButton.
This aligns with the new design system and helps maintain consistency across the codebase.
294-295: Good use of thedestructivevariant for a delete action.
This is consistent with typical design conventions for permanent or potentially harmful actions.
355-360: Camera buttons look consistent with the rest of the UI.
Switching and capturing events appear well-handled. Thanks for keeping the code clean and straightforward.
399-401: Smart approach resetting preview and camera states upon closing.
Everything seems to be correctly cleaned up, including the camera reference. Great job.src/components/Resource/ResourceCommentSection.tsx (2)
5-6: Good transition to the newButtoncomponent.
This improves UI consistency with the rest of the application.
55-63: Button usage is straightforward and correct.
The asynchronous submit and subsequent refetch logic is properly handled within the onClick callback.src/components/Files/FileBlock.tsx (3)
6-7: Nice job removingButtonV2in favor ofButton.
This keeps the codebase consistent with the new standard.
94-110: Button actions for viewing and downloading are clear.
Using thesecondaryvariant for these neutral actions makes sense.
114-121: Renaming functionality meets the design goals.
It’s good to see the button remain consistent with your secondary variant.src/components/Users/UserSummary.tsx (2)
7-7: LGTM! Import changes align with PR objectives.The replacement of ButtonV2 with AuthorizedButton maintains the required functionality while adding proper authorization checks.
175-185: LGTM! Button replacement enhances security.The AuthorizedButton replacement:
- Maintains the delete functionality
- Adds proper authorization checks via
authorizeFor- Updates the variant from "danger" to "destructive" following the new button component's API
Mahendar0701
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/CAREUI/display/Callout.tsx(1 hunks)src/CAREUI/misc/PaginatedList.tsx(3 hunks)src/components/Common/ConfirmDialog.tsx(2 hunks)src/components/Common/Menu.tsx(1 hunks)src/components/Patient/PatientDetailsTab/EncounterHistory.tsx(1 hunks)src/components/Users/UserAvatar.tsx(2 hunks)src/components/Users/UserResetPassword.tsx(2 hunks)src/components/Users/UserSummary.tsx(3 hunks)src/components/ui/button.tsx(1 hunks)src/hooks/useFileManager.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/components/ui/button.tsx
- src/components/Users/UserResetPassword.tsx
- src/components/Common/ConfirmDialog.tsx
- src/hooks/useFileManager.tsx
- src/CAREUI/misc/PaginatedList.tsx
- src/components/Users/UserSummary.tsx
- src/components/Users/UserAvatar.tsx
🔇 Additional comments (2)
src/components/Patient/PatientDetailsTab/EncounterHistory.tsx (1)
27-27: Removal of the unused_parameter is appropriate.Eliminating unused parameters cleans up the code and prevents confusion. This is a good improvement to maintain code clarity and readability.
src/CAREUI/display/Callout.tsx (1)
6-6: Good job defining fresh variants for Callout.This change clearly separates the Callout’s look-and-feel from button styling concerns. It aligns with the feedback suggesting not to reuse the button's variant definitions.
|
@rithviknishad I have added |
yup, that should be fine |
I should keep alert and warning variants in button.tsx right? 👀 |
yup |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/components/Common/Menu.tsx (1)
Remove unused button-related classes from CAREUI.css
The verification shows that:
- CAREUI.css still contains multiple button-related classes (button-primary, button-secondary, etc.)
- These button classes are not being used anywhere in the codebase
- The only ButtonV2 reference in AuthorizeFor.tsx is in the documentation/comments
- No other components are using these button styles
You can safely remove the following classes from CAREUI.css:
- All
a.button-*classes (primary, secondary, danger, warning, alert variants)- All
button.button-*classes (primary, secondary, danger, warning, alert variants)- Keep the
.cui-form-button-groupclass as it might still be in use🔗 Analysis chain
Line range hint
1-1: Verify and remove button-related classes from CAREUI.css.As mentioned in the review comments, we need to remove the button-related classes from CAREUI.css that were originally part of ButtonV2's implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for button-related classes in CAREUI.css echo "Checking for button-related classes in CAREUI.css..." rg -A 2 'button-' src/style/CAREUI.css # Check for any remaining usages of these classes echo "Checking for any remaining usages of these classes..." rg -l 'button-(size|variant)-' -g '!CAREUI.css'Length of output: 1682
Script:
#!/bin/bash # Let's check for any remaining usages of the button-related classes found in CAREUI.css echo "Checking for usages of button classes..." rg -l 'button-(primary|secondary|danger|warning|alert)-(default|ghost)' -g '!CAREUI.css' # Also check for any imports or references to ButtonV2 echo "Checking for ButtonV2 references..." rg -l 'ButtonV2'Length of output: 306
Script:
#!/bin/bash # Let's check the content of AuthorizeFor.tsx to see how ButtonV2 is used echo "Checking ButtonV2 usage in AuthorizeFor.tsx..." rg -A 5 'ButtonV2' src/Utils/AuthorizeFor.tsx # Let's also check if there are any other button-related styles or classes in use echo "Checking for other button styles..." rg 'class(Name)?="[^"]*button[^"]*"' -g '!CAREUI.css'Length of output: 682
🧹 Nitpick comments (8)
src/components/Patient/PatientConsentRecords.tsx (2)
Line range hint
181-211: Consider simplifying the disabled state logicThe button implementation looks good overall with proper loading states and visual feedback. However, the disabled state condition could be simplified.
Consider this more readable version:
- disabled={ - fileUpload.uploading || - (newConsent.type === 2 && - newConsent.patient_code_status === 0) - } + disabled={ + fileUpload.uploading || + (newConsent.type === 2 && !newConsent.patient_code_status) + }
Line range hint
224-228: Simplify file chooser button stylingThe current className implementation is verbose and potentially includes redundant utility classes that might already be covered by buttonVariants.
Consider simplifying to:
- className={`${buttonVariants({ variant: "primary", size: "default" })} inline-flex h-min w-full cursor-pointer items-center justify-center gap-2 whitespace-pre font-medium outline-offset-1 transition-all duration-200 ease-in-out`} + className={`${buttonVariants({ variant: "primary" })} w-full cursor-pointer gap-2`}The buttonVariants helper likely already includes most of the utility classes you've specified.
src/components/Common/AvatarEditModal.tsx (3)
257-275: Consider restructuring the file input implementationWhile the button works, having the file input nested inside the button could lead to click handling issues. Consider moving the file input outside the button for better maintainability.
+ <input + id="file-input" + title="changeFile" + type="file" + accept="image/*" + className="hidden" + onChange={onSelectFile} + /> <Button id="upload-cover-image" variant="primary" type="button" onClick={() => document.getElementById("file-input")?.click() } > <CareIcon icon="l-cloud-upload" className="text-lg" /> {t("upload_an_image")} - <input - id="file-input" - title="changeFile" - type="file" - accept="image/*" - className="hidden" - onChange={onSelectFile} - /> </Button>
287-297: Consider removing unnecessary event propagation stopThe
e.stopPropagation()call might be unnecessary since the button is not within a parent click handler that would conflict with it.<Button variant="outline" onClick={(e) => { - e.stopPropagation(); closeModal(); dragProps.setFileDropError(""); }} disabled={isProcessing} > {t("cancel")} </Button>
Line range hint
307-324: Consider using primary variant for the save buttonSince saving is a primary action, consider using the
primaryvariant instead ofoutlineto maintain consistency with UI patterns.<Button id="save-cover-image" - variant="outline" + variant="primary" onClick={uploadAvatar} disabled={isProcessing || !selectedFile} >src/components/Common/Menu.tsx (3)
24-26: Enhance the deprecation notice with migration guidance.While the deprecation notice is good, it would be more helpful to include:
- The target version for removal
- A link to migration documentation
- Reference to the replacement component
/** - * @deprecated This component will be replaced with ShadCN's dropdown. + * @deprecated This component will be replaced with ShadCN's dropdown menu component. + * @see https://ui.shadcn.com/docs/components/dropdown-menu + * @todo Remove in next major version */
37-43: Improve className handling and button styling.A few suggestions to enhance the implementation:
- Use
classNamesutility for better className composition- Consider making the height configurable
- Use type-safe buttonVariants
- className={`inline-flex w-full cursor-pointer items-center justify-center gap-2 font-medium outline-offset-1 transition-all duration-200 ease-in-out disabled:cursor-not-allowed disabled:bg-secondary-200 disabled:text-secondary-500 lg:justify-between ${props.className} ${buttonVariants({ variant: "primary", size: "default" })}`} + className={classNames( + buttonVariants({ variant: "primary", size: "default" }), + "inline-flex w-full cursor-pointer items-center justify-center gap-2", + "font-medium outline-offset-1 transition-all duration-200", + "disabled:cursor-not-allowed disabled:bg-secondary-200 disabled:text-secondary-500", + "lg:justify-between", + props.className + )} > - <div className="flex items-center gap-2 whitespace-nowrap h-6"> + <div className="flex items-center gap-2 whitespace-nowrap min-h-[1.5rem]">
83-88: Remove hardcoded styles in preparation for component replacement.Since this component will be replaced with shadcn's dropdown menu:
- Remove the
dropdown-item-primaryclass from CAREUI.css- Consider using Tailwind classes directly for flexibility
- `dropdown-item-primary`, + "hover:bg-secondary-100 active:bg-secondary-200", isAuthorized ? "pointer-events-auto cursor-pointer" : "!hidden", className, )} > - <i className="text-lg text-primary-500">{icon}</i> + <i className="text-lg text-secondary-700">{icon}</i>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
public/locale/en.json(1 hunks)src/CAREUI/display/PopupModal.tsx(0 hunks)src/components/Common/AvatarEditModal.tsx(5 hunks)src/components/Common/ConfirmDialog.tsx(2 hunks)src/components/Common/Menu.tsx(4 hunks)src/components/Facility/FacilityHome.tsx(1 hunks)src/components/Patient/PatientConsentRecords.tsx(4 hunks)src/components/ui/button.tsx(2 hunks)src/style/CAREUI.css(0 hunks)
💤 Files with no reviewable changes (2)
- src/style/CAREUI.css
- src/CAREUI/display/PopupModal.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- public/locale/en.json
- src/components/Facility/FacilityHome.tsx
- src/components/Common/ConfirmDialog.tsx
- src/components/ui/button.tsx
🔇 Additional comments (7)
src/components/Patient/PatientConsentRecords.tsx (3)
2-2: LGTM: Import changes align with PR objectivesThe replacement of ButtonV2 with Button/buttonVariants and addition of Loader2 for loading states improves the component's functionality.
Also applies to: 7-7
130-130: LGTM: Appropriate variant name changeThe change from "danger" to "destructive" better reflects the semantic meaning of the action and aligns with the new Button component's variant naming convention.
212-219: LGTM: Clear files button implementationThe clear files button correctly uses the destructive variant and properly handles the disabled state during upload.
src/components/Common/AvatarEditModal.tsx (4)
13-13: LGTM! Import of new Button componentThe import statement correctly references the new standardized Button component.
277-285: LGTM! Camera button implementationThe button implementation is clean and correctly handles camera initialization.
299-305: LGTM! Delete button implementationThe button correctly uses the destructive variant and has proper conditional rendering.
Line range hint
360-415: LGTM! Camera control buttons implementationThe camera control buttons are well-implemented with appropriate variants, loading states, and translations.
@rithviknishad Done |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/Common/AvatarEditModal.tsx (2)
257-272: Enhance accessibility of file upload buttonWhile the implementation follows the asChild approach, it can be improved for better accessibility.
Apply these changes to establish an explicit label-input association:
<Button id="upload-cover-image" variant="primary" asChild> - <label className="cursor-pointer"> + <label htmlFor="avatar-file-input" className="cursor-pointer"> <CareIcon icon="l-cloud-upload" className="text-lg mr-1" /> {t("upload_an_image")} <input + id="avatar-file-input" title="changeFile" type="file" accept="image/*" className="hidden" onChange={onSelectFile} /> </label> </Button>
Line range hint
357-397: Implement consistent loading states for camera operationsThe submit button correctly shows a loading state, but other camera operation buttons could benefit from similar treatment to prevent multiple clicks during processing.
Consider adding loading states to camera operation buttons:
- <Button variant="primary" onClick={handleSwitchCamera}> + <Button + variant="primary" + onClick={handleSwitchCamera} + disabled={isProcessing} + > <CareIcon icon="l-camera-change" className="text-lg" /> {`${t("switch")} ${t("camera")}`} </Button> <Button variant="primary" + disabled={isProcessing} onClick={() => { captureImage(); }} > <CareIcon icon="l-capture" className="text-lg" /> {t("capture")} </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/CAREUI/interactive/FiltersSlideover.tsx(1 hunks)src/components/Common/AvatarEditModal.tsx(5 hunks)src/components/Patient/PatientDetailsTab/patientUpdates.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/CAREUI/interactive/FiltersSlideover.tsx
- src/components/Patient/PatientDetailsTab/patientUpdates.tsx
🔇 Additional comments (2)
src/components/Common/AvatarEditModal.tsx (2)
13-14: LGTM: Import change aligns with PR objectiveThe Button import change correctly supports the migration from ButtonV2.
Line range hint
284-321: LGTM: Well-implemented action buttonsThe action buttons (save, cancel, delete) are well implemented with:
- Appropriate variant usage
- Proper loading states and disabled conditions
- Consistent icon usage and translations
|
@Mahendar0701 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! 🙌 |
|
Hey @rithviknishad i want to work on further cleaning up of code base. We have 10 deprecated utilities such as Notification.Error, useTanStackQueryInstead, CarePatientTokenKey and others. Please assign some of them to me if needed. |
|
go ahead and handle notification first 🚀 |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Release Notes
Localization
UI Components
ButtonV2with a newButtoncomponent across multiple componentswarningandalertButton Variants
dangertodestructiveLayout Improvements
Deprecation
ButtonV2component and associated type definitionsChipcomponent in favor ofBadge