Replace hardcoded CSS_UNITS and inherit values from theme.json #31822
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: -71 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
May 17, 2021
| * | ||
| * @return {Array} Filtered units based on settings. | ||
| */ | ||
| export const useCustomUnits = ( { units, availableUnits, defaultValues } ) => { |
There was a problem hiding this comment.
Thanks for the PR @aristath. Thinking a bit I'm not at ease with exposing this hook in the components package. Can you clarify how it's supposed to be used? Why do we need both this hook and the component with the units prop.
I guess on my mind doing something like this should be always sufficient, why it's not the case?
<UnitControl units={ useSetting( 'spacing.units' ) } />
There was a problem hiding this comment.
Yeah this was a very deep rabbit hole and I spent hours trying and iterating things to find something that works... 😅
These controls are used in lots of places, in different contexts. The default values vary depending on the context (for example the cover block's height should by default include vh units while the layout or column-width units should not).
useSetting will only get the value from theme.json which is formatted like ["px", "em", "rem"]. The control however requires a different format so useCustomUnits needs to run in order to convert the units to [{value:"em", ...}, {...}, ...] (that's what we were doing before too).
I suppose we could make UnitControl understand an array of units like ["px","em","rem"] and just make the conversion to the full array inside the control itself... But then if spacing.units is empty it would not fallback to the default units as it does now.
Using useSetting to get these would not work correctly unless we add hardcoded fallback in useSetting to get defaults - both in the block-editor and the site-editor packages since useSetting is implemented separately in each of them (which is also what triggered the experimentation on #31782).
The reason I placed it in components inside the control's utils is because it seemed that the most suitable place for it... Do you think some other package would be a better home?
There was a problem hiding this comment.
I suppose we could make UnitControl understand an array of units like ["px","em","rem"] and just make the conversion to the full array inside the control itself... But then if spacing.units is empty it would not fallback to the default units as it does now.
I feel this might be the best way potentially but right now maybe we can just start by marking the hook as experimental?
There was a problem hiding this comment.
I went ahead and marked the hook as experimental for now.
|
It would be cool to get this ready for tomorrow's RC (5.8 feature freeze) |
youknowriad
force-pushed
the
add/useCustomUnits
branch
from
9e393ca
to
21c5f14
Compare
youknowriad
reviewed
May 19, 2021
| import { __experimentalBoxControl as BoxControl } from '@wordpress/components'; | ||
| import { | ||
| __experimentalUseCustomUnits as useCustomUnits, | ||
| __experimentalBoxControl as BoxControl, |
There was a problem hiding this comment.
Makes me wonder why these hooks (margin, padding) are not using the UnitControl component from the block editor components directly.
youknowriad
reviewed
May 19, 2021
| @@ -17,11 +17,12 @@ import { | |||
| TextControl, | |||
There was a problem hiding this comment.
Same here, there's already a hooked UnitControl in block-editor package, why isn't this one used here instead.
|
I'm going to merge this to include in 5.8 but it definitely sounds like all these components need to be consolidated. I think all of the "hooks" and "block-library" usage should rely on the Can we open an issue or follow-up about that? |
youknowriad
approved these changes
May 19, 2021
youknowriad
force-pushed
the
add/useCustomUnits
branch
from
042c0d0
to
3bc0ffc
Compare
youknowriad
force-pushed
the
add/useCustomUnits
branch
from
3bc0ffc
to
e1cad86
Compare
jffng
pushed a commit
that referenced
this pull request
May 19, 2021
Co-authored-by: Riad Benguella <benguella@gmail.com>
7 tasks
This was referenced Jun 3, 2021
7 tasks
This was referenced Sep 30, 2021
14 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.
Description
Currently, most controls have units hardcoded in
CSS_UNITSconstants, and many components don't use the units defined intheme.jsonfiles.We already had a
useCustomUnitsfunction insidepackages/block-editor/src/components/unit-control/, so this PR does the following:useCustomUnitstopackages/components/src/unit-control/utils.jsuseCustomUnitsin@wordpress/componentsCSS_UNITSarrays, and replace them with calls touseCustomUnits. The calls check fortheme.jsonsettings (usinguseSetting), and if no settings are located then fallback to an array of units (this array was extrapolated from the previousCSS_UNITSconstants).ALL_CSS_UNITS) containing all existing CSS units. This way users can define their arrays like["px","em"]and all labels will be properly populated - even if they decide to use unusual units likecm,inorpc🤷While doing the above, I realized that
spacing.unitsdoesn't make sense for everything... Spacing refers to padding & margins, not widths. So In some instances (like for example Layout settings & column widths), instead of checking forspacing.unitsI'm checking forlayout.units.Overall, this PR cleans up the code a bit and feels like an improvement, increasing consistency.
How has this been tested?
Checked in an FSE theme with and without values defined in
theme.jsonforsettings.spacing.units&settings.layout.units.Confirmed that it works as expected for layout, padding, margin, column-width, cover block, search block. Checked both the post-editor & site-editor.
Checklist:
*.native.jsfiles for terms that need renaming or removal).