-
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
Patterns: Add category selector to pattern creation modal #55024
Conversation
Size Change: +6.23 kB (0%) Total Size: 1.65 MB
ℹ️ View Unchanged
|
Flaky tests detected in 862078f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6429768639
|
} | ||
); | ||
|
||
const categoryMap = useMemo( () => { |
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.
One question with merging is do we dedupe by label or slug and do we prioritise the core label/slug combinations over the user ones, or the other way around?
I think we should dedupe by label as below as that is what the user sees, so avoids confusion as currently, the likes of the core Headers/header
label slug combination could lead to a user adding a Headers/headers
combination and two Headers
labels appearing. But, I don't have a strong opinion on whether the users categories should have priority over core or not.
Not sure if doing a ! uniqueCategories.has( category.label ) &&
check is any less/more performant than just doing an unnecessary uniqueCategories.set
to avoid duplicates - with the small number of categories I am guessing 6 of one, half a dozen of the other.
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 don't have a preference regarding deduping the labels or the slugs.
I don't think the micro-optimization matters here for performance until proven otherwise either. 👍
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 think we should dedupe by label as below as that is what the user sees
This logic makes sense to me 👍
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.
Works well for me! The test error seems unrelated.
const categories = [ | ||
...userPatternCategories, | ||
...corePatternCategories, | ||
].reduce( ( uniqueCategories, category ) => { | ||
if ( | ||
! uniqueCategories.has( category.label ) && | ||
// There are two core categories with `Post` label so explictily remove the one with | ||
// the `query` slug to avoid any confusion. | ||
category.name !== 'query' | ||
) { | ||
// We need to store the name separately as this is used as the slug in the | ||
// taxonomy and may vary from the label. | ||
uniqueCategories.set( category.label, { | ||
label: category.label, | ||
value: category.label, | ||
name: category.name, | ||
} ); | ||
} | ||
return uniqueCategories; | ||
}, new Map() ); |
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, non-blocking: Personally I would think a for
loop could be easier to understand, but I can read this just fine too 👍 .
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.
Most people reading the code are probably still more use to simple for
loops but I could be biased given that's my preference given too many use generic variable names in reduce
e.g. accumulator
. That's obviously not the case here though.
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 quite like the slightly more self-contained nature of the .reduce for this sort of thing, but for the sake of quick readability let's switch it to a forEach - done.
2ee82db
to
01bca09
Compare
have rebased, hopefully that fixes it. |
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 the improvement here @glendaviesnz 👍
In general, it tests as advertised for me.
Screen.Recording.2023-10-05.at.3.53.48.pm.mp4
There's a small jump in modal height when selecting a category from the suggestions. Adding a second category also slightly increases the width of the modal.
If we can, we should probably tighten that up a little.
Screen.Recording.2023-10-05.at.3.58.08.pm.mp4
} | ||
); | ||
|
||
const categoryMap = useMemo( () => { |
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 think we should dedupe by label as below as that is what the user sees
This logic makes sense to me 👍
const categories = [ | ||
...userPatternCategories, | ||
...corePatternCategories, | ||
].reduce( ( uniqueCategories, category ) => { | ||
if ( | ||
! uniqueCategories.has( category.label ) && | ||
// There are two core categories with `Post` label so explictily remove the one with | ||
// the `query` slug to avoid any confusion. | ||
category.name !== 'query' | ||
) { | ||
// We need to store the name separately as this is used as the slug in the | ||
// taxonomy and may vary from the label. | ||
uniqueCategories.set( category.label, { | ||
label: category.label, | ||
value: category.label, | ||
name: category.name, | ||
} ); | ||
} | ||
return uniqueCategories; | ||
}, new Map() ); |
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.
Most people reading the code are probably still more use to simple for
loops but I could be biased given that's my preference given too many use generic variable names in reduce
e.g. accumulator
. That's obviously not the case here though.
Thanks @aaronrobertshaw , will try and get that tidied up tomorrow |
|
||
return { | ||
corePatternCategories: getBlockPatternCategories(), | ||
userPatternCategories: getUserPatternCategories(), |
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.
Isn't it a bit weird that we have two functions + two REST APIs to retrieve pattern categories. Do we need to unify these at some point? I mean "core" and "user" can be query filters no? I guess things have evolved organically but maybe we should take a step back and assess whether the current APIs and storage we have is the right one for these things?
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.
Yes, this is a little weird, but we used the existing core taxonomy API endpoint for the user categories as a quicker path to get this into 6.4 with the shorter release cycle.
The plan is that now the user-created patterns and categories are in place to look at refactoring how these are retrieved and stored. As well as the category handling there is a bit of duplication between the site editor and block editor with how the patterns are handled so it would be good to see how we can abstract that away a bit. But we need to work out how best to do that while still keeping the block-editor uncoupled from the core data.
I fixed this, along with the width jump when adding multiple categories, in 906df26. Kapture.2023-10-06.at.13.16.26.mp4 |
Thanks for fixing the jumps @kevin940726. I'd say we're pretty close though another small issue has been introduced. The suggestions list is now wider than the category input and looks a bit off. |
Thanks for noticing it! I fixed in 862078f, but it's getting trickier with lots of overridden styles 😅 . |
Thanks @kevin940726 this tested well for me in terms of the UI being stable now when selecting categories. @aaronrobertshaw if you are happy with this now can you please merge it first thing Monday so it is ready for the beta 3 build which is happening on 9 Oct, thanks. |
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.
Looking closer at the form token field, its __next40pxDefaultSize
prop actually makes it 42px
tall. When it is set it makes the inner elements 40px high instead of the overall component.
To me, that's a bug for the component itself and should probably be addressed there as a follow-up.
I don't think that needs to prevent landing this to address the specific feedback for 6.4 though.
P.S. Additionally, while updating the form token field component we might need to provide a means of apply a classname to the suggestion list so as to avoid needing to use the internal component classname to make overrides such as those in this PR.
--------- Co-authored-by: Kai Hao <kai@kaihao.dev>
I just cherry-picked this PR to the 6.4-beta3 branch to get it included in the next release: 9398b4a |
* Add clearer labels and context to BlockPatternsSyncFilter (#54838) * Add help text & clearer labeling * Theme & Plugins * Font Library: use snake_case instead of camelCase on fontFamilies endpoint param (#54977) * use snake_case instead of camelCase on endpoint param * updating test * Fix output of Navigation block classnames in the editor. (#54992) * Block Editor: Avoid double-wrapping selectors when transforming the styles (#54981) * Block Editor: Avoid double-wrapping selectors when transforming the styles * Include space in the check * Don't display the navigation section in template parts details when a menu is missing (#54993) * Scripts: Properly use CommonJS for default Playwright config (#54988) * Fix path to `globalSetup` in default Playwright config Oversight from #54856 * `module.exports` * Fix default export usage * Add template replace flow to template inspector (#54609) * Add a modal to allow template switching * fetch template info * Allow switching to different patterns * Allow switching to different patterns * Add columns * move availble templates to the actions * filter for the correct templates * create the right data structure in the use select * move to a hook * inject theme attribute into pattern again * put the overlay over the top of the dropdown * fix the pattern to templates hook * set the template on click * Also set the blocks * remove calls to set template with the current template, since setting blocks correctly updates the content in the editor * serialize blocks so that we have correctly processed template parts * remove duplicated code * Remove unnecessary mapping * refactor * memoize the patterns * combine the useSelect * Update packages/edit-site/src/components/sidebar-edit-mode/page-panels/hooks.js Co-authored-by: Andrei Draganescu <me@andreidraganescu.info> * Fix ESLint error * Only show the button is there is more than 1 pattern * Copy update * Move the hook to a subdir * check that there are patterns * move the check * remove useCallback * change condition to show the button * change condition * move to use editEntityRecord * combine filters * add comments * Update packages/edit-site/src/components/sidebar-edit-mode/template-panel/replace-template-button.js Co-authored-by: Andrei Draganescu <me@andreidraganescu.info> --------- Co-authored-by: Andrei Draganescu <andrei.draganescu@automattic.com> Co-authored-by: Andrei Draganescu <me@andreidraganescu.info> Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com> * List View: Fix performance issue when selecting all blocks (#54900) * List View: Fix performance issue when selecting all blocks within the editor canvas in long posts * Add a comment, rename const * Move block focus to be performed only once at the root of the list view, instead of within each block * Fix left and right aligmnent in children of Post Template (#54997) * Fix left and right aligmnent in children of Post Template * Add align center styles * Fix image placeholder disappearing * Site Editor: Avoid stale navigation block values when parsing entity record (#54996) * Removed unwanted space from the string (#54654) * Update styles.js Removed unwanted space with translation * Update deleted-navigation-warning.js Unwanted space at the end of the string shows translation warning. * Update inspector-controls.js Unwanted space at the end of the string shows translation warning * Fix Deleted Navigation Menu warning string (#55033) * [Inserter]: Fix reset of registered media categories (#55012) * [Inserter]: Fix reset of registered media categories * convert `useInserterMediaCategories` to selector and make private * Try fixing the flaky 'Toolbar roving tabindex' e2e test (#54785) * Try fixing the flaky 'Toolbar roving tabindex' e2e test * Add a link in the comment * Fallback to Twitter provider when embedding X URLs (#54876) * Fallback to Twitter provider when embedding X URLs * Avoid processing empty urls when trying a different provider * Update `react-native-editor` changelog # Conflicts: # packages/react-native-editor/CHANGELOG.md * Based on the efforts in #51761, remove caps case from Template Part and prefer sentence case. As all instances of this string are stand alone, it's okay to have Template capitalized as it's the start of a sentence. (#54709) * Update pattern import menu item (#54782) * Update pattern import menuitem * Revert label * Image Block: Fix browser console error when clicking "Expand on Click" (#54938) * Patterns: Remove category description in inserter panel? (#54894) * Media & Text: Fix React warning (#55038) * Block Style: Display default style labels correctly in the block sidebar (#55008) * Site Editor: Do not display 'trashed' navigation menus in Sidebar (#55072) * Site Editor: Do not display 'trashed' navigation menus in Sidebar * Extract selector into a hook Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> --------- Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> * Image: Fix Lightbox display bug in Classic Themes. (#54837) * If current theme is not a block theme add a default value for $background_color and $close_button_color. * Set lightbox buttons' background & border to none on hover & focus * Change logic to support lightbox in classic themes * Update logic to avoid unnecessary calls * Add style fixes * Remove unnecessary code * Fix close button color --------- Co-authored-by: Mario Santos <santosguillamot@gmail.com> Co-authored-by: Ricardo Artemio Morales <ric.morales22@gmail.com> * Latest Posts: add screen reader title text to Read more links and use alternative to excerpt_more filter (#55029) * In the editor: adds a screen reader text span with the post title in the i18n interpolator In the frontend: removes excerpt_more filter so we don't override themes and also replaces the default ellipsis with an accessible read more link * Removing "of" preposition in favour of a semi-colon. "Read more" is already translated so using a specifier to add it to the string * Fix Image block lightbox missing alt attribute and improve accessibility (#55010) * Move lightbox open button after the image. * Fix getting the lightbox image alt attribute. * Improve docblocks. * Do not render empty role attribute. * Remove unnecessary aria-hidden attribute. * Set aria-modal attribute dynamically. * More meaningful and simpler modal dialog aria-label. * Increase Close button target size. * Add enlarged image base64 encoded placeholder. * Better check for alt attribute as a string. * Update changelog. * Move changelog entry to the block library changelog. * Set lightbox dialog aria-label dynamically. * Hide background scrim container from assistive technology. * Remove obsolete code --------- Co-authored-by: Ricardo Artemio Morales <ric.morales22@gmail.com> # Conflicts: # packages/block-library/CHANGELOG.md * Patterns: Add category selector to pattern creation modal (#55024) --------- Co-authored-by: Kai Hao <kai@kaihao.dev> * Iframe: Fix positioning when dragging over an iframe (#55150) * Site Editor: Fix template part area listing when a template has no edits (#55115) * Alternative: Fix template part area listing when a template has no edits * Fix typos --------- Co-authored-by: Rich Tabor <hi@richtabor.com> Co-authored-by: Matias Benedetto <matias.benedetto@gmail.com> Co-authored-by: tellthemachines <tellthemachines@users.noreply.github.com> Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com> Co-authored-by: Pascal Birchler <pascalb@google.com> Co-authored-by: Ben Dwyer <ben@scruffian.com> Co-authored-by: Andrei Draganescu <andrei.draganescu@automattic.com> Co-authored-by: Andrei Draganescu <me@andreidraganescu.info> Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com> Co-authored-by: mujuonly <muju.only@yahoo.in> Co-authored-by: Dave Smith <getdavemail@gmail.com> Co-authored-by: Nik Tsekouras <ntsekouras@outlook.com> Co-authored-by: Carlos Garcia <fluiddot@gmail.com> Co-authored-by: Ramon <ramonjd@users.noreply.github.com> Co-authored-by: James Koster <james@jameskoster.co.uk> Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com> Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> Co-authored-by: Michal <mmczaplinski@gmail.com> Co-authored-by: Mario Santos <santosguillamot@gmail.com> Co-authored-by: Ricardo Artemio Morales <ric.morales22@gmail.com> Co-authored-by: Andrea Fercia <a.fercia@gmail.com> Co-authored-by: Glen Davies <glendaviesnz@users.noreply.github.com> Co-authored-by: Kai Hao <kai@kaihao.dev>
What?
Adds a list of available categories to select from to the pattern creation modal category selection field.
Why?
It was noted in 6.4 beta testing that the simple tag input box is confusing to users and potentially difficult for them to use, potentially causing a lot of additional categories being added, etc.
How?
Uses the
FormTokenFields
__experimentalExpandOnFocus
andsuggestions
props to show a select list of existing categories when the category field is focusedTesting Instructions
/wp-admin/edit-tags.php?taxonomy=wp_pattern_category
Create pattern
optionHeaders
option and save the pattern/wp-admin/edit-tags.php?taxonomy=wp_pattern_category
and make sure category was added with name ofHeaders
but slug ofheader
Screenshots or screencast
pattern-categories-select.mp4