Typography: Switch to ToolsPanel for block support UI #33744
Conversation
aaronrobertshaw
added
[Type] Enhancement
|
Size Change: +583 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
aaronrobertshaw
force-pushed
the
update/typography-support-to-use-new--panel
branch
from
86f2bef
to
f2ab691
Compare
aaronrobertshaw
force-pushed
the
update/typography-support-to-use-new--panel
branch
from
f2ab691
to
d4149ae
Compare
aaronrobertshaw
changed the title
aaronrobertshaw
requested review from
ajitbohra,
chrisvanpatten and
ellatrix
as code owners
|
Nice one. In general for all these toolspanel refactors, we want to tread carefully with changing too much from what's shipping at the moment. Ideally give or take a few exceptions, we want the default design tools to be the same as they were before the panel switch. The bigger benefit of the tools panel is for any future features to benefit global styles and be available in a non-default way in the post editor. So in this case, we want to at the very least show font size and line height by default. What do you think? |
|
@jasmussen There's a bit of a mix of what blocks show for Typography by default. I think I agree that we should stick to font size and line height as the default for a lot of these unless there's a good reason not to? Here's the blocks using Typography and what they currently show as the default. Let me know how we should move forward:
|
|
I think we could merge this work first, and then follow up on a decision on defaults in a separate PR. |
|
If we were to merge this one with a completely empty typography panel for all those blocks, we'd want to move pretty fast to at least enable font size and lineheight for the Paragraph block. It honestly seems better to at least enable those two by default for all these as a starting point. |
Agreed, if we stick to font size and line height as the two defaults for everything, then we could add it to this PR /cc @aaronrobertshaw |
The block.json changes are easily done though I have two thoughts I'd like feedback on.
|
There's a PR over here: #34064 It's based off trunk for now. I can rebase it on top of this for testing once we're ready.
For now I've defaulted to which control was available, that is, setting fontSize and lineHeight defaults where each block supports them. |
Is it not yet possible to tailor the support on a per-block basis? |
It is. That's why some blocks have opted into the support while others haven't. We can also choose which controls are defaults on a per-block basis.
I only wanted to confirm that we do indeed want to turn on line height for the blocks that hadn't already opted into it. Or, whether we wanted to just display font size and line height as default controls if the blocks currently support them. For example, the paragraph block has font size and line-height support, so it would show both by default. The code block only has font size support at the moment. Does it only show the font size control by default, or do we also update it to opt into line-height support and display that control as well? |
Right. Having both font size and line-height feel valuable, the only niggle is we want to make sure the line-height works well before enabling it. So for those that haven't yet had access to lineheight we might want to start without it and go from there. |
|
I've rebased this PR again. In addition, I've removed the change of the Global Styles typography UI to the The following screenshot is how the Global Styles panel appeared before the removal of those changes as described above:
|
aaronrobertshaw
force-pushed
the
update/typography-support-to-use-new--panel
branch
from
2153158
to
eaac5c4
Compare
|
This has now been updated to handle the font-family moving from a property on the style attribute to a top level attribute itself. The font-size reset e2es have also been updated to be |
There was a problem hiding this comment.
I've gone through a number of different blocks and tried all typography settings on, off, and with them reset. Everything worked as expected. I can't see a reason for this not to be approved, so I'll kick the process off. :)
There are a couple of UI issues that are related to components that I don't think need to be fixed prior to this being merged. I can follow up with those afterwards.
|
I've been testing this PR as a base for Typography block support: add typography support and defaults #34064. The ToolsPanel is working as expected for all the blocks I'm testing. Here's the Site Title, which opts-in to all Typography support:
Looks great! I think it's also good to go! It'll also undergo more testing as a product of testing #34064. |
|
Thanks for the testing @apeatling and @ramonjd. The more eyes on this one the better I think. Having said that, this PR is probably in a good place to push forward with a view to merging soon. Because this PR touches a lot of files, I'll write up a bit of a summary to help break down the changes and make it easier for other reviewers or reference in the future. |
SummaryNew InspectorControls slotAs part of the switch to the ToolsPanel, this PR adds a new Slot to the grouped Inspector Controls. This allows for the injection of ad hoc controls by blocks or plugins into a block support’s panel. This way we can avoid overlapping panel responsibilities. An example of this could be a block adding a drop cap control into the Typography panel even though that is not currently offered through block supports. Updating block support hooksEach typography block support feature requires its hook to be updated with util functions to facilitate resetting or checking the presence of values. The block support edit components have also had their checks that skip rendering streamlined. These will now only be made once, at the
This will check whether values have been set in a block’s attributes. Depending on the feature, it might be top level attributes or a property on the style attribute that must be checked.
This handles clearing values from a block’s attributes and will also vary per feature. Updated HooksSwitching to the ToolsPanelThe real shift to using the Here, the original use of a StylingNot all components used in block support controls have been converted to the new hook and context system. As such, we need to rely on some additional CSS to polish up the display of these controls within the Typography support opt-in & default controlsIt was requested that the paragraph block opts into the typography support for at least font size and line height and these controls be displayed by default within the E2E test changesAs a result of the switch to the Clean upThere is currently no use of the combined text decoration and transform controls. These are used individually. Moving forward in the |
There was a problem hiding this comment.
Great work @aaronrobertshaw, it's super satisfying seeing the ToolsPanel rolled out to the Typography support, and I like the consistent patterns used for all the different hooks 👍
Each of the typography supports is working well in my testing (along with reset and rest all behaviour). I noticed with the Navigation block that setting text decoration updates the setting correctly, and the styling on the front end renders correctly, but the style doesn't appear to win the specificity/inheritance within the editor if we're using the editor version of the Navigation block (e.g. clicking Edit on the block so we're not using the server rendered view in the editor). This is the same behaviour as on trunk, though, so unrelated to this PR and not a blocker.
The code change is looking good, I noticed one issue with the Font Appearance control. We might want to update the label in the menu dropdown so that it shares the same logic as the label used by the hook (I've left a comment about this).
Since this is already a fairly big PR, I'd be in support of merging this in as-is, and us looking at tweaking the label for Font Appearance in a quick follow-up PR, since we're already going to be doing a bunch of other follow-ups anyway.
LGTM! 🚀
| <ToolsPanelItem | ||
| className="single-column" | ||
| hasValue={ () => hasFontAppearanceValue( props ) } | ||
| label={ __( 'Appearance' ) } |
There was a problem hiding this comment.
In font-appearance-control the label is conditional as to whether font styles, font weights, or both are available. Should we add a conditional here, too? (or abstract that logic into its own function so it can be used in both places?
I noticed while testing the Heading block in TT1-Blocks that it felt a little confusing the label in the menu being different to the one in the panel item itself.
| Panel label | Menu label |
|---|---|
 |
 |
|
Thanks for the thorough review @andrewserong, always appreciate it 👍
I've been able to replicate this but only after selecting the individual item/link within the Navigation block and choosing Edit from the block's toolbar. After a little exploration, it appears that we end up with a div inside the item's As mentioned, it occurs on
Nice catch! I like the idea of addressing this in a follow-up as well. Given the use of the label as a key for the panel's internal menu item state and for item registration, I'll need to check for any edge cases around making this change when used in conjunction with the typography Inspector Controls SlotFill. Thank you everyone for the testing and reviews. As we have multiple approvals and plans for a few small follow ups I'll merge this now to unblock #34064 |
aaronrobertshaw
deleted the
update/typography-support-to-use-new--panel
branch
|
A potential fix for the Nav item not inheriting the text-decoration style correctly is up in #35859 |
Depends on:
Dimensions Panel: Add new ToolsPanel component and update spacing supports #32392Block Supports: Switch dimensions inspector controls slot to bubble virtually #34725Related:
Description
This PR:
typographyInspectorControls group slotDefault Controls
This PR only addresses the switch to the new
ToolsPanelcomponent. Deciding which controls should display by default for which blocks will be addressed via a separate PR.How has this been tested?
Manually.
Test instructions
Screenshots
Screen.Recording.2021-10-20.at.7.47.48.pm.mp4
Types of changes
New feature.
Checklist:
*.native.jsfiles for terms that need renaming or removal).