Add initial draft of container block #10562
Conversation
chrisvanpatten
added
[Type] Enhancement
| PanelColorSettings, | ||
| } from '@wordpress/editor'; | ||
|
|
||
| export const name = 'core/section'; |
There was a problem hiding this comment.
I think "container" is a better name, since "section" implies the usage of a <section> element, but often you just want to use a simple container just to add a background to something that is not semantically a section. Issues to add color options to other blocks keep being rejected because of the assumption that a generic container block could accomplish the same thing. But if the block is limited to using the <section> element, then that limits its usage, unless you want to encourage bad semantics.
|
First of all... YES! WOO-HOO! IT'S HAPPENING! 🎈 🎉 🎆 👍 ❤️ 😄
Issues to add text color options to the List block and other text-related blocks always seem to end up being closed because of the assumption that the Section/Container block would have text color options, I think. So yes, it should have text color options.
I would say yes, because if this block is meant to act as a generic container, it would be useful to float things you normally could not.
I've said it before, but ideally a section/container block should have the ability to switch between using |
IT IS NOT NECESSARILY HAPPENING THIS IS JUST AN EXPLORATION 😂😁😇😅 (I hope it happens, but let's not get too excited just yet! Lots of problems to solve first!) |
|
Honestly, even if it got in exactly like it is right now, I would be pretty happy. All I ask for WordPress 5.0 is to have a generic container block. Even if it didn't have any background options it would still be useful for applying CSS classes to a group of blocks. |
| InspectorControls, | ||
| InnerBlocks, | ||
| PanelColorSettings, | ||
| } from '@wordpress/editor'; |
There was a problem hiding this comment.
This is just a nitpick, but could the imports be put in alphabetical order?
There was a problem hiding this comment.
@ZebulanStanphill I’m waiting to get some feedback on the UX but if we determine to move forward I have a lot of tweaks I’ll make to the code itself. Thanks :)
|
In my world, |
|
I renamed the block as In my opinion, it would be good to sort out the following before committing this to 5.0:
A few smaller things:
EDIT: To clarify a bit: I think the next step is to get some reviews on whether we even like the experience of this block right now. It shouldn't be hard to solve some of these other concerns, if it's worth continuing the exploration. If it's not worth continuing, we can table this and revisit as part of the Phase 2 work! |
I agree.
Ditto. I've had to deal with this issue before and it is pretty annoying. I would definitely consider it a blocker for WP 5.0.
This is probably a good thing to work out before 5.0 since people are going to be making themes that override base Gutenberg styles, and ideally the base styles should be as unspecific as possible so that the theme styles also don't end up overriding per-container styles. If this gets fixed later, then theme-provided editor styles made before the change may still have the issue.
This sounds like a neat idea to me.
That sounds like it could be a good idea. Note also the ideas for inserter revisions in #10519.
Personally I think adding them is fine, though I'm fine with them not being added. Perhaps it would be best to wait and see how many people would want those alignments to be available. Here are some features I think this block should have. Not necessarily for WP 5.0, but definitely at some point later on:
For reference/inspiration, here are all the WordPress plugins I know of that implement a container-esque block:
And here are some links to page builder plugin demos to get some inspiration from how they implement sections and their settings: They don't have demos, but Elementor (which has a free version) and especially Oxygen are worth checking out as well. |
|
@chrisvanpatten: as the PR title reads "WIP", let us know when you'd like more reviews. |
|
@mcsf I'm interested in conceptual reviews at this point — is the UX good enough/is there enough value to continue? Perhaps a "Needs Design Feedback" tag would be appropriate at this stage? Happy to remove WIP as well if you feel it's appropriate. |
|
(That is to say I'm not looking for a super thorough review at this point, just a "yes, we like what we see, please keep iterating" at this stage. I'm totally happy to keep iterating, but with so many other things going on I just want to make sure we're on the right track before sinking too much time into this!) |
I think that's important, let's.
#7635 would solve this for us, but it's not going to make 5.0, I think. Re alignment, my personal inclination is to not add anything. I'm not sure about how to handle color.
All good ideas, we can add the label and remove the WIP prefix.
Yeah, I think this is worth pursuing. How available are you in the next days, if we had 4.1 in mind for this? The mantra is clearly to keep it simple. :) |
|
@mcsf I feel pretty confident that we can have something ready to ship with 4.1! |
chrisvanpatten
changed the title
|
Okay, I've updated this block and think we're ready for a formal review. What's in this PR:
I did add the text color attribute, and was able to work around the cascade issue with a well-placed Screenshot
What's not in this PRI didn't add a "Group in Container Block" settings menu item, although I still think this would be valuable (just juggling priorities on my end). I would love to see someone else pick that up, but I think it's also okay to punt that until post 5.0, when container blocks would presumably be more thoroughly addressed. Okay, with that said… review me! 🚀 |
|
Oh, I think I might need some help generating fixtures for the test suite. My testing skills are not quite up to snuff 🙈 |
| /* translators: Block title modifier */ | ||
| __( '%1$s (%2$s)' ), | ||
| __( 'Container' ), | ||
| __( 'beta' ) |
There was a problem hiding this comment.
Is the UI freeze a block freeze? In which case, I don't think we should want anything "beta"-denoted.
There was a problem hiding this comment.
Good catch 👍
| @@ -0,0 +1,5 @@ | |||
| .wp-block-container .editor-block-list__block { | |||
| // We need to enforce a cascade by telling nested blocks to inherit | |||
There was a problem hiding this comment.
Where is there a color otherwise being assigned that is breaking the default cascade? It's not clear to me that this should be needed.
There was a problem hiding this comment.
@aduth See my comment here: #10067 (comment)
We map body to .editor-block-list__block inside editor-styles.scss which means each block resets the cascade for these properties.
There was a problem hiding this comment.
That's a bit unfortunate 😕 But OK 👍
There was a problem hiding this comment.
Agreed we should refactor the editor styles to apply to the editor container. The downside though is that it can affect the UI bits (toolbars, movers...) so we need to be careful there.
There was a problem hiding this comment.
I almost wonder if there's a way to do a forced reset for Gutenberg chrome? Something to ponder for the future but for now this works :)
| [ textColor.class ]: textColor.class, | ||
| } ) } | ||
| style={ { | ||
| backgroundColor: backgroundColor.color, |
There was a problem hiding this comment.
Should the inline styles be necessary? Why isn't it enough to assign the color classes?
(Maybe this is a known limitation, but at a high-level it feels wrong)
There was a problem hiding this comment.
@aduth if someone picks a custom color, it has to be applied via style. I just copied this over from the paragraph block.
There was a problem hiding this comment.
So then what's the purpose of the class name being assigned?
Again, not particularly directed at you or this block, maybe more a general issue we have with colors assignment.
There was a problem hiding this comment.
@aduth If you pick one of the colors in the palette, it assigns a class. If you pick a color outside the palette, it applies a style.
This is the rendered HTML for a block using the default red/light gray colors:
<div class="wp-block-container has-background has-text-color has-vivid-red-background-color has-very-light-gray-color">
<p>Container, Paragraph One</p>
<p>Container, Paragraph Two</p>
</div>Vs this rendered HTML from the same block using two custom colors selected via the color picker popover:
<div class="wp-block-container has-background has-text-color" style="background-color:#2eaacf;color:#ffffff">
<p>Container, Paragraph One</p>
<p>Container, Paragraph Two</p>
</div>I'm not quite sure how this magic works, but it does 😅
There was a problem hiding this comment.
@aduth Oh I see you're referring to the edit representation of the block, not the front-end version, and that's a great question. I'll take a closer look.
There was a problem hiding this comment.
I'm pushing up a commit to fix this. I can open a separate PR to do this for the paragraph block as well.
| @@ -0,0 +1,5 @@ | |||
| .wp-block-container { | |||
| // 1px top/bottom padding allows us to prevent margin collapsing while | |||
There was a problem hiding this comment.
Does margin collapse happen on the front-end, or is this meant to offset the editor styles? If the latter, should we then include this only in an editor-specific stylesheet?
There was a problem hiding this comment.
Front and back-end, but in my testing it was good/necessary for both cases.
You'll need to add a |
|
Okay we are now officially passing! ✅ |
|
Whoops, posted in the wrong issue. 😛 |
|
Tagged for |
|
I had a little explore around this and a few bits of feedback.
That said, I feel this could be good to push on with accepting it need some polish. The idea I like, it's just feeling a little like needs some iterations. I would like an a11y review to be super sure though as I have my doubts as this gets complicated with blocks in blocks. I could be worrying about nothing but for me it got intense :) @tofumatt can you give a little look here please? |
tofumatt
added
[Focus] Accessibility (a11y)
|
Thanks @karmatosed! The UI right now is just the standard/default UI for nested blocks — I made almost no changes otherwise. If those concerns are blockers (and totally fair if so) then we need to sort out a few questions:
I totally understand if we need to punt this block — I obviously tried to set that expectation up front in my original comment! — just need a bit of high level direction so we can decide whether or not to move forward. |
|
@chrisvanpatten thanks for comments and I really appreciate you getting this in. Whilst it does use nested blocks with the coloring options and complexities added this exposes some issues.
I think yes :) We could tackle the color issues and problems finding the + when a background for example.
Right now this is where I am leaning towards. I understand the push here but perhaps thinking about how this works in that phase is the right approach. That said, I am willing to be convinced we just have this block in and iterate for that phase. @mtias and @jasmussen going to tag you in here as for me I am very much happy either way. As you are leading phase two @alexislloyd and @youknowriad, going to also loop you in for insight as this flows into that. |
|
I'm commenting from afar so take my thoughts in that light. I also don't have strong opinions. In an ideal scenario to me, you can colorize the background, perhaps even the text. Choose whether there's padding or not, and have the benefit of the contrast checker. On the flip side, I used to think it was enough with a container that did zilch. Simply because it would allow you to group other blocks together and then perhaps make the container reusable. But since you can do this already without a container now, perhaps the bar is raised. What is the sweet spot? What is enough for a user that inserts this block to get an idea what the point is? I don't know, and perhaps that's not the deciding factor either? Maybe the fact that we know we want it is sufficient? I know @kjellr mentioned having this block, I imagine he could field a positive argument for having this. |
|
I ❤️ this block and would fully endorse it—this is really integral to building some of the super-common layouts that sites are building these days. Combining this block with the columns block would be really 💯 (or just building support for a few columns into this block), since that's likely to be a pretty popular request. There are definitely some issues with CSS specificity, especially when child blocks use a placeholder:
I imagine this is a bit of a pain to work with if there aren't CSS selectors specific to content vs. UI. (Perhaps we could add some?) |
My ideal scenario is somewhat more advanced. I picture the ability to choose a background color or gradient, as well as an image or video to appear beneath it. I think custom padding should be allowed, as should custom margins, though presets could be available and perhaps themes could disable showing the custom option. For the sake of being able to use Gutenberg as a fully-semantic site builder, the ability to choose between using a It would also be really nice if there were vertical and horizontal alignment options like what Oxygen has, but since they involve using Flex, that would make you unable to use floats. In the past, I've considered that perhaps the Container block could have a toggle to switch between using the standard
I would prefer to avoid this. I forsee the Container block potentially having a lot of options: background color/image/video/gradient, padding, margin, perhaps even HTML tag selection. Adding columns support would be too much. Additionally, I imagine the Container block could function as an alternative, non-column-based layout block in the future. You can use CSS Flex to arrange things horizontally in a simple way that doesn't require columns, and you could have vertical and horizontal alignment options that make building layouts really nice, and don't use tons of nested I recommend checking out how Oxygen does layouts. It is very nice and very impressive. They have a Columns element, but its really secondary to the standard Flex layout options that are provided. https://oxygenbuilder.com/documentation/visual-editing/layout-spacing/ Notably, I think the Columns block could also have background and text options, but I don't see it as being that necessary since you can simply nest a Columns in a Container. I see the Container block as a more lightweight and no-columns alternative to the Columns block for layouts. Essentially, I think Gutenberg should try to avoid becoming too column-based in its layout options. I think the future of page building involves more Flex or even Grid-based layouts than |
|
Thanks for the thoughts everyone! I think at this point there are too many open questions and issues to make this viable for 5.0. I think this feels more like a phase two challenge. Unless there are any strong objections from the leads, I think this PR should be closed. During phase two, once we’ve all had time to breathe, we can start fresh and approach it from the ground up. Sound good? |
|
Personally, I think a lot of the issues with implementing the Container block as-is right now stem from UI/UX problems that really should be fixed before WP 5.0 rather than after, but if those issues aren't going to be fixed before WP 5.0, then I guess this block shouldn't come until after WP 5.0 as well. 😞 |
|
I removed from 4.1. It doesn't seem like it can be ready for it. |
|
(close and comment and comment buttons are too close :P ) |
|
Hi everyone! I'm going to close this out. Thanks for all the great feedback. A bummer this didn't make the cut, but I'm excited to revisit this in Phase 2 with a little more time for exploration. |
Please see the newest notes/information here, potential reviewers!
Original description
This is an initial draft of a section block. It definitely needs polish. It's based on @mtias's comment here and supports the following options:
A few open questions:
<div>the most appropriate element? We could also use<section>and perhaps adjust to<div>when using left/right align if we support that?I recognise this has a ways to go so seriously treat this as an early early alpha/WIP/exploration ❤️
A few additional notes: