Don't hardcode CSS units #32482
Merged
Don't hardcode CSS units #32482
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
aristath
requested review from
ajitbohra,
chrisvanpatten and
ellatrix
as code owners
|
Size Change: +5 B (0%) Total Size: 1.03 MB
ℹ️ View Unchanged
|
7 tasks
aristath
force-pushed
the
fix/no-hardcoded-css-units
branch
from
3b3e04a to
777eb73
Compare
aristath
force-pushed
the
fix/no-hardcoded-css-units
branch
from
777eb73 to
f169091
Compare
youknowriad
reviewed
Jun 10, 2021
| @@ -34,12 +21,16 @@ const CSS_UNITS = [ | |||
| * @return {WPElement} Letter-spacing control. | |||
| */ | |||
| export default function LetterSpacingControl( { value, onChange } ) { | |||
| const units = useCustomUnits( { | |||
| availableUnits: useSetting( 'layout.units' ) || [ 'px', 'em', 'rem' ], | |||
There was a problem hiding this comment.
in which situation the fallback is necessary?
There was a problem hiding this comment.
If for some reason value can't be retrieved from a theme.json file then the fallback will be used. It's a "better safe than sorry" fallback to make sure nothing breaks.
There was a problem hiding this comment.
It feels we should have a system that provide defaults in the block-editor package for all settings instead of repeating the same code over and over in all places.
There was a problem hiding this comment.
@nosolosw @jorgefilipecosta how defauts for the block-editor are provided now? (I mean in the frontend without the core theme.json)
There was a problem hiding this comment.
To make sure I follow: do you mean what happens in the editor when a theme doesn't provide a theme.json? In the specific case of layout.units we don't provide a fallback. We do for some things like spacing.units. We pre-populate the block editor store with defaults, but we also don't do that for __experimentalFeatures.
Adding the fallback in the useSetting hook sounds like something we should explore.
There was a problem hiding this comment.
I'm fine if the defaults are duplicated between client and server, if there's a valid reason for it. The endpoint for settings is good to get the settings that are not "defaults" but for the defaults, I don't see the issue for mobile.
There was a problem hiding this comment.
How do you feel passing defaults as initialization to the block editor?
The fact that we have them in multiple places (client defaults for the store, useSetting, server, etc) has been problematic in the past and this issue is also a good demonstration of that.
There was a problem hiding this comment.
How do you feel passing defaults as initialization to the block editor?
I think the editor should have defaults bundled, and not require passing settings to work properly.
There was a problem hiding this comment.
the reason is simple, the block-editor package is independent and it's really not great to force its users to pass a complex settings object to get sane defaults.
There was a problem hiding this comment.
I've prepared #32702 that adds a default value for __experimentalFeatures in the @wordpress/block-editor package. It ports the existing defaults set by the core theme.json.
youknowriad
reviewed
Jun 10, 2021
youknowriad
reviewed
Jun 10, 2021
youknowriad
approved these changes
Jun 10, 2021
7 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.
Fixes #32478
In #31822 we replaced all hardcoded CSS units with calls to
useCustomUnits, providing defaults and an array of units where appropriate.Since that PR got merged, changes were pushed to border controls and letter-spacing, adding some more instances of hardcode CSS_UNITS. This PR replaces the new instances with calls to
useCustomUnits, so we'll have a single source of truth for CSS units.How has this been tested?
Tested the letter-spacing and border-radius features, everything seems to be working fine.
Also added an array of units in the theme's
theme.jsonfile undersettings.layout.units(formatted as['px', 'em']) and confirmed that the units defined are then used in these controls.Checklist:
*.native.jsfiles for terms that need renaming or removal).