HeightControl: Stabilise the height control component in the block editor package #47475
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
andrewserong
added
[Type] Code Quality
andrewserong
requested review from
ellatrix,
ajlende,
ajitbohra,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
14 tasks
|
Update: just a note that I've done a search in https://wpdirectory.net for usage of |
|
Size Change: -338 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
|
I'll let folks more experienced with the |
aaronrobertshaw
approved these changes
Jan 27, 2023
There was a problem hiding this comment.
Thanks for putting this PR together @andrewserong 👍
This is testing pretty well for me though I spotted a couple of typos in the readme files in this PR. It might also be good to tidy them up while we're here.
✅ Can adjust min-height values within Global Styles
✅ Min height adjustments work for individual blocks in the post editor
❓ Some minor typos in readme files (not introduced by this PR, though)
I agree with your logic regarding the decision to stabilize vs lock the component. Ultimately though, I don't hold a strong opinion on either option.
I'm happy to approve this PR, pending the typo tweaks, but it might pay to seek an extra confirmation from someone more well-versed in stabilizing components or APIs in Gutenberg.
Patch for typos
diff --git a/docs/how-to-guides/themes/theme-json.md b/docs/how-to-guides/themes/theme-json.md
index 0003404df5..8e2b60a769 100644
--- a/docs/how-to-guides/themes/theme-json.md
+++ b/docs/how-to-guides/themes/theme-json.md
@@ -359,7 +359,7 @@ The following presets can be defined via `theme.json`:
- `steps`: the number of steps to generate in the spacing scale. The default is 7. To prevent the generation of the spacing presets, and to disable the related UI, this can be set to `0`.
- `mediumStep`: the steps in the scale are generated descending and ascending from a medium step, so this should be the size value of the medium space, without the unit. The default medium step is `1.5rem` so the mediumStep value is `1.5`.
- `unit`: the unit the scale uses, eg. `px, rem, em, %`. The default is `rem`.
-- `spacing.spacingSizes`: themes can choose to include a static `spacing.spacingSizes` array of spacing preset sizes if they have a sequence of sizes that can't be generated via an increment or mulitplier.
+- `spacing.spacingSizes`: themes can choose to include a static `spacing.spacingSizes` array of spacing preset sizes if they have a sequence of sizes that can't be generated via an increment or multiplier.
- `name`: a human readable name for the size, eg. `Small, Medium, Large`.
- `slug`: the machine readable name. In order to provide the best cross site/theme compatibility the slugs should be in the format, "10","20","30","40","50","60", with "50" representing the `Medium` size value.
- `size`: the size, including the unit, eg. `1.5rem`. It is possible to include fluid values like `clamp(2rem, 10vw, 20rem)`.
diff --git a/packages/block-editor/README.md b/packages/block-editor/README.md
index b893a6a93e..2b6897c3c6 100644
--- a/packages/block-editor/README.md
+++ b/packages/block-editor/README.md
@@ -401,7 +401,7 @@ _Returns_
### getComputedFluidTypographyValue
-Computes a fluid font-size value that uses clamp(). A minimum and maxinmum
+Computes a fluid font-size value that uses clamp(). A minimum and maximum
font size OR a single font size can be specified.
If a single font size is specified, it is scaled up and down by
@@ -757,7 +757,7 @@ to try to find a match we need to things:
1\. Block's client id to extract it's current attributes.
2\. A block variation should have set `isActive` prop to a proper function.
-If for any reason a block variaton match cannot be found,
+If for any reason a block variation match cannot be found,
the returned information come from the Block Type.
If no blockType is found with the provided clientId, returns null.
Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>
|
Thanks for the reviews @ciampo and @aaronrobertshaw! I've committed the typo fixes (cheers for providing the patch, made it nice and easy to apply locally 👍) Since I think all three of us are slightly hesitant about public changes to the |
|
Flaky tests detected in 54f7c11. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4040352293
|
|
Looks good to me. As I understand it our deprecation policy is:
So yes this is fine. |
|
Thanks for confirming, Rob! 🙇 |
andrewserong
deleted the
try/stabilise-height-control-component-in-block-editor
branch
Screenshotted and printed |
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.
What?
Part of #47196 — this PR looks to stabilise the
HeightControlcomponent within the block editor package.Why?
As flagged in #47196, it'd be good to either lock or stabilise APIs that are new for WordPress 6.2. I was a little torn between keeping the height control locked and stabilising it, however after thinking it over I'm leaning a little toward stabilising it for the following reasons:
How?
exportfor the component to remove the__experimentalprefix.appearanceToolsto flag thatdimensions.minHeightis included.Testing Instructions
A question for the reviewer: are there any other changes that need to be considered when stabilising this component? Also, if anyone has strong feelings about locking this component instead, do feel free to raise them — I don't feel too strongly about stabilising vs locking, but decided to lean toward stabilising as a documented feature feels better to me than an undocumented one.