Remove the name and element props from the TypographyPanel component #47908
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
[Type] Code Quality
youknowriad
commented
Feb 9, 2023
| * | ||
| * @return {Object} Merge of settings and supports. | ||
| */ | ||
| export function overrideSettingsWithSupports( settings, supports ) { |
There was a problem hiding this comment.
The idea is that ultimately this function is going to allow us to merge "block.json" supports with "theme.json" settings "like" object.
youknowriad
commented
Feb 9, 2023
| } | ||
|
|
||
| function useHasTextDecorationControl( name, element ) { | ||
| const supports = useSupportedStyles( name, element ); | ||
| return supports.includes( 'textDecoration' ); |
There was a problem hiding this comment.
Notice that the previous implementation of text transform didn't take into consideration the "setting" value that comes from theme.json. I think this is an inconsistency and probably something that was "forgotten". So in that sense, the current PR changes the behavior a bit but I believe it's better anyway.
youknowriad
commented
Feb 9, 2023
| @@ -52,52 +39,38 @@ export function useHasTypographyPanel( name, element, settings ) { | |||
| ); | |||
| } | |||
|
|
|||
| function useHasFontSizeControl( name, element, settings ) { | |||
| const supports = useSupportedStyles( name, element ); | |||
There was a problem hiding this comment.
Now this function is only called once for the "TypographyPanel" rendering, there might be a small perf improvement here (very hard to measure). I believe this addresses one of the points raised by @jorgefilipecosta in the original PR.
youknowriad
commented
Feb 9, 2023
| @@ -48,33 +52,59 @@ function TypographyInspectorControl( { children } ) { | |||
| ); | |||
| } | |||
|
|
|||
| function useBLockSettings( name ) { | |||
There was a problem hiding this comment.
While right now, this function is "adhoc" in the "typography" hook. I believe this can be one big unique function that "builds" this object per block and make it available for all hooks... in the BlockEditContext or something like that.
Base automatically changed from
extract/typography-panel-into-block-editor
to
trunk
February 9, 2023 08:44
youknowriad
force-pushed
the
refactor/remove-name-element-props-from-typography-panel
branch
from
baa5460 to
6b61f3d
Compare
|
Flaky tests detected in 21802dc. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4182078371
|
|
Size Change: +131 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
youknowriad
force-pushed
the
refactor/remove-name-element-props-from-typography-panel
branch
from
6b61f3d to
df46199
Compare
youknowriad
force-pushed
the
refactor/remove-name-element-props-from-typography-panel
branch
from
df46199 to
732e14e
Compare
youknowriad
force-pushed
the
refactor/remove-name-element-props-from-typography-panel
branch
from
5688236 to
15317d7
Compare
jorgefilipecosta
approved these changes
Feb 14, 2023
| }; | ||
| } | ||
|
|
||
| if ( ! supports.includes( 'lineHeight' ) ) { |
There was a problem hiding this comment.
Minor: All the rules from now on could be replaced with a single forEach loop:
[ 'lineHeight', 'fontStyle', ....].forEach(...)
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.
Builds on top of #47356
What?
This continues the work to bring clarity to our code base and define a clear mental model for Global Styles UI and Block Inspector.
In this particular PR, I'm removing the "block name" and "element" props from the TypographyPanel component (Style UI component) in favor of the existing "settings" prop. In other words, The mental model becomes as such:
For an inspector control:
For the global styles panel:
Why?
All the reasoning and specifications for this work is in the following discussion #37064
Testing Instructions
Make sure to test that the style style panels are shown in block inspectors and global styles UI between this branch and trunk.