Skip to content
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.

Sign up for GitHub

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

Jump to bottom

DataViews: extract FilterEnumeration component #56514

Closed
wants to merge 2 commits into from
Closed

DataViews: extract FilterEnumeration component #56514

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
03ad035
Extract EnumerationFilter component
oandregal Nov 24, 2023
67d5dec
Reuse FilterEnumeration in ViewList
oandregal Nov 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 6 additions & 23 deletions packages/edit-site/src/components/dataviews/add-filter.js
View file
Open in desktop
Expand Up @@ -14,12 +14,12 @@ import { __ } from '@wordpress/i18n';
*/
import { unlock } from '../../lock-unlock';
import { ENUMERATION_TYPE, OPERATOR_IN } from './constants';
import FilterEnumeration from './filter-enumeration';

const {
DropdownMenuV2: DropdownMenu,
DropdownSubMenuV2: DropdownSubMenu,
DropdownSubMenuTriggerV2: DropdownSubMenuTrigger,
DropdownMenuItemV2: DropdownMenuItem,
} = unlock( componentsPrivateApis );

export default function AddFilter( { fields, view, onChangeView } ) {
Expand Down Expand Up @@ -78,28 +78,11 @@ export default function AddFilter( { fields, view, onChangeView } ) {
</DropdownSubMenuTrigger>
}
>
{ filter.elements.map( ( element ) => (
<DropdownMenuItem
key={ element.value }
onSelect={ () => {
onChangeView( ( currentView ) => ( {
...currentView,
page: 1,
filters: [
...currentView.filters,
{
field: filter.field,
operator: OPERATOR_IN,
value: element.value,
},
],
} ) );
} }
role="menuitemcheckbox"
>
{ element.label }
</DropdownMenuItem>
) ) }
<FilterEnumeration
filter={ filter }
view={ view }
onChangeView={ onChangeView }
/>
</DropdownSubMenu>
);
} ) }
Expand Down
52 changes: 52 additions & 0 deletions packages/edit-site/src/components/dataviews/filter-enumeration.js
View file
Open in desktop
@@ -0,0 +1,52 @@
/**
* WordPress dependencies
*/
import { privateApis as componentsPrivateApis } from '@wordpress/components';

/**
* Internal dependencies
*/
import { unlock } from '../../lock-unlock';
import { OPERATOR_IN } from './constants';

const { DropdownMenuCheckboxItemV2: DropdownMenuCheckboxItem } = unlock(
componentsPrivateApis
);

