Extract the Global Styles Typography Panel into block-editor package #47356
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
Global Styles
youknowriad
requested review from
noisysocks,
jasmussen,
carolinan,
jorgefilipecosta and
andrewserong
|
Size Change: -22 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
noisysocks
reviewed
Jan 23, 2023
packages/block-editor/src/components/global-styles/typography-panel.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/global-styles/typography-panel.js
Outdated
Show resolved
Hide resolved
|
You beat me to it @youknowriad! 😀 Nice work.
Yeah think we need to unify these. Worth noting is that there's already some duplication as I have been thinking that maybe it makes sense to move the theme styles and settings into So:
Makes sense. I don't see this mapping in this PR though?
Agreed. There's a lot of assumptions here that are difficult to validate (e.g. my comment about heading levels) until we try to use the same |
|
What's a good way to provide some design feedback on this one? As far as I can tell, the approach makes sense, and the typography panel looks right. The main disconnect I see between global styles and blocks, is that there's an almost but not quite connection between inspector panels and GS > Blocks sections. I.e. the mapping is right between typography and color, but layout has become a kitchen sink for any panel that doesn't have a GS section counterpart. |
|
@jasmussen yes, so far this is more technical so it allows to actually use the same code. So As far as nothing breaks (same typography settings, we can edit the typography styles properly...) we're on a good path. So far I'm only updating the "global styles UI" code, but I'll also touch the block inspector a biter later. |
|
Flaky tests detected in 143dc47. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4132145710
|
|
It looks like a reasonable direction to take 👍 the test failures seem genuine though. |
youknowriad
commented
Jan 25, 2023
| letterSpacing: true, | ||
| textTransform: true, | ||
| textDecoration: true, | ||
| }; |
There was a problem hiding this comment.
It is interesting to note here that the "default controls" are sourced from the block.json for the block inspector but they are actually "static" for the global styles. In other words, it's an inconsistency between say "button" panel in global styles and "button" panel in the block inspector.
I've considered removing the "defaultControls" prop and just "always" sourcing from the block.json but I thought maybe we should not change any behavior in the current PR and just leave this unification for its own PR.
youknowriad
commented
Jan 25, 2023
| settings, | ||
| panelId, |
There was a problem hiding this comment.
"panelId" and "as" prop go hand in hand. I believe there's so magic in the "InspectorControls" panel that "forces" us to do this. There might be a simpler way to do this but this is fine enough I believe.
There was a problem hiding this comment.
There's some explanatory docs here:
gutenberg/packages/components/src/tools-panel/types.ts
Lines 24 to 28 in 7cfbdfd
I don't really understand why it's necessary. Surely each block would have a different ToolsPanelContext by virtue of the key={ clientId } in BlockSupportToolsPanel. If not then that seems like a bug in ToolsPanel.
If it truly turns out to be necessary then could make this "more correct" by having an as prop for the ToolsPanelItems.
🤷♂️ agree it's fine for now.
youknowriad
commented
Jan 25, 2023
packages/block-editor/src/components/global-styles/typography-panel.js
Outdated
Show resolved
Hide resolved
youknowriad
commented
Jan 25, 2023
|
I'm actually really satisfied about how this PR is going, I think it's in a state where we can consider moving forward. Just need to address the small details/tests... Would love more reviews here :) |
youknowriad
force-pushed
the
extract/typography-panel-into-block-editor
branch
from
19e0eee to
453e4a5
Compare
youknowriad
commented
Jan 26, 2023
|
@jorgefilipecosta The e2e test failure should be fixed in the last commit 4a1508f The problem was that we were using This is what the commit is about, it touches a number of files, so I'm going to extract it to its own PR. |
youknowriad
force-pushed
the
extract/typography-panel-into-block-editor
branch
from
841804c to
143dc47
Compare
3 tasks
1 task
3 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 to #37064 and #47348
What?
A year ago now, I've opened a discussion in #37064 that proposes unifying the components that are used in the block inspector and global styles. The idea is that we'd have components that look like this:
That component would render a panel used to edit a given style (say "typography" or "colors"), the
valuehas a well specified prop (that is mapped over the "style" object of a block or a global style "node" ). The idea is that we could use that exact same panel in both places, the only difference is a wrapper that map the "value" to the right place:This PR is a first step towards that.
How?
Right now this PR basically extracts the "TypographyPanel" from the global styles panel into a reusable component with the API as described above. I've reused that extracted component and mapped it into the global styles UI in edit-site but I haven't replaced the block inspector one (meaning at the moment, we still have two similar components within block-editor). There are some notes that would have to be answered to move forward with the block inspector integration and PR:
The component at the moment need to be rendered with aI've extracted a settings prop.GlobalStylesContextmeaning it's still a bit hard to reuse within the block inspector unless we addGlobalStylesContextwithBlockEditorProvidersomehow or that we remove that dependency by requiring another prop asettingsprop maybe.We need to implement "mapping" between the style object and the block attributes, it's basically the same format except for the "presets" where for some properties we don't store theI've implemented this for the typography styles.var|preset|valueformat but instead "extract" that value to a separate dedicated block attribute (like color or background). I've long wanted to remove that exception from blocks but It would require a number of block deprecations. So it's probably safer here to start with a mapping function.Block inspector controls often have slots within them to allow third-party blocks to inject "extra" elements within these panels. It should be solvable but I haven't looked at it yet.Managed to solve this with some extra props (as, panelId props)Testing Instructions
This PR can also be considered a first step towards this issue #47348 as it will become easier to share the exact same components between these two contexts (local vs global)