Group block: use a variation picker in the placeholder #43496
Conversation
|
Size Change: +1.44 kB (0%) Total Size: 1.3 MB
ℹ️ View Unchanged
|
ramonjd
added
[Type] Enhancement
|
To follow up:
Looks like we can add attributes here if required: |
With this PR, if a group block is programmatically created without a layout (e.g., with
Example block HTML<!-- wp:group {"align":"full","backgroundColor":"vivid-red"} -->
<div class="wp-block-group alignfull has-vivid-red-background-color has-background"><!-- wp:paragraph -->
<p>Paragraph inside a Group block</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p>Paragraph inside a Group block</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p>Paragraph inside a Group block</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p>Paragraph inside a Group block</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph {"align":"right"} -->
<p class="has-text-align-right">Paragraph inside a Group block</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p>Paragraph inside a Group block</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p>Paragraph inside a Group block</p>
<!-- /wp:paragraph -->
<!-- wp:group {"layout":{"type":"constrained"},"backgroundColor":"luminous-vivid-orange"} -->
<div class="wp-block-group has-luminous-vivid-orange-background-color has-background"><!-- wp:paragraph -->
<p>Paragraph inside a Group block inside a Group block</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group --></div>
<!-- /wp:group -->
<!-- wp:group {"layout":{"type":"constrained"},"align":"full","backgroundColor":"vivid-purple"} -->
<div class="wp-block-group alignfull has-vivid-purple-background-color has-background"><!-- wp:paragraph -->
<p>Paragraph inside a Group block</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph {"align":"left"} -->
<p class="has-text-align-left">Paragraph inside a Group block</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p>Paragraph inside a Group block</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p>Paragraph inside a Group block</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p>Paragraph inside a Group block</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph {"align":"right"} -->
<p class="has-text-align-right">Paragraph inside a Group block</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p>Paragraph inside a Group block</p>
<!-- /wp:paragraph -->
<!-- wp:group {"layout":{"type":"constrained"},"backgroundColor":"luminous-vivid-orange"} -->
<div class="wp-block-group has-luminous-vivid-orange-background-color has-background"><!-- wp:paragraph -->
<p>Paragraph inside a Group block inside a Group block</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group --></div>
<!-- /wp:group -->This is a bit of a blocker in my opinion, at least until we work out an acceptable way around it. The placeholder is premised on the ability to select a layout with the implication that no layout is yet selected. A recent PR introduced changes the Group block so newly-added blocks are automatically set to “constrained” layout. Newly-added comprises blocks added in the editor and those programmatically, via templates. If we are to honour the default layout attributes in the group block, we need to somehow know that a block has been recently inserted in the editor, since all newly-created blocks will have the default layout. An alternative is to add a flag in the block json to indicate that the default value is being used: "layout": {
"type": "object",
"default": {
"type": "constrained",
"isDefault": true
}
}Then, in |
|
Another bug: When inserting a Group block, the block is not focussed as with all other blocks. Tried This test fails: |
ramonjd
force-pushed
the
try/group-variation-placeholder
branch
from
6b46208 to
d5ef0f8
Compare
|
I've addressed the default 'is-constrained' in #43496 (comment) and selected block issues in #43496 (comment) in c1b2044 and d5ef0f8 respectively. |
|
Nice, thanks for the ping! From #43433, which this would fix, here's a mockup for how this could look:
Here's the current state of the branch:
I'm happy to help push style changes here if helpful, but for now I'll share changes as a list, then I trust you to let me know if my assistance would be helpful:
No need to add the following illustrations to the icon library, these can exist just for the Group. Stack: Row: Group: Let me know what you think! |
To elaborate a bit, I would expect every classic placeholder to eventually move towards either the dashed style (see unselected image placeholder) or the above style (white card but with minimal setup options). Columns and Table also come to mind. This is not something that needs to happen in short order for all of those, but is nevertheless a destination to move towards, and worth considering how that impacts the implementation, i.e. the above isn't a one-off for the group. |
|
Thanks for the guidance, @jasmussen! 🙇 I was hoping that we could perhaps get the base functionality in and then work on the styles immediately after merging? It might be easier to iterate and separate the code review from the style (?) Also to keep the size of this one in check. There are a couple of things that I think wouldn't create too much noise among the refactoring etc going on this PR however.
Definitely doable. I will commit the following changes:
Edit: the skip button features in a lot of E2E tests so I'll leave that for another commit so I don't break CI 🤭
I've reused the
If we want to create a reusable component with the leaner style might require a bit of a refactor, and/or an extension of the variations objects, perhaps adding a React also complains about the I did a quick test of just swapping the icons and removing the offending svg props:
|
|
In order to land this before the freeze, yes we probably can work towards a minimal v1 and then move fast on the visual changes after that fact. We should distinguish between icons and illustrations. The group/row/stack icons should not change (and they also shouldn't be scaled up to double size). Columns is an example as it retains its default columns icon, but includes illustrations for the various variants in What do you think? |
Definitely should be the goal 👍
The difference between columns and group variations is that the group variations are shown in the inserter and transform scopes, and there's not yet a way to differentiate between scopes for the purpose of distinguishing between icons and illustrations (or anything else). So my guess is one would have to extend the variations API, or create something bespoke for variation placeholders. Not sure yet. It makes me think that it might take a bit longer than is available before freeze to get something stable through the hoop, on top of the other migration work. 🤔 If the functionality as it is appears like an improvement, it could be part of 6.1, then work could continue on both the UI and the plan to rework other placeholders later? |
|
With improvements to group/row/stacks on their own (clearer icons, multiple interfaces for transforming, clearer empty resting states), #43433 does need a bit of fidelity in order to be a better experience. I do think the right design helps it achieve that, but would also agree, if we don't think we can get there, maybe it won't be a 6.1 thing, but it would still benefit the plugin. So we have until sep 20 to figure that out. Is the main problem the need to refactor the placeholder component or the variation picker? |
Good question! I think the answer is "potentially both" but maybe only the variation picker 😄 We'd need to work on the variation picker both in terms of styling and code - positioning the variations, and adding logic to use images/icons other than those used in the inserter for example. This way we could take on the columns and group placeholders at the same time, if that's desirable. The variation picker uses the placeholder component - I'm not 100% sure we'd need to do anything quite yet, unless we want to, for example, change the outline of all placeholder containers. Ultimately, I don't think it'll be difficult task to tweak the design, yet believe it'd be better suited to separate PRs as they would be easier to review and test. I'm very happy to work on it, but the catch is I'm away next week and still have to devote some time to migrating all the PHP files it appears I've touched this year 🤣 |
|
I've been playing around to test some of my assumptions. For this PR (or a direct follow up)1, I can get it looking something like this without too much refactoring ping pong and PR clutter: 2022-09-07.12.43.37.mp4I'm still convinced that it'd be better to tackle the placeholder/variation placeholder holistically. For example, the main "Group" heading is controlled by Footnotes
|
ramonjd
force-pushed
the
try/group-variation-placeholder
branch
2 times, most recently
from
e9fce8f to
dbb8a9c
Compare
Is there any downside to merging this design now? If we're changing the UI ideally we should do it in as few steps as possible, so as not to subject users to constant change. I haven't looked at the code in detail but it's working pretty well in testing ✅ |
|
Thanks so much for looking! I'm also a big fan of small iterations, but we want to be sure that we can get very close to the mockup here, for 6.1, otherwise I would lean towards us doing this in, as you call it, "the holistic way". To put it differently: this one is important in the slightly longer than 6.1 view, but it isn't as urgent as everything else is, so we don't have to rush it. |
…rained layout type. It must be specified.
…hemeral property to identify blocks that have been inserted.
updating tests
…variation is selected.
- default layout is "constrained" when programmatically inserting a group block - using createBlock with no attributes passed
…pdate (and potentially break) the blockvariationpicker component, which is used elsewhere.
Add class to button. Fix outline heights for button.
Updating design/spacing of placeholder Hardcoding custom icons
…on to keep track of whether a variation has been selected since it adds a potentially useless attribute. In this iteration, we'll keep track of whether to show the placeholder via useState in the editor, and, more permanently, whether the block has innerblocks.
Testing on the CI whether the changes to CPT are required :)
ramonjd
force-pushed
the
try/group-variation-placeholder
branch
from
ceca35d to
4f1fd81
Compare
What?
Implement a variation-selector placeholder for the Group block for web browsers (not mobile).
Resolves: #43433
Why?
For ease of access to current and future layouts.
How?
Using
<__experimentalBlockVariationPicker />also used by the Columns block.Testing Instructions
In trunk create a few group blocks:
Here are some:
Save the post.
Checkout this branch
git checkout -t origin/try/group-variation-placeholder.Refresh the editor.
Check that the Group blocks appear as they should (no placeholder).
Now insert a new Group block.
You should see the variation placeholder picker.
Check behaviour in the site editor as well.
Expectations:
is-layout-constrainedwhen transforming blocks to Group blocks, including multiselect transforms.Screenshots or screencast