Paragraph: Merge text settings into typography panel #36334
Conversation
aaronrobertshaw
added
[Block] Paragraph
|
Size Change: +85 B (0%) Total Size: 1.09 MB
ℹ️ View Unchanged
|
|
✅ In the Inspector Controls sidebar ensure there is no longer a Text Settings panel Looks good in Chrome/FF/Safari Before After Unrelated to this PR, I noticed that the letter spacing label bottom margin looks off in comparison with other controls.
WIP PR to fix that #36385 |
aaronrobertshaw
force-pushed
the
update/paragraph-consolidate-typography-settings
branch
from
5813ec9
to
3785b97
Compare
|
The e2e failures here are related to the drop cap control now being contained within the |
|
I've updated the e2es to access the drop cap control via the tools panel. If there are better options in terms of controls we should use to perform the same tests let me know. For now though I think this approach ticks the required boxes. |
aaronrobertshaw
force-pushed
the
update/paragraph-consolidate-typography-settings
branch
from
9f9c022
to
34fba4c
Compare
There was a problem hiding this comment.
Nice work @aaronrobertshaw, the code change and e2e updates look good to me, and this is testing well in the editor.
The prominence of the Drop cap feature is now very much reduced, so I could imagine it taking a little while for someone to find it the first time (if they're used to seeing it in the sidebar). However, given how infrequently the feature is used in practice, I think it works well having it hidden by default in the ToolsPanel menu 👍
I thought at first that I'd noticed a bug when switching between paragraphs, that when a paragraph is focused it does not render the drop-cap style. However, it turns out that appears to be by design and avoids a bunch of other bugs, so not something that needs to be fixed:
LGTM! 🎉
|
Thanks for the review @andrewserong 👍
The drop cap control does have much lower prominence. If it does prove to be something that should have higher visibility for the paragraph block I can create a quick follow-up PR to display that control by default. I'll proceed with merging this for now. @jasmussen Do you have any thoughts on whether we should make the drop cap control a default control within the Typography panel for the paragraph block? |
aaronrobertshaw
deleted the
update/paragraph-consolidate-typography-settings
branch
One of the things that is unique about the Drop Cap property is that it is very much a Paragraph block specific feature. It's not something that makes sense to add and apply to a Heading block, or inside a Table, or as part of a Code block or otherwise. There are always going to be properties that are bespoke to just that one block:
And so in exploring inspector refreshes, in many cases it's felt appropriate to add such properties right to the initial block card itself, like so:
It's not always going to work, and just like how we're curating the ToolsPanel, it will need to be a value judgement on a per-block basis. But for some of the more simple blocks, it's feeling even more obvious: Adding a dedicated panel with extra tab stops just to hold a single unique property doesn't make much sense. Coming back to the question of whether Drop cap should be shown default in the ToolsPanel. Well, as suggested above, I don't think it should be part of the reusable typography panel at all, because it should only ever be used for the Paragraph. This is not a strong opinion, and retiring the separate "Text settings" panel on its own was a good improvement. But just personally, I would consider this change to be a temporary improvement, and not one we optimize for:
This "inspector title card" concept needs separate dev work, and is an aspect of the work happening in #34574, so it needs to both crystallize in terms of design, but also in dev work. So my personal opinion is that this is probably fine to keep as is for now, so long as we don't have to create a bunch of deprecations if we extract the property back out of the tools panel in the future. What do you think? |
|
Thanks for the great summary and detailed insight @jasmussen, I appreciate it 👍
The only thing that has changed behind the scenes here is that instead of injecting a new panel including the drop cap field into the inspector controls sidebar, we wrap the drop cap control in a I imagine that any "inspector title card", would offer its own
To be clear, no new Typography block support was added for this. The existing ad-hoc drop cap control was moved to render within the Typography panel, after the block support features. No other blocks could opt-in for the drop cap even if they wanted to.
I agree. The removal of the "extra" panel cleans things up in the short term but doesn't have to be the end result. For the time being, leaving drop cap as an optional control makes sense to me. |
|
Sounds good, thank you for clarifying! 👌 I'm sure we'll also learn a ton about what works well and not so well with the ToolsPanel, this is also an opportunity to revisit these things. |
Fixes: #23767
Description
This PR leverages the recent addition of grouped
InspectorControlsto inject the paragraph block's drop cap control into the block support typography panel.Changes include:
ToolsPanelso that controls with help text maintain spacing between control and help textHow has this been tested?
Manually.
Testing instructions
Screenshots
Types of changes
Enhancement.
Checklist:
*.native.jsfiles for terms that need renaming or removal).