Skip to content

Conversation

@CharlyMartin
Copy link
Collaborator

@CharlyMartin CharlyMartin commented Dec 1, 2025

📝 Description

This PR adds mobile-responsive table filters to the Service Providers table and refactors the filtering implementation for better maintainability.

[UXIT-3675]

🛠️ Key Changes

  • Added MobileTableFilters component with slide-over panel using SlideOver from ui-filecoin
  • Added DesktopTableFilters component extracted from original TableFilters implementation
  • Updated TableFilters to conditionally render desktop/mobile variants based on screen size using responsive classes
  • Extracted individual filter components for better reusability:
    • CountryFilter - handles country checkbox filtering
    • CapacityFilter - handles capacity min/max range inputs
    • ProvingPeriodFilter - handles proving period min/max range inputs
    • StatusFilter - handles status checkbox filtering
    • InpiFilter - handles IPNI checkbox filtering
  • Extracted shared UI components:
    • CheckboxContainer - wrapper for checkbox lists
    • CheckboxWithLabel - reusable checkbox with label component
    • FiltersSectionHeading - consistent section headings for filters
    • InputContainer - wrapper for input groups
    • NumberInput - reusable number input with accessibility labels
  • Extracted utility functions to ../app/service-providers/utils/:
    • parse-numeric-input.ts - parses string input to numeric values
    • toggle-value-in-array.ts - toggles values in filter arrays
  • Refactored TableFilters component props to only accept options (removed state and setState)
  • Updated ServiceProvidersTable to remove setFilterQueries prop from TableFilters
  • Migrated components to use useBackground hook (replacing deprecated useBackgroundVariant):
  • Updated @filecoin-foundation/ui-filecoin dependency to v0.5.4

📸 Screenshots

CleanShot 2025-12-02 at 12 03 42@2x

@vercel
Copy link

vercel bot commented Dec 1, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
filecoin-cloud Ready Ready Preview Comment Dec 8, 2025 6:48pm

@CharlyMartin CharlyMartin mentioned this pull request Dec 3, 2025
…Period filters

Removed duplicated useCallback handlers and parseNumericInput logic from CapacityFilter.tsx and ProvingPeriodFilter.tsx. Updated to use the new updateNumberQuery function from useFilterQueryState hook for consistent numeric input parsing and state updates, reducing code duplication.
Introduce new `FilterHeading` component as a simple, reusable h3 for filter sections. Update `CapacityFilter`, `CountryFilter`, `InpiFilter`, `ProvingPeriodFilter`, and `StatusFilter` to use it instead of the deprecated `FiltersSectionHeading`, improving consistency and reducing duplication in styling.
Updated prop interface names in CheckboxesContainer, InputsContainer, and
NumberInputWithLabel to match their respective component names for better
consistency and readability. Minor formatting adjustments in
NumberInputWithLabel for destructuring.
Restructured Popover in DesktopTableFilters to include a "Reset Filters" button
that clears query state and closes the panel. Updated styling for better layout.
Applied similar import and structure changes to MobileTableFilters for consistency.
…ng period filters

Introduce utility function `isMinAboveMax` to detect invalid min > max values in filter queries. Update `CapacityFilter` and `ProvingPeriodFilter` components to use Headless UI `Fieldset` for semantic structure, conditionally render `ErrorMessage` component when validation fails, and display user-friendly error for min exceeding max. This improves UX by providing immediate feedback on invalid range inputs without breaking form functionality.
Introduce `hasActiveFilters` in `useFilterQueryState` hook using `useMemo` to detect if any filter queries have values (non-empty arrays or non-null).

Update `DesktopTableFilters` and `MobileTableFilters` to destructure and use `hasActiveFilters`, disabling the clear/reset buttons when false.

This improves UX by preventing redundant reset actions when no filters are applied.
…ries

Replace direct state updates with functional form in toggleFilterQuery and
updateNumberQuery to avoid stale closures. Remove filterQueries from useCallback
dependencies, as prev state is now used internally, improving performance and
preventing unnecessary re-renders.
…ading to Location

Renamed the `options` prop destructuring from `countryOptions` to `options` for clarity, and changed the filter heading from "Country" to "Location" to better reflect the intended usage in service providers filtering.
…lter, StatusFilter

Updated CountryFilter, InpiFilter, and StatusFilter components to use Headless UI's
Fieldset for the outer container instead of a plain div. This improves semantic HTML
and accessibility for form groupings in the service providers filters.
…tainer

Adjust layout to prevent wrapping of search and filter elements, ensuring they stay in a single row for better responsiveness on medium screens and up.
{ipniOptions.length > 1 && <InpiFilter options={ipniOptions} />}

