Block library: Add text align support to List block #16792

gziolo · 2019-07-29T13:51:26Z

Description

Partially implements #15096.

This PR adds text alignment support to the List block:

There is a different order on the screen above but it should be tackled seperately as it's baked in the RichText implementation.

How has this been tested?

Add List block, set one of the existing text alignments and ensure that text alignment changes. In addition, inspect HTML output in the Code editor and ensure it looks like the following:

<!-- wp:list {"textAlign":"right"} -->
<ul class="has-text-align-right"><li>Welcome to WordPress.</li><li>This is your first post.</li><li>Edit or delete it, then start writing!</li></ul>
<!-- /wp:list -->

Screenshots

Types of changes

New feature/Enhancement.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.

gziolo · 2019-07-29T13:57:25Z

There is a different order on the screen above but it should be tackled seperately as it's baked in the RichText implementation.

@ellatrix, is there any plan to move List controls out of RichText component?

gziolo · 2019-07-29T14:16:04Z

packages/block-editor/src/components/rich-text/index.js

@@ -463,7 +463,7 @@ RichTextContainer.Content = ( { value, tagName: Tag, multiline, ...props } ) =>
 	const content = <RawHTML>{ html }</RawHTML>;

 	if ( Tag ) {
-		return <Tag { ...omit( props, [ 'format' ] ) }>{ content }</Tag>;
+		return <Tag className={ className || undefined } { ...omit( props, [ 'format' ] ) }>{ content }</Tag>;


@ellatrix - this is necessary to avoid the case where <ul class="">...</ul> is saved in the post content. Does it make sense?

Why does the parent not pass undefined? What value is it passing?

it passes an empty string:

classnames() === '';

Ok, but then it should be handled by the parent, as the parent uses classnames, not RichText? E.g. classnames() || null.

Yes, this is one approach but then all blocks need to do it which is suboptimal. I prefer we have it covered.

I can add this logic to the List block if you strongly disagree.

ellatrix · 2019-07-30T07:57:19Z

@gziolo I'll have a look today. Ideally they should move to the list block, yes.

ellatrix · 2019-07-30T10:33:19Z

@gziolo #16536 moves the list controls as part of a refactoring.

ellatrix · 2019-07-31T08:26:41Z

Btw, does text alignment make sense at all on a list block? Shouldn't the bullets and everything move as well then?

gziolo · 2019-07-31T08:49:50Z

Btw, does text alignment make sense at all on a list block? Shouldn't the bullets and everything move as well then?

That's a very good question, I don't like how it looks as well. That's why I asked for design feedback. @mapk, @kjellr or @karmatosed - what do you think?

kjellr · 2019-07-31T12:08:47Z

Yeah, I think the bullets would need to move as well — for example, when using a RTL language the bullets move over to the right side:

For center aligned lists, the bullets should just appear inline:

... But really, I don't think alternate alignments are all that common for the list block. I like including it in here for consistency's sake, but I don't think it'll be regularly used. If it's not a big deal implement these bullets, go for it. Otherwise, I think it's probably fine to leave alignment out for this block.

draganescu · 2019-07-31T12:19:25Z

Aside from the change on which you have discussed with @ellatrix, the code seems ok, but the result of using text align is questionable especially if using indent as well:

gziolo · 2019-07-31T12:41:12Z

I have something like this working locally, I will push it soon:

gziolo · 2019-07-31T12:58:27Z

422c987 contains all styling changes presented on screenshots presented in the previous comment. I think it looks better than I anticipated. I had to play a bit with the margin applied by default but I managed to skip it when text alignment is applied.

ellatrix · 2019-07-31T14:30:43Z

Do you think nesting + aligning will work well? In the example above the text length of the nested items is shorter, so it looks like it's nested, but that will not always be the case. You may have some duplicate numbers without any indentation.

gziolo · 2019-08-01T07:03:28Z

Do you think nesting + aligning will work well? In the example above the text length of the nested items is shorter, so it looks like it's nested, but that will not always be the case. You may have some duplicate numbers without any indentation.

I don't think this option will be super popular but it seems like we should offer it for consistency. There is a live preview of the effect so people can decide whether it makes sense in their particular case. I'm sure we can't anticipate all the reasons why people might want to use this capability. Theme authors might also have some good ideas about how this could be styled to make their design more compelling.

…tent

shaunandrews · 2019-08-01T12:45:43Z

This seems like a bad idea. Just because other text-based blocks have alignment, doesn’t mean lists need it. Are there any real world use-cases for centered or right aligned lists? (And RTL doesn’t count, as the block should do that automatically.)

mapk · 2019-08-02T14:54:03Z

I'd like to echo the concerns of the others here and say that while this consistency is nice, I can't really imagine a real-world use case that works aesthetically. I suggest NOT including alignment controls on the List block.

kjellr · 2019-08-02T15:24:19Z

That seems fine to me, too. It looks weird, and I don't think we've heard from anyone really asking for this feature. 👍

gziolo · 2019-08-05T06:28:55Z

Thanks for all your feedback. Let's close this PR and update the original issues with what we agreed upon here 👍

gziolo requested review from ajitbohra, jorgefilipecosta, Soean, talldan and youknowriad as code owners July 29, 2019 13:51

gziolo added Needs Design Feedback Needs general design feedback. [Block] List Affects the List Block [Type] Enhancement A suggestion for improvement. labels Jul 29, 2019

gziolo self-assigned this Jul 29, 2019

gziolo added the [Package] Block library /packages/block-library label Jul 29, 2019

gziolo mentioned this pull request Jul 29, 2019

Bring consistency to block toolbar for text blocks #15964

Closed

5 tasks

gziolo requested review from daniloercoli, ellatrix, etoledom and SergioEstevao as code owners July 29, 2019 14:13

gziolo commented Jul 29, 2019

View reviewed changes

gziolo force-pushed the update/list-text-align branch from 3c38745 to f0b8632 Compare July 29, 2019 14:19

gziolo removed the Needs Design Feedback Needs general design feedback. label Jul 31, 2019

gziolo force-pushed the update/list-text-align branch from f0b8632 to 422c987 Compare July 31, 2019 12:56

gziolo added 4 commits August 1, 2019 13:35

Block library: Add text align support to List block

0ca3015

Ensure that class name with an empty string is never saved in the con…

d0b01d4

…tent

Improve styling for List block with text alignment applied

c83674f

Regenerate test fixutuerse for blocks

86dd1ef

gziolo force-pushed the update/list-text-align branch from 422c987 to 86dd1ef Compare August 1, 2019 11:44

gziolo requested review from nerrad and ntwb as code owners August 1, 2019 11:44

gziolo closed this Aug 5, 2019

gziolo deleted the update/list-text-align branch August 5, 2019 06:29

jasmussen mentioned this pull request Aug 5, 2019

Bring consistency to block toolbar for text blocks #15096

Closed

MaxMood96 mentioned this pull request Mar 14, 2023

[Snyk] Security upgrade webpack from 5.65.0 to 5.76.0 MaxMood96/gutenberg#91

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Block library: Add text align support to List block #16792

Block library: Add text align support to List block #16792

gziolo commented Jul 29, 2019 •

edited

Loading

gziolo commented Jul 29, 2019

gziolo Jul 29, 2019

ellatrix Jul 30, 2019

gziolo Jul 30, 2019

ellatrix Jul 30, 2019

gziolo Jul 30, 2019

ellatrix commented Jul 30, 2019

ellatrix commented Jul 30, 2019

ellatrix commented Jul 31, 2019

gziolo commented Jul 31, 2019

kjellr commented Jul 31, 2019

draganescu commented Jul 31, 2019 •

edited

Loading

gziolo commented Jul 31, 2019 •

edited

Loading

gziolo commented Jul 31, 2019

ellatrix commented Jul 31, 2019

gziolo commented Aug 1, 2019

shaunandrews commented Aug 1, 2019

mapk commented Aug 2, 2019

kjellr commented Aug 2, 2019

gziolo commented Aug 5, 2019

Block library: Add text align support to List block #16792

Block library: Add text align support to List block #16792

Conversation

gziolo commented Jul 29, 2019 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

gziolo commented Jul 29, 2019

gziolo Jul 29, 2019

Choose a reason for hiding this comment

ellatrix Jul 30, 2019

Choose a reason for hiding this comment

gziolo Jul 30, 2019

Choose a reason for hiding this comment

ellatrix Jul 30, 2019

Choose a reason for hiding this comment

gziolo Jul 30, 2019

Choose a reason for hiding this comment

ellatrix commented Jul 30, 2019

ellatrix commented Jul 30, 2019

ellatrix commented Jul 31, 2019

gziolo commented Jul 31, 2019

kjellr commented Jul 31, 2019

draganescu commented Jul 31, 2019 • edited Loading

gziolo commented Jul 31, 2019 • edited Loading

gziolo commented Jul 31, 2019

ellatrix commented Jul 31, 2019

gziolo commented Aug 1, 2019

shaunandrews commented Aug 1, 2019

mapk commented Aug 2, 2019

kjellr commented Aug 2, 2019

gziolo commented Aug 5, 2019

gziolo commented Jul 29, 2019 •

edited

Loading

draganescu commented Jul 31, 2019 •

edited

Loading

gziolo commented Jul 31, 2019 •

edited

Loading