Normalize the block toolbar for more blocks: button, buttons, list, heading, paragraph and quote #29863
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
youknowriad
added
[Feature] Blocks
|
Size Change: +275 B (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
…eading, paragraph, quote
youknowriad
force-pushed
the
update/normalize-more-block-toolbars
branch
from
9946aec
to
b23be06
Compare
|
Love this PR, thank you. All these are solid:
Regarding the heading:
The mix of alignments and heading level feel slightly eclectic here. I think this might be a case where we adopt the same principle we do for the Navigation block, and call the heading level a "Meta" group. To elaborate, in #29816 I merged some new documentation, and this bit for navigation is relevant:
The heading level does feel like a meta choice, and putting it in its own group feels right to me. But it's not a strong opinion. List looks good:
But is there an opportunity for us to revisit #21477 with this one? Or is that still best done separately? |
I agree that it feels odd as is and we can't really control the order since the block alignments come from a hook. but yeah once we normalize everything we can think of a better solution for that one.
For the list block, it's a small change but I prefer to keep it separate because of the potential design back and forth. |
|
I noticed a subtle change to the Search block when inside the navigation block, which resulted from the Cover block normalization PR:
You can see there's a double border. That border is for the alignment slot, which is disabled when the block is nested. This slot wasn't there before the PR. Did something about empty slots change? |
|
There are extra slots there and the thing there is that the slot is actually not empty, there's a component that renders nothing. It can be fixed though (you can see that I fixed it in the current PR for the button block), I just didn't reach the navigation block yet :) |
|
👌 |
|
I could use a review here if anyone has time. |
gziolo
reviewed
Mar 18, 2021
gziolo
approved these changes
Mar 18, 2021
| allowedControls = [ 'left', 'center', 'right', 'space-between' ], | ||
| isCollapsed = true, | ||
| onChange, | ||
| value, | ||
| popoverProps, | ||
| isToolbar, | ||
| isToolbarButton = true, |
There was a problem hiding this comment.
Is it necessary here – isToolbarButton? What would be a use case to set it as false? The same question applies to AlignmentUI.
There was a problem hiding this comment.
the reason for this is if this control is rendered outside the block toolbar (which is not the case now), but if we want these controls to be generic, that's something we need to consider.
There was a problem hiding this comment.
Right, should it consume the toolbar context instead to detect if it should be a toolbar button or not? @diegohaz might have an idea how to do it.
There was a problem hiding this comment.
I think it's possible to consume the toolbar context, yes. But I'd try a more untangled approach like this:
<JustifyContentUI as={ToolbarGroup} /><JustifyContentUI as={DropdownMenu} toggleAs={ToolbarButton} /><DropdownMenu toggleAs={ToolbarButton} />
7 tasks
This was referenced Jul 11, 2022
Closed
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 and #29247
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