-
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: make filters footprint more condensed #56983
Conversation
Size Change: +472 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
Flaky tests detected in 8482576. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7196854806
|
Nice, thank you for working on this. A couple of small requests:
I think there may be a case to remove the "Reset filters" button now that we have one in the filter menu, but we can probably look at that separately. |
Done these two. I've also added an item to reset the individual filter. |
There are a couple of e2e test failures that are unrelated to this PR, and I'm fixing at #56992 |
3b09285
to
accafc8
Compare
@jameskoster would you be able to jump in and tweak any styles you think that need adjustment? Otherwise, I'd think this is ready. I've pushed some changes to:
Gravacao.do.ecra.2023-12-13.as.11.17.18.mov |
I'm working on adding the list layout to templates at #57014 That PR'd benefit from landing this one first, though it's not a hard requirement. |
Pushed a couple of styling tweaks. There are a couple of minor issues it would be good to address:
count.mp4 |
Pushed a fix for both: Gravacao.do.ecra.2023-12-13.as.13.11.44.mov |
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.
Thank you, this is looking good :)
47770ac
to
8482576
Compare
> | ||
<Icon icon={ plus } style={ { flexShrink: 0 } } /> | ||
{ __( 'Add filter' ) } | ||
{ view.type === LAYOUT_LIST && filterCount > 0 ? ( |
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 feels like it's better as a prop like showFilterCount
.
@@ -73,6 +73,10 @@ function WithSeparators( { children } ) { | |||
} | |||
|
|||
export default function FilterSummary( { filter, view, onChangeView } ) { | |||
if ( view.type === LAYOUT_LIST ) { |
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.
Similar with the above comment. it feels to me we should not make such explicit checks. Maybe this are kind of a config of a layout(showFilterSummary, etc..)? 🤔 This also applies to resetFilter
.
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.
When you suggest making it a prop, are you thinking of lifting it up all the way to the DataViews
component for consumers to configure?
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.
Not sure if it's what you were looking for, but since you mentioned it, I revisited this code and prepared a follow-up to lift the visibility control to the parent #57056
Co-authored-by: James Koster <[email protected]>
Part of #55083
Related #55100
What?
Implements a more condensed filter UI. It also updates its behavior to be able to do any filter operation (change value, change operator).
Gravacao.do.ecra.2023-12-13.as.11.17.18.mov
Why?
Some layouts such as list have a narrower space for information, and we'd like to condense it.
This is the first step of implementing #55100 (comment)
How?
AddFilter
component UI and behavior.Testing Instructions