-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
DataViews: extract FilterEnumeration
component
#56514
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 } ) { | ||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||
); | ||
} ); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
|
@@ -55,7 +57,7 @@ const sortingItemsInfo = { | |
}; | ||
const sortIcons = { asc: chevronUp, desc: chevronDown }; | ||
|
||
function HeaderMenu( { dataView, header } ) { | ||
function HeaderMenu( { dataView, header, view, onChangeView } ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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.). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
|
@@ -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> | ||
) } | ||
|
@@ -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, | ||
|
@@ -351,7 +246,6 @@ function ViewList( { | |
] | ||
: [], | ||
globalFilter: view.search, | ||
columnFilters: toTanStackColumnFilters( view.filters ), | ||
pagination: { | ||
pageIndex: view.page, | ||
pageSize: view.perPage, | ||
|
@@ -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( { | ||
|
@@ -469,6 +356,8 @@ function ViewList( { | |
<HeaderMenu | ||
dataView={ dataView } | ||
header={ header } | ||
view={ view } | ||
onChangeView={ onChangeView } | ||
/> | ||
</th> | ||
) ) } | ||
|
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.
I like that we now have a
FilterEnumeration
component. I imagine in the future we could have aFilterString
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.
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.
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.
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.
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:
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.
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?
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.
I feel we should ship this one as it is and iterate on UI after. Everything (including updating UI) becomes easier.
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.
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.