-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
De-emphasise pattern filters in inserter #54681
Conversation
Size Change: +317 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
Thanks @SaxonF this looks like a nice fixe for the UX issues with merged patterns tabs filtering options. One thing that would be good to sort is to disable the sync filter options if the Theme patterns option is selected as they don't apply in that case. |
Flaky tests detected in a75cccf. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6270644465
|
This PR rectifies some issues that have been identified with the UX changes that were made during the introduction of the new user pattern categories feature into the inserter patterns tab, so we think it is important that these fixes go into the 6.4 release. |
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 new direction definitely feels much better @SaxonF, nice work ✨
I ran into one issue with the filtering of patterns. Now that the source and sync status filters were combined into a single control the original filtering logic needed tweaking. I pushed a fix for that and a few minor nits to clean up some unused things.
One thing that would be good to sort is to disable the sync filter options if the Theme patterns option is selected as they don't apply in that case.
I explored two options here and neither seemed great.
Disabling the sync status filters appears heavy-handed given two two-thirds of the source options might cause them to be disabled. At least if we are looking to match something like what is on trunk where the sync status filtering is only available when the user source is selected.
Omitting the sync status options entirely unless the user source is selected seems worse still and is probably not just an a11y issue but general UX problem as well as a user would have to reopen the filter dropdown to see the sync filter options.
Leaving the status quo isn't great either so I've tweaked the filtering further to consider theme and directory patterns as unsynced. Let me know if that suits.
Copy needs some work.
I came up empty for alternatives to Theme
that were both concise and covered the fact the patterns include theme and directory patterns.
After discussing some possibilities, I've split theme and directory patterns into two separate filters and updated the required logic for that as well.
The new copy needs some thinking as well though 🙂
Also, it is worth noting that the the "directory" source currently covers both the directory and core patterns. If we were to split those too, the dropdown is starting to grow considerably.
This will need a thorough round of testing but unfortunately, I'm out of time today and will have to leave iterating on this until tomorrow.
Here's a glimpse of the current state of the filtering:
Screen.Recording.2023-09-21.at.5.53.16.pm.mp4
If we open the popover to the right of the filter button, keep it open as you select options, and make the panel header sticky, then you can effectively filter and browse at the same time. Could be worth a try? patterns.mp4The "Theme" category also includes patterns added by plugins, so the name may need to be adjusted. We might move patterns bundled with core to this category and call it something like "System – bundled with WordPress, active plugins & theme". Though I acknowledge "System" sounds a bit technical, so alternatively we could have dedicated categories for WordPress, Theme, and Plugins. Or perhaps two categories: WordPress, Theme & Plugins. Directory could have a description like "Created by the WordPress community", and User can follow that convention; "Created by members of this site". Agree the synced filter experience is a bit awkward given only one category can contain synced patterns (for now). I don't suppose it's trivial, but could the inclusion of a count help? Then at least you know the filter will return no results without having to click it. |
I have pushed a suggested change that disables the sync filters to see if this helps to reduce some confusion by not allowing users to select invalid combinations. There is not 100% agreement on whether this is the best approach, but it can be easily reverted, and after discussion with @aaronrobertshaw and @SaxonF it was decided to go with this approach for now. It can easily be changed if further testing shows that it adds to user confusion rather than reducing it. |
Have pushed another change that keeps the menu open while changing options and the sticky header as @jameskoster. |
86f60fd
to
42e8aa1
Compare
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 for the continued iterations @glendaviesnz and @kevin940726, this is looking good 👍
In particular, the recent updates to the scroll-to-top functionality in the patterns tab are working nicely.
Screen.Recording.2023-09-22.at.5.41.41.pm.mp4
we could have dedicated categories for WordPress, Theme, and Plugins. Or perhaps two categories: WordPress, Theme & Plugins.
There are still the above tweaks around the source filters we can make but perhaps those are better left to a follow-up so we can be sure the bulk of these UI fixes/improvements aren't delayed.
What do you all think?
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.
LGTM 👍
Just a couple of suggestions which I can help fix later!
packages/block-editor/src/components/inserter/block-patterns-explorer/patterns-list.js
Outdated
Show resolved
Hide resolved
import { Icon } from '@wordpress/icons'; | ||
import { useState } from '@wordpress/element'; | ||
|
||
export const PATTERN_TYPES = { |
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.
Nit: I'm not sure if we want to reuse the same constants in edit-site
or patterns
? That might not be trivial as the block-editor
might not be able to depend on those packages. I wonder if tree-shaking helps here, but I doubt it since we have side effects in both packages 🤔 . c.c. @ramonjd if you have any feedback!
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 plan when we added the new constants to the patterns package was to then tidy up the block editor to use the same constants, but it is the editor that currently depends on the patterns package rather than the block editor so not sure if this will be possible - it would be good to try and work out how all the pattern related constants can be centralised somehow.
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.
That might not be trivial as the block-editor might not be able to depend on those packages.
Yeah, that's the reason I didn't touch these either. 😄
To be honest, my only motivation was to clean up the site editor package. There were some constants being used, others duplicated etc. I think it's okay to have some dupes across packages, but the site editor needed some spring cleaning IMO
packages/block-editor/src/components/inserter/block-patterns-filter.js
Outdated
Show resolved
Hide resolved
* de-emphasise pattern filters in inserter * Remove obsolete props from PatternListHeader * Remove unnecessary dependencies comment * Remove unused pattern source filters * Fix pattern filtering logic * Split theme patterns from core and directory sourced ones * Allow theme and directory patterns as unsynced * Disable the sync filter options if the pattern source is not user * Add constants for different all values * Keep the menu open while toggling options * Add sticky header * Fix padding and move pagination * Fix pagination scroll to top * Reset pagination if filter changes * Revert the update to paging scroll to top * Fix scrolling-to-top after page changes * Fix scrolling-to-top after filter changes * Scroll to top on category change * Fix scroll-to-top on category change * Derive pattern sync menu options * Update the translators comment --------- Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> Co-authored-by: Glen Davies <glen.davies@automattic.com> Co-authored-by: Kai Hao <kai@kaihao.dev>
I just cherry-picked this PR to the release/16.7 branch to get it included in the next release: 2831076 |
What?
Moves all filtering into a dropdown and removes from pattern explorer for now. Also adds a sticker header so the menu stays in place while scrolling
Why?
#54620
The way we filter patterns and highlight different types needs some more time to refine the designs. In the near term we can move filtering into a more discrete place for power users.
How?
Consolidate filtering into single file and move it to the top of the pattern list panel.
Testing Instructions
Screen recording
new-pattern-filters.mp4