Light quote block, introduce block HoC #22803
Conversation
ellatrix
requested review from
ajitbohra,
jorgefilipecosta,
Soean,
talldan and
youknowriad
as code owners
|
Size Change: -339 B (0%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
ellatrix
changed the title
ZebulanStanphill
added
[Feature] Themes
| @@ -183,31 +183,31 @@ function BlockListBlock( { | |||
| <> | |||
| { blockEdit } | |||
| { mode === 'html' && ( | |||
| <Block.div __unstableIsHtml> | |||
| <block.Div __unstableIsHtml> | |||
There was a problem hiding this comment.
As noted in the PR description, the Div is actually the component, not block.
There was a problem hiding this comment.
And block.Div is shorthand for block( 'div' ) (which otherwise must be added on a separate line outside the component).
| export const Block = ExtendedBlockComponent; | ||
| ELEMENTS.forEach( ( element ) => { | ||
| const ElementBlock = block( element ); | ||
| block[ element ] = ElementBlock; |
There was a problem hiding this comment.
Should we deprecate the lowercase shortcuts? If so, we should probably add a comment here about it.
There was a problem hiding this comment.
Since it's experimental, we could just remove it, but I suspect it might be used by plugins by now so I'm hesitant to remove it since it's just one line.
There was a problem hiding this comment.
Personally, I think it's confusing to allow both, so we should at least deprecate the lowercase versions, even if we never remove them.
|
This change causes some weirdness on Twenty Twenty due to its editor styles that were compensating for the "heavy" block wrapper: Side note: this is also a good example of the fragility of the auto-margin-everything approach to handling block "alignment". Anything that overrides the horizontal margins on the outer wrapper of a block will cause issues like this. The negative-margins-for-wide-and-full-width-blocks approach, although feeling somewhat hacky (and running into problems with scrollbar widths and |
|
I feel the light block wrapper usage is becoming too complex and we need to take a step back and consider what APIs we want to offer. Also related to the discussion we had a few weeks ago with @aduth and others on #core-editor I think the conclusion was that our preferred API would be: <Block as={ Component } />and on situations where we need a component instead of an element (RichText, InnerBlocks) we'd do const BlockDiv = block( 'div' ); // like this PR proposesand then on the render function <RichText tagName={ BlockDiv } />So, just two APIs. I think ideally we avoid the shortcuts Another related thing is that we have some components that use |
|
@youknowriad Ok, then I think I'd prefer a single |
|
Ideally, I wanted it be a hook |
|
I'm still a bit torn about custom block components and a HoC. This will mean that the block author will need to handle merging props such as event handlers, classes and styles. If something goes wrong here, it could result in the block not working correctly. |
I agree that it's not great and ideally it's the opposite direction: "CustomComponent renders but I still see a value of the HoC to avoid having |
|
@youknowriad We cannot have I agree that we shouldn't add extra complexity though. I've removed the elements array in favour of mirroring it from If we want For now though, we seem to be limited by either a Component with a HoC or predefinitions for tagNames. |
|
If we weren't blocked by |
|
Yeah, it sounds to me like we need to request a solution from |
|
...Aaaand I just finished reading the issue and see you already found a solution: #22936. (Probably should have done that before posting my previous comment. 😛) |
|
Closing in favour of #23034. |
Description
This PR makes the quote block a light block. Since the wrapper is using another component, we also need to support this use case. I've added the
blockto support this, whileblock.Figureetc. (properties on the HoC) can still be used as a shorthand of course (instead of having to call the HoC).Note that I've also also added capitalisation of the elements because these are actually components. I've left the lowercase properties, we don't have to be strict about it.
How has this been tested?
Screenshots
Types of changes
Checklist: