Global styles: convert preset font size values to CSS vars #44967
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
ramonjd
added
[Status] In Progress
ramonjd
force-pushed
the
try/global-styles/use-css-var-font-size-preset
branch
from
6ca7f1d to
1b8469c
Compare
|
Size Change: +310 B (0%) Total Size: 1.29 MB
ℹ️ View Unchanged
|
ramonjd
commented
Oct 21, 2022
ramonjd
added
[Type] Enhancement
There was a problem hiding this comment.
This is testing pretty well for me @ramonjd! Just left a couple of comments, one nit, and one question about whether we should also allow this behaviour on themes that haven't opted-in to fluid typography.
Also, since this updates the font size picker in the components package, shall we add a changelog entry, too?
I'll just CC @ciampo and @mirka in case they have input about the update to the signature of the onChange callback, too 🙂
Thanks for the follow-up, it'll be great to store the var: form of fluid values in particular, rather than the clamp() value, and it'll be really good to have this in there for when we eventually allow custom control over the fluid typography rules, too 👍
packages/edit-site/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
Thank you for the ping! Regarding the
Yes! On top of that, I think it would be great if we could coordinate with @noisysocks and his PR adding more better unit tests so that we can add good test coverage for this change too. Another preference is to try and keep changes to |
ciampo
added
[Package] Components
ramonjd
force-pushed
the
try/global-styles/use-css-var-font-size-preset
branch
from
af9a739 to
5839acd
Compare
|
Thanks for the reminder about the changelog, folks! I had intended to get unit tests done, but might wait until #45298 is merged until I do. 🙇 |
|
#45298 is merged! The test coverage for |
ramonjd
force-pushed
the
try/global-styles/use-css-var-font-size-preset
branch
from
5839acd to
0f991ee
Compare
ramonjd
commented
Nov 2, 2022
ciampo
reviewed
Nov 2, 2022
There was a problem hiding this comment.
FontSizePicker's code changes are looking good! I'll let @noisysocks give the final blessing ✨
ramonjd
force-pushed
the
try/global-styles/use-css-var-font-size-preset
branch
from
e2da9f7 to
3a574fd
Compare
|
@noisysocks Could you give this one a sanity check? Also pass your eye on the fix for the issue highlighted in #44966 (comment) Thank you, sir! |
…d of the clamp value in global styles.
…nd values. Ensure that we allow presets for all font size preset, not only while fluid typography is activated
…tSize object argument.
…d to pass the immediate, currently-selected font size object containing preset slug information to the consuming parent component. Updated tests. General post-rebase chicanery
…o clampify incoming values for the site editor font size picker
ramonjd
force-pushed
the
try/global-styles/use-css-var-font-size-preset
branch
from
bf85ece to
09692b7
Compare
noisysocks
reviewed
Nov 8, 2022
packages/edit-site/src/components/global-styles/typography-panel.js
Outdated
Show resolved
Hide resolved
noisysocks
approved these changes
Nov 8, 2022
andrewserong
approved these changes
Nov 8, 2022
There was a problem hiding this comment.
Nice one, this is testing well for me @ramonjd!
✅ Tested that setting existing preset values for Typography in global styles prior to applying this PR results in those preset values still showing up as selected after applying this PR
✅ Tested that with this PR applied, setting Typography based on presets works as expected, the CSS var is applied rather than the raw clampified value, and after a reload the selected values display as they should.
LGTM, and I like the suggestion from @noisysocks to potentially rename the hook, too 👍
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.
Parent issue: #44888
Context: #44752 (comment)
What?
This PR converts font size presets to
var:preset|font-size|${slug}values so that they can be parsed bygetValueFromVariableand converted tovar(--wp--preset--font-size--${slug}) values.Why?
Why not?
At the moment we're saving the raw value. When fluid is turned on this means we're saving the
clamp()value.CSS vars are there. Why not use them?
Testing
In the site editor, apply a font size preset to an element/block in global styles (not block styles)
Check that we're using the preset CSS variable, and not the raw font size value.