Normalize the block toolbar of the audio, file, media and text, video, site logo and post featured image blocks. #30012
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…, site logo and post featured image blocks
youknowriad
added
[Type] Enhancement
youknowriad
requested review from
ajitbohra,
ajlende and
ellatrix
as code owners
|
Size Change: +97 B (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
gziolo
reviewed
Mar 19, 2021
| @@ -187,20 +187,6 @@ function MediaTextEdit( { attributes, isSelected, setAttributes } ) { | |||
| gridTemplateColumns, | |||
| msGridColumns: gridTemplateColumns, | |||
| }; | |||
| const toolbarControls = [ | |||
There was a problem hiding this comment.
Should we discourage the usage of controls prop at the end of the refactoring with all non-default groups in BlockControls ?
There was a problem hiding this comment.
I think it's not supported in non default group blocks (I did this on purpose as I think component composition is better)
gziolo
approved these changes
Mar 19, 2021
|
Nice work, I'm so excited for you to get to the navigation block soon :D These are all excellent:
though it would be nice to revisit dropdowns for list style and indentation ;)
I'm seeing a small issue with the site logo:
Is that because I'm inserting it in the post editor when I should be inserting it in the site editor? For media & text. it helps with a "before":
There was so much going on there it was hard to see what was what. So that aspect is definitely improved:
With this drastic reduction, some things I need to work on separately become clear:
I think we should put Justification next to Alignment, as those two feel related, then left/right can come after. That might also improve the appearance of the link icon, which right now feels like it just sits there on its own. |
|
Nice teamwork @jasmussen with reviewing, it's very effective 💯 I love all the small refinements that improve the readability of the block toolbar. |
|
@jasmussen For the site logo block, it seems that you're in a context where "left/right/center" alignments don't seem to be allowed. I'm not sure how I can do that (in post editor, I see them personally), That said, this highlighted something for me. we have a |
|
Yes, that's an excellent thought. I think it touches on the wide/fullwide alignment discussion we've had, where I see it as a property of post content or other containers optionally. In most cases, like the site logo, I would imagine its position be largely dictated by its parent, rather than on its own. Give or take. |
I'm not sure I agree with this personally, I think all blocks should have all the alignments options that their parent allow them to have. This means that in a regular context, all blocks should be allowed to be aligned left/right/wide/full. In some context, parent blocks could remove some things from their children, examples:
Also, this is not an adhoc thing, if the container is another kind of layout (think flex, grid, absolute), all these alignments options should be removed from all children and replaced by new ones. |
|
In other words, I don't think it's the responsibility of a single block to decide what alignments options it can have. |
|
I was unclear, and I was thinking primarily of how the Group block centers content inside by default. I would not want you to have to make a Header template part be "full-wide" or else it was in a centered column. I definitely agree you should be able to left align a site logo inside such a block (though I can see a situation where we have flex properties on a group, and space-between plus margins might be the better aligner). I need to take a weekend, I think, thanks again for this PR! |
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related #25983 similar to #29205 #29247 and #29863
Similar to the previous PRs, this one uses the new BlockControls "groups" (block, inline, other) for the following blocks:
In order to do so, I had to remove the "ToolbarGroup" that was explicitly added to some reusable components (toolbars).
Testing instructions