export default function ( { filter, view, onChangeView } ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that we now have a FilterEnumeration component. I imagine in the future we could have a FilterString or something else as well. I also like to keep all of these components with similar API (props). The current props look good to me.

What I don't like is that this component assumes that it's rendered within a dropdown menu. For me, this makes the component less reusable and it's also not something that we will be able to support for all types of filters (how do you put an input as a dropdown menu item). I'd have preferred if this component was just a normal form.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd have preferred if this component was just a normal form.

Could you clarify this bit? How would you do it?

I'd very much like having a standalone component that doesn't depend on the trigger component being any particular type. I'm just not sure how to achieve it with the existing component library. I've been resistant to open this PR until today, precisely because I didn't like this trigger/parent dependency.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm not wrong, dropdowns (not menu dropdowns) or modals can render any random component. So a design like something in my related comment here #56479 (comment) would be able to be achieved in a reusable component.

Pseudo code:

EnumarationFilter( props ) {

   return (
       <VStack>
             <Select label="operator" values={ [ 'in', 'not in' ] } />
             <Select label="value" multiple values={ props.filter.elements } />
       </VStack>
   );
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. Do you feel this PR would be useful towards implementing that vision (all components are centralized in a single place, so it's easier to change the UI for all of them at once)? Or do you feel we should clarify that vision first?

Copy link
Member Author

Choose a reason for hiding this comment

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

I feel we should ship this one as it is and iterate on UI after. Everything (including updating UI) becomes easier.

Copy link
Contributor

Choose a reason for hiding this comment

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

In my mind these intermediary steps are a detour and we should just build the generic filters. That said, I'm ok if we have different opinions here. So if you all agree about the current path, please go ahead. Nothing is set in stone.

const filterInView = view.filters.find( ( f ) => f.field === filter.field );
const activeElement = filter.elements.find(
( element ) => element.value === filterInView?.value
);

return filter.elements.map( ( element ) => {
return (
<DropdownMenuCheckboxItem
Copy link
Contributor

@ciampo ciampo Nov 30, 2023 •
edited

Choose a reason for hiding this comment

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

As also discussed previously (and recently noted by @andrewhayward ), mutually exclusive options should be radio menu items, instead of checkbox menu items.

If we wanted to keep the scope of this PR focused on the refactor, we could make the changes to radios in a follow-up

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I've addressed Andrew's feedback in that PR for filters and prepared a different one reviewing every other menu item in dataviews: #56676

key={ element.value }
value={ element.value }
checked={ activeElement?.value === element.value }
onSelect={ () =>
onChangeView( ( currentView ) => ( {
...currentView,
page: 1,
filters: [
...view.filters.filter(
( f ) => f.field !== filter.field
),
{
field: filter.field,
operator: OPERATOR_IN,
value:
activeElement?.value === element.value
? undefined
: element.value,
},
],
} ) )
}
>
{ element.label }
</DropdownMenuCheckboxItem>
);
} );
}
43 changes: 7 additions & 36 deletions packages/edit-site/src/components/dataviews/filter-summary.js
View file
Open in desktop
Expand Up @@ -12,13 +12,10 @@ import { __, sprintf } from '@wordpress/i18n';
/**
* Internal dependencies
*/
import { OPERATOR_IN } from './constants';
import { unlock } from '../../lock-unlock';
import FilterEnumeration from './filter-enumeration';

const {
DropdownMenuV2: DropdownMenu,
DropdownMenuCheckboxItemV2: DropdownMenuCheckboxItem,
} = unlock( componentsPrivateApis );
const { DropdownMenuV2: DropdownMenu } = unlock( componentsPrivateApis );

export default function FilterSummary( { filter, view, onChangeView } ) {
const filterInView = view.filters.find( ( f ) => f.field === filter.field );
Expand All @@ -43,37 +40,11 @@ export default function FilterSummary( { filter, view, onChangeView } ) {
</Button>
}
>
{ filter.elements.map( ( element ) => {
return (
<DropdownMenuCheckboxItem
key={ element.value }
value={ element.value }
checked={ activeElement?.value === element.value }
onSelect={ () =>
onChangeView( ( currentView ) => ( {
...currentView,
page: 1,
filters: [
...view.filters.filter(
( f ) => f.field !== filter.field
),
{
field: filter.field,
operator: OPERATOR_IN,
value:
activeElement?.value ===
element.value
? undefined
: element.value,
},
],
} ) )
}
>
{ element.label }
</DropdownMenuCheckboxItem>
);
} ) }
<FilterEnumeration
filter={ filter }
view={ view }
onChangeView={ onChangeView }
/>
</DropdownMenu>
);
}
133 changes: 11 additions & 122 deletions packages/edit-site/src/components/dataviews/view-list.js
View file
Open in desktop
Expand Up @@ -37,7 +37,9 @@ import { useMemo, Children, Fragment } from '@wordpress/element';
*/
import { unlock } from '../../lock-unlock';
import ItemActions from './item-actions';
import { ENUMERATION_TYPE, OPERATOR_IN } from './constants';
import { ENUMERATION_TYPE } from './constants';

import FilterEnumeration from './filter-enumeration';

const {
DropdownMenuV2: DropdownMenu,
Expand All @@ -55,7 +57,7 @@ const sortingItemsInfo = {
};
const sortIcons = { asc: chevronUp, desc: chevronDown };

function HeaderMenu( { dataView, header } ) {
function HeaderMenu( { dataView, header, view, onChangeView } ) {
Copy link
Member Author

@oandregal oandregal Nov 24, 2023 •
edited

Choose a reason for hiding this comment

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

What do you all think of using both TanStack (dataView, header) and our own APIs (view, onChange) in HeaderMenu for the time being?

Refactoring view list to use our own APIs is a big PR. We can piece it down by temporarily allowing both things to coexist here and prepare small PRs that update it bit by bit (column filters, sorting, pagination, visibility, search, etc.).

Copy link
Member Author

Choose a reason for hiding this comment

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

If this is not a plan we can agree upon, I'll remove 67d5dec from this PR and work on that separately,

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok with that personally, but let's add a task to follow-up on that and not leave it like that forever.

if ( header.isPlaceholder ) {
return null;
}
Expand Down Expand Up @@ -150,66 +152,11 @@ function HeaderMenu( { dataView, header } ) {
</DropdownSubMenuTrigger>
}
>
{ filter.elements.map( ( element ) => {
let isActive = false;
const columnFilters =
dataView.getState().columnFilters;
const columnFilter = columnFilters.find(
( f ) =>
Object.keys( f )[ 0 ].split(
':'
)[ 0 ] === filter.field
);

if ( columnFilter ) {
const value =
Object.values( columnFilter )[ 0 ];
// Intentionally use loose comparison, so it does type conversion.
// This covers the case where a top-level filter for the same field converts a number into a string.
isActive = element.value == value; // eslint-disable-line eqeqeq
}

return (
<DropdownMenuItem
key={ element.value }
suffix={
isActive && <Icon icon={ check } />
}
onSelect={ () => {
const otherFilters =
columnFilters?.filter(
( f ) => {
const [
field,
operator,
] =
Object.keys(
f
)[ 0 ].split( ':' );
return (
field !==
filter.field ||
operator !==
OPERATOR_IN
);
}
);

dataView.setColumnFilters( [
...otherFilters,
{
[ filter.field + ':in' ]:
isActive
? undefined
: element.value,
},
] );
} }
>
{ element.label }
</DropdownMenuItem>
);
} ) }
<FilterEnumeration
filter={ filter }
view={ view }
onChangeView={ onChangeView }
/>
</DropdownSubMenu>
</DropdownMenuGroup>
) }
Expand Down Expand Up @@ -281,58 +228,6 @@ function ViewList( {
);
}, [ view.hiddenFields ] );

/**
* Transform the filters from the view format into the tanstack columns filter format.
*
* Input:
*
* view.filters = [
* { field: 'date', operator: 'before', value: '2020-01-01' },
* { field: 'date', operator: 'after', value: '2020-01-01' },
* ]
*
* Output:
*
* columnFilters = [
* { "date:before": '2020-01-01' },
* { "date:after": '2020-01-01' }
* ]
*
* @param {Array} filters The view filters to transform.
* @return {Array} The transformed TanStack column filters.
*/
const toTanStackColumnFilters = ( filters ) =>
filters?.map( ( filter ) => ( {
[ filter.field + ':' + filter.operator ]: filter.value,
} ) );

/**
* Transform the filters from the view format into the tanstack columns filter format.
*
* Input:
*
* columnFilters = [
* { "date:before": '2020-01-01'},
* { "date:after": '2020-01-01' }
* ]
*
* Output:
*
* view.filters = [
* { field: 'date', operator: 'before', value: '2020-01-01' },
* { field: 'date', operator: 'after', value: '2020-01-01' },
* ]
*
* @param {Array} filters The TanStack column filters to transform.
* @return {Array} The transformed view filters.
*/
const fromTanStackColumnFilters = ( filters ) =>
filters.map( ( filter ) => {
const [ key, value ] = Object.entries( filter )[ 0 ];
const [ field, operator ] = key.split( ':' );
return { field, operator, value };
} );

const shownData = useAsyncList( data );
const dataView = useReactTable( {
data: shownData,
Expand All @@ -351,7 +246,6 @@ function ViewList( {
]
: [],
globalFilter: view.search,
columnFilters: toTanStackColumnFilters( view.filters ),
pagination: {
pageIndex: view.page,
pageSize: view.perPage,
Expand Down Expand Up @@ -413,13 +307,6 @@ function ViewList( {
onGlobalFilterChange: ( value ) => {
onChangeView( { ...view, search: value, page: 1 } );
},
onColumnFiltersChange: ( columnFiltersUpdater ) => {
onChangeView( {
...view,
filters: fromTanStackColumnFilters( columnFiltersUpdater() ),
page: 1,
} );
},
onPaginationChange: ( paginationUpdater ) => {
onChangeView( ( currentView ) => {
const { pageIndex, pageSize } = paginationUpdater( {
Expand Down Expand Up @@ -469,6 +356,8 @@ function ViewList( {
<HeaderMenu
dataView={ dataView }
header={ header }
view={ view }
onChangeView={ onChangeView }
/>
</th>
) ) }
Expand Down