Template part block: Add category panel #29159
Conversation
david-szabo97
added
[Type] Feature
david-szabo97
requested review from
vindl,
jeyip,
mattwiebe,
Addison-Stavlo,
Copons and
jameskoster
| <PanelBody title={ __( 'Category' ) }> | ||
| <SelectControl | ||
| label={ __( 'Select template part category' ) } | ||
| labelPosition="top" |
There was a problem hiding this comment.
Any idea why labelPosition="top" is not working? 🤔 The label is still positioned on the side.
There was a problem hiding this comment.
I hope you don't mind a pushed a patch here: ffa154b
I'm not sure if this is the right way of handling this though, as it seems this PR: #28609 introduced this 'conflict' (? 🤔 ). @sarayourfriend @ItsJonQ any feedback here?
There was a problem hiding this comment.
Root correctly applies the CSS based on the labelPosition:
But for some reason a CSS class (which seems to be the Flex component) overrides it:
This is interesting because Flex's styled CSS class should come BEFORE Root. Root extends Flex:
Oh, and it works fine in Storybook. So it's something with the Editor itself?
There was a problem hiding this comment.
I think the issue is that we're using two different styled functions and the specificity of the Root selector is less than the base Flex selector. There's one that comes from @wp-g2/styles that I think using would fix this as it adds some specificity rules. @ItsJonQ what's the intended solution in this situation?
Do we actually need the extra specificity? I wonder what would happen if we did without it.
There was a problem hiding this comment.
As @sarayourfriend has pointed out, the issue is indeed caused by the 2 ways the CSS-in-JS styles are being compiled. Something we need to smooth out for the Component System integration.
Do we actually need the extra specificity? I wonder what would happen if we did without it.
We do. Even before the next Component System (G2), we had to resort to hacks like &&& for certain components just so they would render correctly:
My intuition tells me that we need to adjust some props in the input-control/input-base.js to ensure it plays nicely with the new Flex component (rather than a CSS fix).
There was a problem hiding this comment.
Update! I have a working solution. It involves remapping props from input-control to feed into the new flex component. Will create a PR shortly
|
Size Change: +179 B (0%) Total Size: 1.38 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks for working on this @david-szabo97 !
I left some comments but the main functionality was tested and works 👍
packages/block-library/src/template-part/edit/category-panel.js
Outdated
Show resolved
Hide resolved
| ); | ||
|
|
||
| return ( | ||
| <PanelBody title={ __( 'Category' ) }> |
There was a problem hiding this comment.
I'm not sure this should be in its own panel and with this title. Same remark about the below label Select template part category. I think we want to avoid template part wording and maybe just Area should be okay? 🤔
@mtias
There was a problem hiding this comment.
For now, I followed @jameskoster 's mockup that you can find here: #27875
|
Thinking things through a little more... I am not sure that the title + area options should be so prominent in this context. It could easily mislead users into believing that any changes they make here are local to the parent template. I know the multi-entity save flow clears that up, but that might be a bit late... Therefore, I'm thinking it might be better to move these options to the "Advanced" section:
The prominence could perhaps increase when editing a template part in isolation... something like:
But that is something to explore in more detail elsewhere (#29148). This is possibly tangential, but should the html element change when you switch area? IE if I switch from Header to Footer, should the tag also change from |
IMO, this makes sense. This is more like a power-user feature anyway.
I'd say this is OK if the current area matches the current tag. IE, area = header, tag = header, but I when switch to area = footer then tag becomes footer too. But if area = header, tag = div, then I switch to area = footer then tag should stay div. Maybe that's too smart? |
I think it makes perfect sense to do exactly what you described. |
|
Agreed with placing them in "Advanced". I think "area" needs some contextual explanatory text. Ideally we could also use the icons on the select element. Considering we might just have 3 or 4 canonical areas. It might make sense to list them as icons in a segmented control like the toolbar? |
I think it makes sense to only set a tag based on the semantic area if a tagName attribute isn't set. So maybe we add an 'area default' to the tag name dropdown (and maybe add that default value in parens) that corresponds to the tagName being unset. Then when no tagname is set, we default to that value that corresponds to the semantic area. |
|
We could use the existing icon buttons to create a simple segmented control:
Or we might create something more elaborate that can also be used in #29222
The helper text needs optimisation. |
It looks nice! Im not sure how many different 'areas' we plan to support in the future, but if we add a handful that second one with the large icons might become a bit weighty. |
|
It could probably scale in rows – like the Inserter. But yeah, the simpler implementation might be the way to go to begin with. |
|
The first one looks nice to me :) |
|
I prefer it when the text with the area name is visible, or even the select list. |
david-szabo97
force-pushed
the
add/category-panel-to-template-parts
branch
from
f0a8988
to
23adb49
Compare
|
This is working well! |
Maybe it makes sense to start with the dropdown list in this PR. And explore the icon designs in follow up? |
I agree, let's keep them separate. Easier to review and discuss as well. |
| @@ -91,31 +93,12 @@ export default function TemplatePartEdit( { | |||
|
|
|||
| return ( | |||
| <> | |||
| <InspectorControls> | |||
There was a problem hiding this comment.
Nice call moving all this to another file. Its definitely a bit more organized now.
| const advancedPanelXPath = `//div[contains(@class,"interface-interface-skeleton__sidebar")]//button[@class="components-button components-panel__body-toggle"][contains(text(),"Advanced")]`; | ||
| const advancedPanel = await page.waitForXPath( advancedPanelXPath ); | ||
| await advancedPanel.click(); |
There was a problem hiding this comment.
I wonder if current e2e utils ensureSidebarOpened, findSidebarPanelToggleButtonWithTitle, findSidebarPanelWithTitle, may be useful here?
There was a problem hiding this comment.
Would be great, but just checked them and they are only working in edit-post context 😞
There was a problem hiding this comment.
are only working in edit-post context
Isn't that the context this function is being run in? It looks like it is used in test setup in before all, after createNewPost and before siteEditor.visit(). Similarly it uses openDocumentSettingsSidebar a few lines above this which is also reliant on edit-post* selectors.
| const AREA_TAGS = { | ||
| footer: 'footer', | ||
| header: 'header', | ||
| unactegorized: 'div', |
There was a problem hiding this comment.
Typo 😬
| unactegorized: 'div', | |
| uncategorized: 'div', |
There was a problem hiding this comment.
Or maybe we should consider renaming to something else, now that we're using "area" rather than "category.
Maybe unassigned? 🤔
There was a problem hiding this comment.
Some template parts need not be assigned to a specific area, but "uncategorized" or "unassigned" might make them feel miss-placed or unorganised. That doesn't feel like the appropriate tone.
Perhaps something like "General" or "Universal"?
There was a problem hiding this comment.
"uncategorized" or "unassigned" seems appropriate in the case that an item exists and this value has never been assigned or distinguished. Or that no other existing value settings are appropriate.
"General" and "Universal" could definitely be 'areas', but they do not fit the same purpose as uncategorized/unassigned. We cannot assume something is "Universal" just because it doesn't fit into the existing categories, and when an item is not universal and does not fit into existing categories something like unassigned/uncategorized serves a purpose.
There was a problem hiding this comment.
#29937 to fix the typos.
Perhaps something like "General" or "Universal"?
I just realized/remembered on the user facing side, 'uncategorized' is being displayed as "General".
Description
Add category panel to template part block. This allows us to change the assigned area of the template part.
Related: #29152
How has this been tested?
Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist: