DataViews: in patterns page, show sync status filter by default #58367
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
oandregal
requested review from
mcsf,
youknowriad,
jameskoster,
jorgefilipecosta,
ntsekouras and
annezazu
oandregal
added
[Type] Enhancement
|
Size Change: +156 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 4d78f9c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7709201422
|
jameskoster
approved these changes
Jan 29, 2024
oandregal
force-pushed
the
update/patterns-show-sync-filter-by-default
branch
from
5560670 to
8d481a3
Compare
mcsf
reviewed
Jan 29, 2024
I pushed 956ee77ab0 as a suggestion, but feel free to pop it out. :)
Follow-up question: when the Reset Filters button is pushed, the sync status filter disappears completely. That's to be expected prior to this PR, but should we aim to keep it somehow? Or not worth the complexity? |
How would users remove them from the view then? Note that this is how filters work (allow "unset" filters to be visible). The PR just makes use of it in a default view. See, for example, filters in pages: Gravacao.do.ecra.2024-01-29.as.16.38.22.mov |
mcsf
force-pushed
the
update/patterns-show-sync-filter-by-default
branch
from
956ee77 to
53fc637
Compare
|
More context about the reset button: the original design for each "filter summary" had a |
My logic was that there is nothing to remove: no "real" filter is active, we are just giving prominence in the UI to a particular filter that the user might be interested in. Here's how it behaves: Screen.Recording.2024-01-29.at.15.43.17.movBut the last thing I wanted was to make this PR more confusing, so I'm perfectly happy if my suggestion is dismissed! |
Yeah, I'm not convinced about the change in reset behavior: users would have to "select" a value to be able to remove the filter from the UI. Thoughts @jameskoster? |
|
Fair enough, feel free to revert :) |
|
Really, really good points, thank you for bringing this up. I suspect the right course of action here depends on how we want the "Reset" button to behave...
If we want the former, then the original PR was probably the correct implementation. That said, I lean towards the latter and @mcsf's suggestion seems to get us most of the way there. In addition I'd propose that the Reset action restores the initial state rather than removing all filters as it does currently. In other words, when a filter is included by a consumer in the code it cannot be removed via the UI, only unset. Github Issues seems like a reasonable model to emulate. There you find that the reset action only becomes available when you filter by author, label, etc., and that those filter controls remain visible even after resetting. |
So glad to see this being followed up on! I just want to +1 this. I think it'll be confusing to folks to have it entirely disappear from the UI by hitting reset. |
This reverts commit 53fc637.
|
I've done the following:
If 58427 is not shipped, we can use this PR as fallback. |
mcsf
approved these changes
Jan 30, 2024
ntsekouras
pushed a commit
that referenced
this pull request
Feb 1, 2024
* Declare page patterns sync-status as a primary filter * Implement primary filter APi * Hide reset when there is only 1 primary filter and none secondary filter * Improve comment * Add author as primary filter for pages for testing * Simplify and take into account list layout condensed view * Simplify * Revert "DataViews: in patterns page, show sync status filter by default (#58367)" This reverts commit 0124e7e. * Document isPrimary * Improve readibility: use filter array method * Improve readibility * Show reset filters if there is one primary filter * Account for primary filters in reset button logic * Show all filters (primary or secondary) in the AddFilter component * Account for primary filters in reset button logic (AddFilter) * AddFilter: remove search condition from Reset enability, as it is not cleared
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of #55083
See alternative at #58427
What?
Makes the sync filter visible, but with no option selected by default.
Users can still remove it by "resetting the filters".
Why?
To give the sync status filter more prominence.
How?
Configures the default view to have it present.
Testing Instructions