<div className="pt-8 flex flex-col gap-4">
<Button variant="primary" onClick={() => setOpen(false)}>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -0,0 +1,3 @@
export function ErrorMessage({ message }: { message: string }) {
Copy link
Collaborator

@mirhamasala mirhamasala Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing Warm Storage Service will also have an ErrorMessage and this and other components might move?

I'm also wondering whether this error text color should be moved to UI-Filecoin. Or, at least, the local globals.css?

Perhaps no action needed - just things I was considering.

}

export function InputsContainer({ children }: InputsContainerProps) {
return <div className="flex gap-4 flex-wrap @sm:flex-nowrap">{children}</div>
Copy link
Collaborator

@mirhamasala mirhamasala Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be able to use container queries, wouldn't the parent need to have an @ prefix?

And you can't really guarantee this context by just looking at this component, right? They're separated from each other..

options: Array<string>
}

export function InpiFilter({ options }: InpiFilterProps) {
Copy link
Collaborator

@mirhamasala mirhamasala Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spelling error. Same for filename, etc?

Suggested change
export function InpiFilter({ options }: InpiFilterProps) {
export function IpniFilter({ options }: IpniFilterProps) {

/>
</InputsContainer>
{minAboveMax && (
<ErrorMessage message="Minimum shouldn't be above maximum" />
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be more actionable

Suggested change
<ErrorMessage message="Minimum shouldn't be above maximum" />
<ErrorMessage message="Minimum capacity must be less than or equal to maximum capacity" />

/>
</InputsContainer>
{minAboveMax && (
<ErrorMessage message="Minimum shouldn't be above maximum" />
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe

Suggested change
<ErrorMessage message="Minimum shouldn't be above maximum" />
<ErrorMessage message="Minimum period must be less than or equal to maximum" />

@@ -0,0 +1,6 @@
export function isMinAboveMax(min: number | null, max: number | null) {
if (min === null || max === null) return false
if (min === max) return false
Copy link
Collaborator

@mirhamasala mirhamasala Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

min > max already returns false when they're equal..

Suggested change
if (min === max) return false

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds mobile-responsive table filters to the Service Providers table through a comprehensive refactoring that improves code maintainability and reusability. The implementation splits the original monolithic TableFilters component into separate desktop and mobile variants, with desktop using a popover panel and mobile using a slide-over drawer. The refactoring extracts individual filter components (Country, Capacity, ProvingPeriod, Status, IPNI) and shared UI primitives (checkboxes, inputs, headings, containers) into reusable components, while also extracting utility functions for common operations.

Key Changes:

  • Added responsive mobile/desktop filter UI with MobileTableFilters (slide-over) and DesktopTableFilters (popover panel)
  • Extracted individual filter components and shared UI primitives for better reusability
  • Enhanced the useFilterQueryState hook with toggleFilterQuery, updateNumberQuery, and hasActiveFilters utilities

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
src/app/service-providers/utils/toggle-value-in-array.ts New utility function to toggle values in filter arrays
src/app/service-providers/utils/parse-numeric-input.ts New utility to parse string inputs to numbers (changed from parseInt to Number)
src/app/service-providers/utils/is-min-above-max.ts New utility to validate min/max range inputs
src/app/service-providers/hooks/useFilterQueryState.ts Enhanced with helper methods and active filters check
src/app/service-providers/components/TableFilters.tsx Refactored to conditionally render desktop/mobile variants
src/app/service-providers/components/DesktopTableFilters.tsx New desktop popover variant with backdrop
src/app/service-providers/components/MobileTableFilters.tsx New mobile slide-over variant with FilterButton trigger
src/app/service-providers/components/CountryFilter.tsx Extracted country filter (heading changed from "Country" to "Location")
src/app/service-providers/components/CapacityFilter.tsx Extracted capacity range filter with validation
src/app/service-providers/components/ProvingPeriodFilter.tsx Extracted proving period range filter with validation
src/app/service-providers/components/StatusFilter.tsx Extracted status checkbox filter
src/app/service-providers/components/InpiFilter.tsx Extracted IPNI filter (component name has typo: should be IpniFilter)
src/app/service-providers/components/CheckboxWithLabel.tsx Reusable checkbox with label component
src/app/service-providers/components/CheckboxesContainer.tsx Container wrapper for checkbox lists
src/app/service-providers/components/FilterHeading.tsx Consistent heading component for filter sections
src/app/service-providers/components/NumberInputWithLabel.tsx Reusable number input with visible label
src/app/service-providers/components/InputsContainer.tsx Container for input groups with responsive flex layout
src/app/service-providers/components/ErrorMessage.tsx Error message component for validation feedback
src/app/service-providers/components/ServiceProvidersTable.tsx Updated to remove setFilterQueries prop and adjust responsive widths

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

<TableFilters options={filterOptions} />
</div>
<div className="md:w-56 w-full">
<div className="md:w-56">
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The w-full class was removed from the NetworkSelector wrapper. This changes the mobile layout behavior. Ensure this is intentional and that the NetworkSelector still displays correctly on mobile devices without the full-width styling.

Suggested change
<div className="md:w-56">
<div className="md:w-56 w-full">

Copilot uses AI. Check for mistakes.
@@ -0,0 +1,3 @@
export function ErrorMessage({ message }: { message: string }) {
return <p className="text-red-500 text-sm pt-2">{message}</p>
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message lacks ARIA attributes to announce it to screen readers. Consider adding role="alert" or aria-live="polite" to ensure users with assistive technology are notified when validation errors appear.

Suggested change
return <p className="text-red-500 text-sm pt-2">{message}</p>
return <p className="text-red-500 text-sm pt-2" role="alert">{message}</p>

Copilot uses AI. Check for mistakes.
@@ -0,0 +1,4 @@
export function parseNumericInput(value: string) {
const parsed = value === '' ? null : Number(value)
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using Number(value) instead of Number.parseInt(value, 10) changes the parsing behavior. Number() accepts decimal values like "123.45", while the original implementation used parseInt which would truncate decimals. For capacity (TiB) and proving period (epochs) values, this could lead to unintended decimal inputs. Consider using Number.parseInt(value, 10) to maintain the original integer-only behavior, or document that decimal values are now intentionally supported.

Suggested change
const parsed = value === '' ? null : Number(value)
const parsed = value === '' ? null : Number.parseInt(value, 10)

Copilot uses AI. Check for mistakes.

return (
<Fieldset>
<FilterHeading>Location</FilterHeading>
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The heading text changed from "Country" (in the original TableFilters) to "Location". While this might be intentional, it creates a mismatch with the component name CountryFilter, the prop name countryOptions, and the filter query key country. Consider either changing the heading back to "Country" or renaming all related identifiers to use "Location" for consistency.

Suggested change
<FilterHeading>Location</FilterHeading>
<FilterHeading>Country</FilterHeading>

Copilot uses AI. Check for mistakes.
anchor={{ to: 'bottom', gap: 16 }}
className={clsx(
backgroundVariants[theme],
'@container bg-white w-[640px] max-h-[80vh] overflow-y-auto p-6 rounded-2xl border border-(--color-listbox-border) shadow-xs',
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bg-white class is redundant here since backgroundVariants[theme] already controls the background color. This could lead to conflicting styles if the theme is not the default white background.

Suggested change
'@container bg-white w-[640px] max-h-[80vh] overflow-y-auto p-6 rounded-2xl border border-(--color-listbox-border) shadow-xs',
'@container w-[640px] max-h-[80vh] overflow-y-auto p-6 rounded-2xl border border-(--color-listbox-border) shadow-xs',

Copilot uses AI. Check for mistakes.
Comment on lines +1 to +4
export function parseNumericInput(value: string) {
const parsed = value === '' ? null : Number(value)
return Number.isNaN(parsed) ? null : parsed
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

console.log(parseNumericInput(' ')) // 0
console.log(parseNumericInput('\t')) // 0

Suggested change
export function parseNumericInput(value: string) {
const parsed = value === '' ? null : Number(value)
return Number.isNaN(parsed) ? null : parsed
}
export function parseNumericInput(value: string) {
const trimmed = value.trim()
const parsed = trimmed === '' ? null : Number(trimmed)
return Number.isNaN(parsed) ? null : parsed
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From Claude Code:

// Current: Uses Number() which allows decimals
const parsed = value === '' ? null : Number(value)

// But filterParsers uses parseAsInteger
capacityMin: parseAsInteger,
Issue: Number("1.5") returns 1.5 but parseAsInteger would parse it as 1. This could cause sync issues between local state and URL state. Recommendation: Use Number.parseInt(value, 10) to match the parseAsInteger behavior.

Comment on lines +2 to +6
const updated = current.includes(value)
? current.filter((v) => v !== value)
: [...current, value]

return updated
Copy link
Collaborator

@mirhamasala mirhamasala Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const updated = current.includes(value)
? current.filter((v) => v !== value)
: [...current, value]
return updated
return current.includes(value)
? current.filter((v) => v !== value)
: [...current, value]

Copy link
Collaborator

@mirhamasala mirhamasala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CharlyMartin This is mega clean! Love it. ✨

There are some renaming that need to happen before merge. And I'd definitely look at this comment.

Other than that mainly nits for you to decide what to implement and what not.

Also left a GitHub Co-Pilot review because we can.

❤️

@github-project-automation github-project-automation bot moved this from 🔎 Awaiting review to ✔️ Approved by reviewer in FS Dec 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: ✔️ Approved by reviewer

Development

Successfully merging this pull request may close these issues.

3 participants