-
Notifications
You must be signed in to change notification settings - Fork 520
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: Clear All Filters Badge Visibility in Resource page #9557
Fix: Clear All Filters Badge Visibility in Resource page #9557
Conversation
WalkthroughThe pull request modifies the Changes
Assessment against linked issues
Possibly Related PRs
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (2)src/hooks/useFilters.tsx (2)
Currently, when
Introducing a dedicated method for date-based filtering helps maintain consistency with the existing 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/Resource/ResourceFilter.tsx (2)
43-51
: Correct the "orginFacilityLoading" naming
The variable name appears to have a minor typo. Renaming it tooriginFacilityLoading
would improve readability and consistency across the codebase.- const { loading: orginFacilityLoading } = useQuery(...) + const { loading: originFacilityLoading } = useQuery(...)
55-63
: Confirm clarity on naming
resourceFacilityLoading
might be slightly ambiguous since it refers specifically to the “approving facility.” Consider using a name such asapprovingFacilityLoading
for consistency.- const { loading: resourceFacilityLoading } = useQuery(routes.getAnyFacility, { + const { loading: approvingFacilityLoading } = useQuery(routes.getAnyFacility, {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Resource/ResourceFilter.tsx
(3 hunks)
🔇 Additional comments (3)
src/components/Resource/ResourceFilter.tsx (3)
19-19
: Switched to a new query hook
The use of useQuery
is aligned with the updated codebase design. This replacement ensures consistency and leverages the newer hook’s maintained features.
34-34
: Removed default value for the “emergency” filter
Changing the default emergency
value from "--"
to ""
is a good way to avoid unintended filter application.
67-75
: Good facility fetch pattern
The usage of useQuery
to prefetch the assigned facility data and update the local state is correct and consistent. No issues identified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/Resource/ResourceFilter.tsx (1)
45-97
: Consider refactoring duplicate facility query implementations.The three facility query implementations share identical patterns and could be refactored into a reusable hook for better maintainability.
Consider creating a custom hook like this:
const useFacilityQuery = (facilityId: string, type: 'origin' | 'approving' | 'assigned') => { const { isLoading, data } = useQuery({ queryKey: [routes.getAnyFacility.path, facilityId], queryFn: () => query(routes.getAnyFacility, { pathParams: { id: facilityId }, }), enabled: facilityId !== undefined, }); useEffect(() => { if (data) { setFilterState({ [`${type}_facility_ref`]: facilityId === "" ? "" : data, }); } }, [data, facilityId]); return { isLoading, data }; };Usage:
const { isLoading: originFacilityLoading } = useFacilityQuery( filter.origin_facility, 'origin' );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Resource/ResourceFilter.tsx
(4 hunks)
🔇 Additional comments (3)
src/components/Resource/ResourceFilter.tsx (3)
1-1
: LGTM: Import changes align with recommended practices.
The switch to direct TanStack Query usage aligns with the documentation and previous review feedback.
Also applies to: 21-21
36-36
: LGTM: Emergency filter default value updated.
The removal of "--" as default value for emergency filter aligns with the PR objective to improve filter behavior.
1-1
: Verify implementation of "Clear all filters" badge visibility.
While the emergency filter default value has been addressed, I don't see changes related to the "Clear all filters" badge visibility mentioned in the PR objectives.
Let's check if this is implemented elsewhere:
✅ Verification successful
Let me gather more information about the "Clear all filters" badge visibility implementation.
Let me check the ResourceBadges component to understand the badge visibility implementation.
Let me try to find the useFilters hook with a different approach.
Let me check the useFilters hook implementation.
"Clear all filters" badge visibility is properly implemented
The "Clear all filters" badge visibility is already implemented in the FilterBadges
component within the useFilters
hook. The badge is shown when:
- There are active filters (determined by
activeFilters.length > 0
) - Or when there are children components (
children
prop is present)
The implementation includes:
- Automatic tracking of active filters
- A "Clear all filters" button that appears when filters are active
- Proper translation support via
t("clear_all_filters")
- Correct styling and positioning with the filter badges
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for files containing "Clear all filters" text or badge-related code
rg -i "clear all filters|clearallfilters" --type tsx --type ts
# Search for FiltersSlideover component definition
fd -e tsx -e ts FiltersSlideover
Length of output: 171
Script:
#!/bin/bash
# Check the FiltersSlideover component for badge-related code
cat src/CAREUI/interactive/FiltersSlideover.tsx
# Search for badge-related code in ResourceFilter and its directory
rg -i "badge|clear.*filter" src/components/Resource/
Length of output: 3402
Script:
#!/bin/bash
# Check the ResourceBadges component implementation
cat src/components/Resource/ResourceBadges.tsx
# Check the useFilters hook implementation
fd -e ts -e tsx useFilters.ts useFilters.tsx
Length of output: 2231
Script:
#!/bin/bash
# Find useFilters hook implementation
fd useFilters src/
# Check the FilterBadges component that's being used
rg -A 5 "export.*FilterBadges"
Length of output: 78
Script:
#!/bin/bash
# Check the useFilters hook implementation
cat src/hooks/useFilters.tsx
# Look for FilterBadges component in Common components
rg -l "FilterBadges" src/components/Common/
Length of output: 9163
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than that, lgtm
…701/care_fe into clear-all-filter-badge
No longer relevant. |
Proposed Changes
clear.badge.mp4
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes
"--"
to an empty string""
.