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: align filter implementations #57059
Conversation
onChangeView( | ||
( currentView ) => ( { | ||
...currentView, | ||
{ filter.elements.map( ( element ) => { |
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.
Three refactorings (no behavioral changes) in this component:
- extracted
otherFilters
- extracted
isActive
- used
onChangeView({ ... })
instead ofonChangeView( ( currentView ) => { ... })
.
Size Change: -51 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
@@ -74,9 +74,13 @@ function WithSeparators( { children } ) { | |||
|
|||
export default function FilterSummary( { filter, view, onChangeView } ) { | |||
const filterInView = view.filters.find( ( f ) => f.field === filter.field ); | |||
const otherFilters = view.filters.filter( |
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.
Three refactorings (no behavioral changes) in this component:
- extracted
otherFilters
- extracted
isActive
- used
onChangeView({ ... })
instead ofonChangeView( ( currentView ) => { ... })
.
@@ -161,7 +153,19 @@ function HeaderMenu( { field, view, onChangeView } ) { | |||
<DropdownSubMenuTrigger | |||
prefix={ <Icon icon={ funnel } /> } | |||
suffix={ | |||
<Icon icon={ chevronRightSmall } /> | |||
<> |
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.
@@ -195,12 +191,13 @@ function HeaderMenu( { field, view, onChangeView } ) { | |||
onSelect={ () => { | |||
onChangeView( { | |||
...view, | |||
page: 1, |
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.
This was a bug: the pagination was not reset upon filter changes.
OPERATOR_IN && ( | ||
<Icon icon={ check } /> | ||
) | ||
} | ||
onSelect={ () => | ||
onChangeView( { | ||
...view, | ||
page: 1, |
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.
This was a bug: the pagination was not reset upon filter changes.
OPERATOR_NOT_IN && ( | ||
<Icon icon={ check } /> | ||
) | ||
} | ||
onSelect={ () => | ||
onChangeView( { | ||
...view, | ||
page: 1, |
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.
This was a bug: the pagination was not reset upon filter changes.
} | ||
const operators = columnOperators.filter( ( operator ) => | ||
[ OPERATOR_IN, OPERATOR_NOT_IN ].includes( operator ) | ||
|
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.
This was rewritten to make it clear and follow the naming schema of filters (filter
, filterInView
, activeElement
, and activeOperation
).
@@ -171,17 +175,9 @@ function HeaderMenu( { field, view, onChangeView } ) { | |||
<WithSeparators> | |||
<DropdownMenuGroup> | |||
{ filter.elements.map( ( element ) => { | |||
let isActive = false; | |||
if ( filterInView ) { | |||
// Intentionally use loose comparison, so it does type conversion. |
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 could no longer reproduce this issue, so this changed to strict comparison like the other places.
Flaky tests detected in 774a70b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7208671143
|
activeElement?.value === | ||
element.value; | ||
return ( | ||
<DropdownMenuItem |
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.
Apologies if this was already discussed, but is there a reason why we're not using DropdownMenuItemRadio
? It should provide the right semantics out of the box, including showing/hiding the icon
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.
The issue was that radio items cannot be unset after they've been set, which is not the behavior we want here (user should be able to unset the value).
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.
Got it, thank you for the reminder
Part of #55083
Related #56514 and #56479 (comment)
What?
This PR:
Why?
Fixes bugs.
In the past, I tried extracting a common UI component at #56514 though it wasn't merged. At this point, the three implementations share the same logical model but vary in UI/interactions slightly (what's the trigger for the filter, whether it should stay open or close on clicking, copy, etc.).
Testing Instructions