Fix: Layout update of Asset Page #9466
Conversation
WalkthroughThe pull request updates the Changes
Assessment against linked issues
Possibly related PRs
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: 1
🧹 Nitpick comments (2)
src/components/Assets/AssetsList.tsx (2)
434-448: Consider utilizing multi-field search capabilityWhile the implementation is functional, the
SearchByMultipleFieldscomponent is currently used with only one search field despite its capability to handle multiple fields.Consider adding more search fields to enhance the search functionality:
<SearchByMultipleFields id="asset-search" options={[ { key: "Name/ Serial no./ QR code ID", label: "name/serial no./QR code ID", type: "text" as const, placeholder: "Search by name/serial no./QR code ID", value: qParams.search || "", shortcutKey: "f", }, + { + key: "location", + label: "Location", + type: "text" as const, + placeholder: "Search by location", + value: qParams.location || "", + }, + { + key: "status", + label: "Status", + type: "select" as const, + placeholder: "Filter by status", + value: qParams.status || "", + options: [ + { label: "Working", value: "working" }, + { label: "Not Working", value: "not_working" }, + ], + }, ]} className="w-full" onSearch={(key, value) => updateQuery({ [key]: value })} clearSearch={clearSearch} />
Line range hint
543-595: Enhance warrantyAmcValidityChip implementationThe function logic is sound, but could benefit from some improvements.
Consider these enhancements:
+const WARRANTY_EXPIRY_THRESHOLDS = { + CRITICAL: 30, + WARNING: 90, +} as const; + +const getDaysDifference = (date1: Date, date2: Date) => + Math.ceil(Math.abs(Number(date1) - Number(date2)) / (1000 * 60 * 60 * 24)); + export const warrantyAmcValidityChip = ( warranty_amc_end_of_validity: string, ) => { if (warranty_amc_end_of_validity === "" || !warranty_amc_end_of_validity) return; const today = new Date(); const warrantyAmcEndDate = new Date(warranty_amc_end_of_validity); - const days = Math.ceil( - Math.abs(Number(warrantyAmcEndDate) - Number(today)) / - (1000 * 60 * 60 * 24), - ); + const days = getDaysDifference(today, warrantyAmcEndDate); if (warrantyAmcEndDate < today) { return ( <Chip id="warranty-amc-expired-red" variant="danger" startIcon="l-times-circle" text="AMC/Warranty Expired" /> ); - } else if (days <= 30) { + } else if (days <= WARRANTY_EXPIRY_THRESHOLDS.CRITICAL) { return ( <Chip id="warranty-amc-expiring-soon-orange" variant="custom" className="border-orange-300 bg-orange-100 text-orange-900" startIcon="l-exclamation-circle" text="AMC/Warranty Expiring Soon" /> ); - } else if (days <= 90) { + } else if (days <= WARRANTY_EXPIRY_THRESHOLDS.WARNING) { return ( <Chip id="warranty-amc-expiring-soon-yellow" variant="warning" startIcon="l-exclamation-triangle" text="AMC/Warranty Expiring Soon" /> ); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Assets/AssetsList.tsx(4 hunks)
🔇 Additional comments (2)
src/components/Assets/AssetsList.tsx (2)
Line range hint 30-42: LGTM: Clean import additions and hook update
The new imports and hook updates are well-structured and maintain consistency with the codebase.
Line range hint 359-423: LGTM: Well-structured filter and export functionality
The advanced filter and import/export menu implementation is clean, with proper authorization checks and comprehensive export options.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/components/Assets/AssetsList.tsx (1)
Line range hint
533-589: Handle edge cases in warranty validationThe warranty validation logic should handle invalid date formats and consider timezone implications.
export const warrantyAmcValidityChip = ( warranty_amc_end_of_validity: string, ) => { if (warranty_amc_end_of_validity === "" || !warranty_amc_end_of_validity) return; + + // Validate date format + const warrantyAmcEndDate = new Date(warranty_amc_end_of_validity); + if (isNaN(warrantyAmcEndDate.getTime())) { + console.error(`Invalid warranty date format: ${warranty_amc_end_of_validity}`); + return; + } + const today = new Date(); - const warrantyAmcEndDate = new Date(warranty_amc_end_of_validity); + // Set time to start of day to avoid timezone issues + today.setHours(0, 0, 0, 0); + warrantyAmcEndDate.setHours(0, 0, 0, 0); const days = Math.ceil( Math.abs(Number(warrantyAmcEndDate) - Number(today)) / (1000 * 60 * 60 * 24), );
🧹 Nitpick comments (1)
src/components/Assets/AssetsList.tsx (1)
431-445: Consider expanding search capabilitiesWhile the SearchByMultipleFields component is a good addition, consider adding more commonly searched fields to improve user experience.
options={[ { key: "Name/ Serial no./ QR code ID", label: "name/serial no./QR code ID", type: "text" as const, placeholder: "Search by name/serial no./QR code ID", value: qParams.search || "", shortcutKey: "f", }, + { + key: "Asset Class", + label: "asset class", + type: "text" as const, + placeholder: "Search by asset class", + value: qParams.asset_class || "", + }, + { + key: "Status", + label: "status", + type: "text" as const, + placeholder: "Search by status", + value: qParams.status || "", + }, ]}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Assets/AssetsList.tsx(4 hunks)
🔇 Additional comments (2)
src/components/Assets/AssetsList.tsx (2)
30-31: LGTM: Clean import declarations
The new imports are properly organized and follow the project's import structure.
Line range hint 322-420: Simplify nested layout structure
The button layout still has unnecessary nesting that can be simplified for better maintainability.
Consider this simplified structure:
-<div className="flex flex-wrap items-center gap-3">
- <div className="mb-2 flex w-full flex-col items-center gap-3 lg:mb-0 lg:w-fit lg:flex-row lg:gap-5">
- <div className="w-full lg:w-fit">
+<div className="flex flex-wrap items-center gap-3">
+ <div className="flex w-full flex-wrap items-center gap-3 lg:w-fit">
<Button
variant="primary"
size="lg"
className="w-full p-[10px] md:w-auto"
onClick={() => setIsScannerActive(true)}
>
<CareIcon icon="l-search" className="text-base mr-2" />
Scan Asset QR
</Button>
- </div>
- <div className="w-full lg:w-fit" data-testid="create-asset-button">
<Button
variant="primary"
size="lg"
disabled={!NonReadOnlyUsers}
className="w-full p-[10px] md:w-auto"
+ data-testid="create-asset-button"
onClick={() => {
if (qParams.facility) {
navigate(`/facility/${qParams.facility}/assets/new`);
} else {
setShowFacilityDialog(true);
}
}}
>
<CareIcon icon="l-plus-circle" className="text-lg mr-2" />
<span>{t("create_asset")}</span>
</Button>
- </div>
</div>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/Assets/AssetsList.tsx (2)
432-446: LGTM: Well-implemented search functionality with accessibilityThe SearchByMultipleFields implementation is solid with proper configuration and accessibility support via shortcut key.
Consider adding an aria-label to improve screen reader support:
<SearchByMultipleFields id="asset-search" + aria-label="Search assets by name, serial number, or QR code ID" options={[
Line range hint
533-589: LGTM: Well-implemented warranty validity status logicThe warrantyAmcValidityChip function provides clear visual feedback with proper date handling.
Consider memoizing the chip components to prevent unnecessary re-renders:
+const ExpiredChip = memo(() => ( + <Chip + id="warranty-amc-expired-red" + variant="danger" + startIcon="l-times-circle" + text="AMC/Warranty Expired" + /> +)); export const warrantyAmcValidityChip = ( warranty_amc_end_of_validity: string, ) => { if (warranty_amc_end_of_validity === "" || !warranty_amc_end_of_validity) return; const today = new Date(); const warrantyAmcEndDate = new Date(warranty_amc_end_of_validity); const days = Math.ceil( Math.abs(Number(warrantyAmcEndDate) - Number(today)) / (1000 * 60 * 60 * 24), ); if (warrantyAmcEndDate < today) { - return ( - <Chip - id="warranty-amc-expired-red" - variant="danger" - startIcon="l-times-circle" - text="AMC/Warranty Expired" - /> - ); + return <ExpiredChip />; } // Similar changes for other chips...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cypress/pageobject/Asset/AssetCreation.ts(1 hunks)src/components/Assets/AssetsList.tsx(4 hunks)
🔇 Additional comments (3)
cypress/pageobject/Asset/AssetCreation.ts (1)
5-5: LGTM: Selector update improves specificity and fixes typo
The change from [data-testid=create-asset-buttom] button to #create-asset-button is an improvement as it:
- Uses a more specific and performant ID selector
- Fixes the typo in the old selector ("buttom")
- Correctly maps to the button's ID in the UI component
✅ Verification successful
LGTM: Selector change is correctly implemented and matches UI component
The ID selector #create-asset-button in the Cypress test correctly corresponds to the button element in src/components/Assets/AssetsList.tsx where the ID is properly implemented. The change improves test reliability by using a unique, specific selector.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the ID selector exists in the codebase
rg -l 'id="create-asset-button"'
Length of output: 71
Script:
#!/bin/bash
# Let's verify the actual implementation in AssetsList.tsx to ensure the ID is used correctly
rg -A 2 'id="create-asset-button"' src/components/Assets/AssetsList.tsx
Length of output: 213
src/components/Assets/AssetsList.tsx (2)
30-31: LGTM: Clean integration of new components and hooks
The new imports and hook updates properly support the UI enhancements.
Also applies to: 42-42
322-355: Consider simplifying the nested layout structure
The button layout still has unnecessary nesting that could be simplified while maintaining the same functionality.
|
I don’t think the Cypress tests failed due to the changes in this PR. Could someone confirm? |
Jacobjeevan
added
needs testing
needs review
and removed
question
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/Assets/AssetsList.tsx (2)
432-446: Enhance search capabilitiesThe SearchByMultipleFields component is currently configured with only one search option. Consider adding more relevant search fields to fully utilize its capabilities.
options={[ { key: "Name/ Serial no./ QR code ID", label: "name/serial no./QR code ID", type: "text" as const, placeholder: "Search by name/serial no./QR code ID", value: qParams.search || "", shortcutKey: "f", }, + { + key: "location", + label: "Location", + type: "text" as const, + placeholder: "Search by location", + value: qParams.location || "", + }, + { + key: "asset_class", + label: "Asset Class", + type: "text" as const, + placeholder: "Search by asset class", + value: qParams.asset_class || "", + } ]}
Line range hint
533-599: Improve date handling and maintainabilityThe warrantyAmcValidityChip function could benefit from the following improvements:
- Extract magic numbers as named constants
- Simplify date difference calculation
- Add JSDoc documentation for better maintainability
+const EXPIRY_THRESHOLDS = { + CRITICAL: 30, + WARNING: 90, +} as const; + +/** + * Generates a chip component based on warranty/AMC validity status + * @param warranty_amc_end_of_validity - The end date of warranty/AMC + * @returns A Chip component indicating the warranty/AMC status + */ export const warrantyAmcValidityChip = ( warranty_amc_end_of_validity: string, ) => { if (warranty_amc_end_of_validity === "" || !warranty_amc_end_of_validity) return; const today = new Date(); const warrantyAmcEndDate = new Date(warranty_amc_end_of_validity); - const days = Math.ceil( - Math.abs(Number(warrantyAmcEndDate) - Number(today)) / - (1000 * 60 * 60 * 24), - ); + const days = Math.ceil((warrantyAmcEndDate.getTime() - today.getTime()) / (1000 * 60 * 60 * 24)); if (warrantyAmcEndDate < today) { return ( <Chip id="warranty-amc-expired-red" variant="danger" startIcon="l-times-circle" text="AMC/Warranty Expired" /> ); - } else if (days <= 30) { + } else if (days <= EXPIRY_THRESHOLDS.CRITICAL) { return ( <Chip id="warranty-amc-expiring-soon-orange" variant="custom" className="border-orange-300 bg-orange-100 text-orange-900" startIcon="l-exclamation-circle" text="AMC/Warranty Expiring Soon" /> ); - } else if (days <= 90) { + } else if (days <= EXPIRY_THRESHOLDS.WARNING) { return ( <Chip id="warranty-amc-expiring-soon-yellow" variant="warning" startIcon="l-exclamation-triangle" text="AMC/Warranty Expiring Soon" /> ); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Assets/AssetsList.tsx(4 hunks)
🔇 Additional comments (1)
src/components/Assets/AssetsList.tsx (1)
322-363: Simplify nested layout structure
The button layout has unnecessary nesting and duplicate flex containers that can be simplified.
…ub.com/AdityaJ2305/care_fe into issue/9349/button_layout_in_asset_page
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/Assets/AssetsList.tsx (1)
322-363: 🛠️ Refactor suggestionSimplify nested layout structure
The button layout still has unnecessary nesting that can be simplified. Consider the previous suggestion to reduce the nesting levels.
Apply this diff to simplify the layout:
- <div className="flex flex-wrap items-center gap-3"> - <div className="mb-2 flex w-full flex-col items-center gap-3 lg:mb-0 lg:w-fit lg:flex-row lg:gap-5"> - <div className="w-full lg:w-fit"> + <div className="flex flex-wrap items-center gap-3"> + <div className="flex w-full flex-wrap items-center gap-3 lg:w-fit"> <Button variant="primary" size="lg" className="w-full p-[10px] md:w-auto" onClick={() => setIsScannerActive(true)} > <CareIcon icon="l-search" className="text-base mr-2" /> Scan Asset QR </Button> - </div> - <div className="w-full lg:w-fit"> <Button variant="primary" size="lg" id="create-asset-button" disabled={!NonReadOnlyUsers} className="w-full p-[10px] md:w-auto" onClick={() => { if (qParams.facility) { navigate(`/facility/${qParams.facility}/assets/new`); } else { setShowFacilityDialog(true); } }} > <CareIcon icon="l-plus-circle" className="text-lg mr-2" /> <span>{t("create_asset")}</span> </Button> - </div> </div>
🧹 Nitpick comments (2)
src/components/Assets/AssetsList.tsx (2)
424-431: Improve responsive layout for count blockThe count block's empty className prop and the flex layout could be improved for better responsiveness.
Apply this diff:
- <div className="mt-4 gap-4 lg:gap-16 flex flex-col lg:flex-row lg:items-center"> + <div className="mt-4 flex flex-col gap-4 lg:flex-row lg:items-center lg:gap-16"> <CountBlock text="Total Assets" count={totalCount} loading={loading} icon="d-folder" - className="" + className="w-full lg:w-auto" />
436-442: Improve search field label clarityThe search field's key and label could be more consistent and clearer.
Apply this diff:
{ - key: "Name/ Serial no./ QR code ID", - label: "name/serial no./QR code ID", + key: "asset_search", + label: "Name / Serial No. / QR Code ID", type: "text" as const, - placeholder: "Search by Name / Serial no. / QR code ID", + placeholder: "Search by Name / Serial No. / QR Code ID", value: qParams.search || "", shortcutKey: "f", },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Assets/AssetsList.tsx(4 hunks)
🔇 Additional comments (3)
src/components/Assets/AssetsList.tsx (3)
11-12: LGTM: Import changes align with component updates
The new imports for Button and SearchByMultipleFields components are correctly structured and follow the absolute import pattern.
Also applies to: 19-19
Line range hint 530-589: LGTM: Well-structured warranty/AMC validation logic
The warrantyAmcValidityChip function is well-implemented with clear conditions and appropriate visual indicators for different warranty states.
42-42: Verify search functionality regression
The new SearchByMultipleFields component and clearSearch hook integration look good, but we should verify that the search functionality works as expected with the new implementation.
Run the following script to check for any existing tests covering the search functionality:
Also applies to: 432-446
✅ Verification successful
Search functionality is well-tested and implemented correctly
The search functionality has been thoroughly verified through the following findings:
- The
SearchByMultipleFieldscomponent is properly integrated with appropriate props including theclearSearchfunctionality - The
useFiltershook correctly implements theclearSearchstate management - Comprehensive Cypress test coverage exists in
cypress/e2e/assets_spec/that specifically tests:- Search by asset name
- Search by QR code
- Search by serial number
- Filter badge presence verification
- Search result verification
The implementation and test coverage indicate that the search functionality is working as expected with the new changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for search-related test coverage
rg -A 5 "asset-search|SearchByMultipleFields" --type=spec --type=test
Length of output: 100
Script:
#!/bin/bash
# Let's try a broader search for test files first
fd "test|spec" --type f
# Then search for any test files related to assets or search
rg -l "AssetsList|SearchByMultipleFields|clearSearch" --type=typescript
Length of output: 264
Script:
#!/bin/bash
# Search for test files with .test or .spec extensions
fd "\.(test|spec)\.(ts|tsx|js|jsx)$"
# Search for any files containing our search terms without file type restriction
rg -l "AssetsList|SearchByMultipleFields|clearSearch"
# Look for the implementation of SearchByMultipleFields to understand the search functionality
ast-grep --pattern 'const SearchByMultipleFields = $_'
Length of output: 500
Script:
#!/bin/bash
# Let's examine the implementation of SearchByMultipleFields and its usage in AssetsList
rg -A 10 "SearchByMultipleFields" src/components/Common/SearchByMultipleFields.tsx
rg -A 10 "SearchByMultipleFields" src/components/Assets/AssetsList.tsx
# Check the useFilters hook implementation since it's related to clearSearch
rg -A 10 "clearSearch" src/hooks/useFilters.tsx
# Look for any Cypress tests that might cover asset search functionality
rg -A 10 "asset.*search" cypress/e2e --ignore-case
Length of output: 10244
rithviknishad
removed
needs review
Deploy-Failed
Just ping the team on slack to rerun the tests next time 😅 |
Edit: Passed 🥳 |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/CAREUI/interactive/FiltersSlideover.tsx (1)
Line range hint
39-47: Update filter action buttons to use shadcn ButtonThe clear and apply buttons should be updated to use the new Button component with appropriate variants.
Here's how to update the buttons:
- <ButtonV2 - variant="danger" - ghost + <Button + variant="destructive" + className="flex items-center gap-2" onClick={onClear} id="clear-filter" > <CareIcon icon="l-filter-slash" className="text-lg" /> <span>{t("clear")}</span> - </ButtonV2> + </Button> - <ButtonV2 ghost onClick={onApply} id="apply-filter"> + <Button variant="default" onClick={onApply} id="apply-filter"> {t("apply")} - </ButtonV2> + </Button>
♻️ Duplicate comments (1)
src/CAREUI/interactive/FiltersSlideover.tsx (1)
7-10: 🛠️ Refactor suggestionRemove ButtonV2 import and usage for consistency
The codebase is transitioning to the new shadcn Button component, but ButtonV2 is still being imported and used in the FiltersSlideover component. This creates inconsistency in the UI.
Replace ButtonV2 usage in the FiltersSlideover component with the new Button component for consistency.
🧹 Nitpick comments (8)
src/components/Shifting/ShiftingList.tsx (1)
91-91: No functional issues found; consider adding an accessible label.
The replacement ofAdvancedFilterButtonwithFilterButtonis correctly implemented. However, for accessibility, consider adding anaria-labelor equivalent text to this button if it has no visible label.src/components/Users/ManageUsers.tsx (1)
126-126: Transition toFilterButtonis clean; add an accessible label for clarity.
The newFilterButtonmaintains the original functionality. For an improved user experience and accessibility, consider providing anaria-labelor visually hidden text describing the filter action.src/components/Resource/ResourceBoard.tsx (1)
104-104: Good consistency with the new filter component; review accessibility needs.
ReplacingAdvancedFilterButtonwithFilterButtonaligns with the standardized approach. Ensure you have sufficient descriptive text or anaria-labelfor screen readers if necessary.src/components/Shifting/ShiftingBoard.tsx (1)
135-135: Filter button usage is valid; consider labeling for accessibility.
The switch toFilterButtonmatches the approach in other components. Confirm that the button includes a textual label oraria-labelto support assistive technologies.src/components/Facility/FacilityList.tsx (1)
159-159: Ensure accessibility metadata for the new FilterButton.While this line successfully toggles the advanced filter, consider adding an
aria-labelor descriptive text to enhance accessibility for screen readers.src/components/Facility/DischargedPatientsList.tsx (1)
288-288: Suggest adding descriptive tooltip or label.While the filter button functionality remains the same, a descriptive label or tooltip could provide a clearer user experience, particularly for new users and screen readers.
src/components/Patient/ManagePatients.tsx (1)
905-905: Maintain uniform styling & accessibility for FilterButton.Double-check styling consistency with other filter buttons across the codebase. Consider verifying if an aria-label (or similar attribute) is needed to improve screen-reader support.
src/CAREUI/interactive/FiltersSlideover.tsx (1)
60-71: Improve className organization for better maintainabilityThe button styling looks good, but the margin utility class could be better organized.
Consider combining the margin class with the existing className:
<Button variant="outline_primary" - className="w-full md:w-auto" + className="w-full md:w-auto flex items-center gap-2" onClick={onClick} id="filter" > <CareIcon icon="l-filter" /> - <span className="py-0.5 ml-2">{t("filters")}</span> + <span className="py-0.5">{t("filters")}</span> </Button>This approach:
- Uses gap utility for consistent spacing
- Moves layout concerns to the parent element
- Follows the component-first approach of shadcn UI
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/CAREUI/interactive/FiltersSlideover.tsx(2 hunks)src/components/Assets/AssetsList.tsx(4 hunks)src/components/Facility/DischargedPatientsList.tsx(2 hunks)src/components/Facility/FacilityList.tsx(2 hunks)src/components/Patient/ManagePatients.tsx(2 hunks)src/components/Resource/ResourceBoard.tsx(2 hunks)src/components/Resource/ResourceList.tsx(2 hunks)src/components/Shifting/ShiftingBoard.tsx(2 hunks)src/components/Shifting/ShiftingList.tsx(2 hunks)src/components/Users/ManageUsers.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Assets/AssetsList.tsx
🔇 Additional comments (5)
src/components/Facility/FacilityList.tsx (1)
7-7: Unified naming of filter component import [FilterButton].
This switch from AdvancedFilterButton to FilterButton aligns well with the PR objective to maintain consistent naming conventions and design system usage across different components.
src/components/Resource/ResourceList.tsx (2)
6-6: Consistent filter button import [FilterButton].
The component import transition from AdvancedFilterButton to FilterButton helps standardize filter interactions in the codebase.
240-240: Re-check FilterButton callback for advanced filter.
Confirm that all references to FilterButton are updated properly. No issues are apparent here, but ensure that old references to AdvancedFilterButton calls (if any) have been removed in the entire file to avoid confusion.
src/components/Facility/DischargedPatientsList.tsx (1)
9-9: Harmonized filter component naming.
Adopting FilterButton in place of AdvancedFilterButton serves as a uniform approach, matching the pattern used across other modules.
src/components/Patient/ManagePatients.tsx (1)
37-37: Updated import for consistent filter naming.
This aligns with the broader refactoring efforts to replace AdvancedFilterButton with FilterButton. Implementation looks good.
|
LGTM |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/CAREUI/interactive/FiltersSlideover.tsx (1)
36-52: Consider removing conflicting button stylesThe apply button uses
variant="outline_primary"but also hasborder-none, which appears contradictory. The outline variant typically implies a border.Consider one of these approaches:
- <Button - variant="outline_primary" - onClick={onApply} - className="border-none" - id="apply-filter" - > + <Button + variant="primary" + onClick={onApply} + id="apply-filter" + >or if you need a borderless look:
- <Button - variant="outline_primary" - onClick={onApply} - className="border-none" - id="apply-filter" - > + <Button + variant="ghost" + onClick={onApply} + id="apply-filter" + >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/CAREUI/interactive/FiltersSlideover.tsx(3 hunks)
🔇 Additional comments (2)
src/CAREUI/interactive/FiltersSlideover.tsx (2)
7-7: LGTM: Successfully migrated to shadcn Button component
The import change aligns with the requirement to use the shadcn design system and removes ButtonV2 as requested in previous reviews.
63-74: LGTM: Successfully implemented requested changes
The changes effectively implement the requested updates:
- Renamed component from AdvancedFilterButton to FilterButton
- Updated button text from "advanced_filters" to "filters"
- Added proper spacing between icon and text
- Implemented responsive width for better mobile experience
|
👋 Hi, @AdityaJ2305, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
|
no longer relevant in new architecture |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Screenshots
Merge Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
Bug Fixes