Enhancement: Update the button layout in Assets Page #9392
Conversation
WalkthroughThe changes in the Changes
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 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: 0
🧹 Outside diff range and nitpick comments (2)
src/components/Assets/AssetsList.tsx (2)
190-215: Consider documenting keyboard shortcutsThe search configuration is well-structured with keyboard shortcuts for better accessibility. Consider adding documentation or tooltips to inform users about these shortcuts (n: Name, p: Serial Number, u: QR Code ID).
const searchOptions = [ { key: "name", label: "Name", type: "text" as const, - placeholder: "Search by Name", + placeholder: "Search by Name (Alt+N)", value: qParams.name || undefined, shortcutKey: "n", }, { key: "serial_number", label: "Serial Number", type: "text" as const, - placeholder: "Search by Serial Number", + placeholder: "Search by Serial Number (Alt+P)", value: qParams.serial_number || undefined, shortcutKey: "p", }, { key: "asset_qr_id", label: "QR Code ID", type: "text" as const, - placeholder: "Search by QR Code ID", + placeholder: "Search by QR Code ID (Alt+U)", value: qParams.qr_code_id || undefined, shortcutKey: "u", }, ];
388-440: Consider improving menu accessibilityThe Export Menu could benefit from ARIA labels for better screen reader support.
<ExportMenu + aria-label="Import and export assets menu" label={ importAssetModalOpen ? "Importing..." : "Import/Export" } exportItems={[ { label: "Import Assets", + options: { + ...options, + "aria-label": "Import assets from file", + }, }, { label: "Export Assets (JSON)", + options: { + ...options, + "aria-label": "Export assets as JSON file", + }, }, { label: "Export Assets (CSV)", + options: { + ...options, + "aria-label": "Export assets as CSV file", + }, }, ]} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Assets/AssetsList.tsx(6 hunks)
🔇 Additional comments (3)
src/components/Assets/AssetsList.tsx (3)
31-31: LGTM: Search functionality improvements
The changes enhance the search capabilities by introducing the SearchByMultipleFields component and properly configuring the cache exclusions for search fields.
Also applies to: 42-42, 45-45
Line range hint 352-443: LGTM: Improved button layout and responsiveness
The reorganization of buttons and search elements improves the UI with better spacing and responsive behavior.
447-462: LGTM: Search integration
The integration of the search component with the count block provides a clean and functional layout.
There was a problem hiding this comment.
LGTM, but let's switch to using Button instead of ButtonV2 in the entire AssetList.tsx file too. We are moving away to using shadcn UI components in all new files changed.
Button: https://github.com/ohcnetwork/care_fe/blob/develop/src/components/ui/button.tsx
Got it! |
In Button: https://github.com/ohcnetwork/care_fe/blob/develop/src/components/ui/button.tsx the primary
@rithviknishad, the background color of the Shadcn button with the primary variant is bg-green-700, but in the deployed one, the ButtonV2 has bg-green-500. Should I change it to bg-500 or keep it as it is? |
|
Skip modifying |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/components/Assets/AssetsList.tsx (2)
465-465: Update theidprop to match the component purposeThe
idprop is currently set to"patient-search", which seems to be a copy-paste error. Update it to"asset-search"for clarity and consistency with the assets context.Apply this diff:
-id="patient-search" +id="asset-search"
200-218: ReconsidershortcutKeyassignments for better intuitivenessThe
shortcutKeyvalues assigned to the search options may not be intuitive. Consider using keys that align with the initial letters of the search fields to enhance user experience.Suggested changes:
{ key: "name", label: "Name", type: "text" as const, placeholder: "Search by Name", value: qParams.name || undefined, - shortcutKey: "n", + shortcutKey: "n", // 'n' for 'name' is appropriate }, { key: "serial_number", label: "Serial Number", type: "text" as const, placeholder: "Search by Serial Number", value: qParams.serial_number || undefined, - shortcutKey: "p", + shortcutKey: "s", // 's' for 'serial_number' is more intuitive }, { key: "asset_qr_id", label: "QR Code ID", type: "text" as const, placeholder: "Search by QR Code ID", value: qParams.qr_code_id || undefined, - shortcutKey: "u", + shortcutKey: "q", // 'q' for 'QR Code ID' improves clarity },src/components/Common/Export.tsx (2)
Line range hint
53-72: Ensure consistent styling forButtoncomponentsWhile replacing
ButtonV2with the newButtoncomponent, verify that thevariantandsizeprops are appropriately set to maintain consistent styling across the application. Review the customclassNameproperties to minimize redundancy.Consider adjusting the button properties:
<Button - variant={"outline_primary"} - className="py-2.5" + variant="outline" + size="lg" >
Line range hint
119-144: CheckButtoncomponent styling inExportButtonEnsure that the
Buttoncomponent inExportButtonadheres to the standard styling guidelines. Usesizeandvariantprops instead of extensive custom classes to maintain consistency.Suggested adjustments:
<Button - className="tooltip mx-2 p-4 text-lg text-secondary-800 disabled:bg-transparent disabled:text-secondary-500" + variant="secondary" + size="lg" + className="tooltip mx-2" >This reduces reliance on custom styles and utilizes standardized properties.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/CAREUI/interactive/FiltersSlideover.tsx(2 hunks)src/components/Assets/AssetsList.tsx(8 hunks)src/components/Common/Export.tsx(6 hunks)
🔇 Additional comments (1)
src/CAREUI/interactive/FiltersSlideover.tsx (1)
63-72: LGTM
The replacement of ButtonV2 with the new Button component is correctly implemented, and the properties are appropriately set for consistent styling.
src/components/Assets/AssetsList.tsx
Outdated
| <SearchByMultipleFields | ||
| id="patient-search" | ||
| options={searchOptions} | ||
| onSearch={(key, value) => updateQuery({ search: value })} | ||
| clearSearch={clearSearch} | ||
| className="w-full" | ||
| /> |
There was a problem hiding this comment.
Fix 'onSearch' handler to correctly update query parameters
The onSearch function currently updates the search query parameter regardless of which field is being searched. To properly handle multiple search fields (name, serial_number, asset_qr_id), use the key parameter to update the corresponding query parameter.
Apply this diff to fix the issue:
-onSearch={(key, value) => updateQuery({ search: value })}
+onSearch={(key, value) => updateQuery({ [key]: value })}📝 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.
| <SearchByMultipleFields | |
| id="patient-search" | |
| options={searchOptions} | |
| onSearch={(key, value) => updateQuery({ search: value })} | |
| clearSearch={clearSearch} | |
| className="w-full" | |
| /> | |
| <SearchByMultipleFields | |
| id="patient-search" | |
| options={searchOptions} | |
| onSearch={(key, value) => updateQuery({ [key]: value })} | |
| clearSearch={clearSearch} | |
| className="w-full" | |
| /> |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/components/Assets/AssetsList.tsx (1)
194-219: Consider enhancing type safety for search options.While the search configuration is well-structured, we can improve type safety by defining an interface for the search options.
Consider applying this change:
+interface SearchOption { + key: 'name' | 'serial_number' | 'asset_qr_id'; + label: string; + type: 'text'; + placeholder: string; + value: string | undefined; + shortcutKey: string; +} -const searchOptions = [ +const searchOptions: SearchOption[] = [ { key: "name", // ... rest remains the same },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Assets/AssetsList.tsx(8 hunks)
🔇 Additional comments (3)
src/components/Assets/AssetsList.tsx (3)
Line range hint 20-33: LGTM! Hook configuration aligns with enhanced search functionality.
The addition of SearchByMultipleFields and Button components, along with the updated useFilters configuration, properly supports the new multi-field search feature.
Also applies to: 43-47
Line range hint 356-450: LGTM! Button layout improvements with proper responsive design.
The updated button layout:
- Uses the new Button component as requested in PR comments
- Maintains proper authorization checks
- Implements responsive design with appropriate flex classes
464-470:
Fix 'onSearch' handler to correctly update query parameters.
The onSearch function currently updates the 'search' query parameter regardless of which field is being searched. This issue was previously identified and still needs to be fixed.
Apply this diff to fix the issue:
-onSearch={(key, value) => updateQuery({ search: value })}
+onSearch={(key, value) => updateQuery({ [key]: value })}
|
@rithviknishad review this |
|
cypress tests are failing |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/components/Assets/AssetsList.tsx (3)
Line range hint
137-198: Consider extracting QR code scanning logicThe QR code scanning logic is complex and handles multiple responsibilities. Consider extracting it into a separate custom hook for better maintainability and reusability.
+// Create a new file: hooks/useQRScanner.ts +export const useQRScanner = () => { + const { t } = useTranslation(); + + const scanAssetQR = async (assetURL: string) => { + // ... existing logic ... + }; + + return { scanAssetQR }; +}; -const accessAssetIdFromQR = async (assetURL: string) => { - // ... existing logic ... -}; +const { scanAssetQR } = useQRScanner();
Line range hint
362-377: Consider using consistent button variantsThe button implementation looks good but consider using consistent size and width props across all buttons for better UI consistency.
-<Button - disabled={isDisabled} - variant={"primary"} - size={"lg"} - className="gap-2 w-full lg:w-fit" +<Button + disabled={isDisabled} + variant="default" + size="default" + className="gap-2 w-full lg:w-fit"
Line range hint
578-632: Optimize date calculations in warrantyAmcValidityChipThe function is well-structured but consider memoizing the date calculations to prevent unnecessary recalculations on re-renders.
+import { useMemo } from 'react'; -export const warrantyAmcValidityChip = ( +export const warrantyAmcValidityChip = useMemo(( warranty_amc_end_of_validity: string, -) => { +) => { // ... existing logic ... -}; +}, [warranty_amc_end_of_validity]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Assets/AssetsList.tsx(7 hunks)
🔇 Additional comments (3)
src/components/Assets/AssetsList.tsx (3)
31-32: LGTM: Clean import of new components
The addition of SearchByMultipleFields and Button components aligns with the PR objectives and maintains good code organization.
467-473:
Fix SearchByMultipleFields implementation
The onSearch handler is not utilizing the key parameter, which defeats the purpose of having multiple search fields.
Apply this fix to properly handle multiple search fields:
-onSearch={(key, value) => updateQuery({ search: value })}
+onSearch={(key, value) => updateQuery({ [key]: value })}43-46: Verify clearSearch implementation for multiple search fields
The clearSearch function is added but its implementation needs verification to ensure it properly clears all search fields (name, serial_number, qr_code_id).
✅ Verification successful
Let me try to find the implementation of clearSearch and useFilters to verify how the search fields are being cleared.
I need to see the full implementation of useFilters and SearchByMultipleFields to understand how the clearSearch functionality is implemented and verify if it properly handles multiple search fields.
Let me try with different patterns to find these implementations, as the previous search didn't yield results.
From the earlier search results, I can see that:
- The clearSearch functionality is implemented in SearchByMultipleFields component and checks for specific params to clear
- The useFilters hook manages the clearSearch state and provides it to components
- The implementation in AssetsList.tsx properly passes the clearSearch prop and includes all necessary fields (name, serial_number, qr_code_id) in the cacheBlacklist
The clearSearch implementation is properly configured for multiple search fields
The codebase shows that clearSearch is correctly implemented to handle multiple search fields:
- The SearchByMultipleFields component checks clearSearch params against field keys
- The cacheBlacklist in AssetsList includes all required fields: name, serial_number, and qr_code_id
- The clearSearch functionality is consistently used across similar components (FacilityList, ManagePatients)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for clearSearch implementation
rg "clearSearch.*=.*{" -A 5
Length of output: 127
Script:
#!/bin/bash
# Search for clearSearch implementation with fixed regex
rg "clearSearch" -A 5
# Also search for useFilters implementation
ast-grep --pattern 'const useFilters = $_'
Length of output: 6245
Script:
#!/bin/bash
# Let's look at the useFilters hook implementation
rg -A 20 "const useFilters = \(" src/hooks/useFilters.tsx
# And also check the SearchByMultipleFields component implementation
rg -A 20 "const SearchByMultipleFields = " src/components/Common/SearchByMultipleFields.tsx
Length of output: 153
Script:
#!/bin/bash
# Try different pattern to find useFilters implementation
ast-grep --pattern 'export const useFilters = $_'
# Also try to find the SearchByMultipleFields component with a different pattern
ast-grep --pattern 'export const SearchByMultipleFields: React.FC<$_> = $_'
Length of output: 129
@Jacobjeevan @rithviknishad checks done! |
src/components/Common/Export.tsx
Outdated
| disabled={isExporting || disabled} | ||
| title={isExporting ? "Exporting..." : label} | ||
| icon={<CareIcon icon="l-export" />} | ||
| className="tooltip border-primary-500 bg-white text-primary-500 hover:bg-primary-100 enabled:border" | ||
| className="tooltip border-primary-700 bg-white text-primary-700 hover:bg-primary-100 enabled:border" | ||
| > | ||
| {exportItems.map((item) => ( | ||
| <DropdownItem |
There was a problem hiding this comment.
Let's switch to dropdown from shadcn too since you've touched this file
There was a problem hiding this comment.
@rithviknishad sure
…enhanced-asset-header
…ii/care_fe into enhanced-asset-header
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/components/Common/Export.tsx (4)
Line range hint
58-77: Consider standardizing button stylingThe gap-2 class is used in the dropdown trigger button but not here. Consider standardizing the spacing across all export buttons for consistency.
- <Button + <Button variant={"outline_primary"} disabled={isExporting || disabled} onClick={() => { let action = item.action; if (item.route) { action = async () => { const { data } = await request(item.route!); return data ?? null; }; } if (action) { exportFile(action, item.filePrefix, item.type, item.parse); } }} - className="py-2.5" + className="py-2.5 gap-2" >
96-117: Refactor click handler for better maintainabilityThe onClick handler logic is duplicated between single export and menu items. Consider extracting this into a reusable function.
+ const handleExport = (item: ExportItem) => { + let action = item.action; + if (item.route) { + action = async () => { + const { data } = await request(item.route!); + return data ?? null; + }; + } + if (action) { + exportFile(action, item.filePrefix, item.type, item.parse); + } + }; return ( <div key={item.label} - onClick={() => { - let action = item.action; - if (item.route) { - action = async () => { - const { data } = await request(item.route!); - return data ?? null; - }; - } - if (action) { - exportFile(action, item.filePrefix, item.type, item.parse); - } - }} + onClick={() => handleExport(item)} {...item.options} >
Line range hint
135-160: Simplify button stylingThe className combines multiple concerns which could be separated for better maintainability. Consider using the cn utility to compose classes conditionally.
- className="tooltip mx-2 p-4 text-lg text-secondary-800 disabled:bg-transparent disabled:text-secondary-500" + className={cn( + "tooltip mx-2 p-4 text-lg", + "text-secondary-800", + "disabled:bg-transparent disabled:text-secondary-500" + )}
No ButtonV2 instances found in Export.tsx, but migration incomplete in other files
The file
src/components/Common/Export.tsxhas been properly migrated to use the new shadcn Button component from"../ui/button". However, there are still numerous files in the codebase that are using the oldButtonV2component, including:
- User management components (
src/components/Users/)- Shifting components (
src/components/Shifting/)- Resource components (
src/components/Resource/)- Patient components (
src/components/Patient/)- Facility components (
src/components/Facility/)- And many others
🔗 Analysis chain
Line range hint
1-165: Verify complete migration from ButtonV2Let's ensure all instances of ButtonV2 have been replaced with the shadcn Button component in this file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining ButtonV2 instances in the file rg "ButtonV2" src/components/Common/Export.tsxLength of output: 46
Script:
#!/bin/bash # Let's verify the Button imports and usage in the file rg -A 1 "import.*Button" src/components/Common/Export.tsx # Also check if there are any other files still using ButtonV2 rg "ButtonV2" --type ts --type tsxLength of output: 170
Script:
#!/bin/bash # Check for ButtonV2 usage across all files (without file type restriction) rg "ButtonV2" # Verify the Button import and its usage in Export.tsx rg -C 2 "Button" src/components/Common/Export.tsxLength of output: 45522
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Assets/AssetsList.tsx(7 hunks)src/components/Common/Export.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Assets/AssetsList.tsx
🔇 Additional comments (1)
src/components/Common/Export.tsx (1)
12-18: LGTM! Proper migration to shadcn components
The imports have been correctly updated to use shadcn UI components for both Button and DropdownMenu, which aligns with the project's direction and addresses the previous review feedback.
Proposed Changes
Summary by CodeRabbit
New Features
name,qr_code_id, andserial_number.AssetsListcomponent.FiltersSlideoverandExportcomponents for a more consistent user interface.Bug Fixes