Spacer: add custom units for height and width #36186
Conversation
stacimc
added
[Status] In Progress
|
Size Change: +583 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
stacimc
changed the title
|
For me the this works in the editor interface as expected, but it isn't saving the selected unit to the attributes: |
| @@ -24,6 +31,133 @@ const MAX_SPACER_HEIGHT = 500; | |||
| const MIN_SPACER_WIDTH = 1; | |||
| const MAX_SPACER_WIDTH = 500; | |||
|
|
|||
| function DimensionInput( { | |||
There was a problem hiding this comment.
Would it be worth exporting these smaller components and adding some unit tests? Also, I know it is common to have multiple functional components in a single file, but I wonder it is worth trying to move more to a single file -> single component approach? I don't have a strong opinion on it, but it does seem that some of the core block files can get quite long and complicated.
stacimc
force-pushed
the
add/custom-unit-to-spacer-dimensions
branch
from
0f81675
to
c57a4bb
Compare
|
Thanks for testing, @glendaviesnz! This has been rebased and updated to respect units on the frontend :)
There's a lot that could be done here. I think it might be a good idea to make a |
|
Thanks for this PR. Additional unit support has been requested quite a few times, and I think you'll make a lot of people happy with landing this. I took it for a spin, and it worked rather well for everything but percentage units: Do you know what's up with percentage units that's making that one tricky? My best guess: the problem is that percentages are relative to the parent, making it hit or miss whether percentages work at all. I.e. if it's inside a parent with zero height, the spacer will be zero height. In that light, can we disable just the percentage unit from spacers? Even doing so in a UI-only way (not preventing themers from still defining them in the code if they want to) would be fine. |
This seems fine to me. 👍 |
stacimc
force-pushed
the
add/custom-unit-to-spacer-dimensions
branch
2 times, most recently
from
36b8192
to
bec1ef2
Compare
stacimc
added
[Status] In Progress
Thanks for catching this! I think disabling the percentage unit is a good idea, as the majority of the time they will not be in a parent container where this makes sense/is meaningful. Updated. |
There was a problem hiding this comment.
Took this for a good spin, and in my testing it works very well, with no perceptible downsides. It works vertically and horizontally and the units appear to behave as they should:
The omission of percentage makes complete sense to me, and feels like a good solution.
I'd encourage a sanity check on the code, but overall, this feels solid. Thank you for your work!
|
Looks like the snapshots need to be updated. |
|
Question: Is it possible to use dynamic CSS values like variables or The main use case I have in mind is this space beneath the header in Twenty Twenty-Two. I'd love to use a spacer to replace that, but to keep it in sync with other areas of the theme we currently use a This wouldn't need to be visible in the UI, but if there were a "custom" unit size or something that would allow other values that would be great. |
|
Hi, @stacimc Thanks for working on this 🌟 What's the reason for storing unit values as separate attributes? |
@Mamaduka @kjellr brings up an interesting limitation of using separate attributes. Edit: I may have been incorrect in assuming that changing the attribute type would require a deprecation. I'll play around with this a bit more. |
|
Also, I hope you don't mind but I rebased the PR branch again in the hope that picking up recent commits such as #37501 will get the failing e2e test passing. |
Thank you!
The step value comes from the configuration here. It's the same for the Cover block px-vs-em-step.movInvestigating, it seems like different steps were chosen for each category of units, with |
That was my thinking, to address it in a separate PR. It could be a little subjective as well. Another perspective to consider is that most using the step functionality will be doing so to fine-tune a value rather than set it for these units. The small step value makes more sense in this case. |
Sounds good. I opened #37620 to allow for discussion. Merging, but I'm not clear if this should receive a |
|
👋 I'm unsure if it requires back porting. We haven't used this in Twenty Twenty-Two at this point, so it's not technically required. Though we do have WordPress/twentytwentytwo#272, which could technically be solved by this. I'll take a closer look in the next day or so. |
|
I opened WordPress/twentytwentytwo#319 to use these custom units in some of Twenty Twenty-Two's patterns and templates. This fixes a bug we had with the header template margin not being editable, and I think it's a very noticeable improvement. So let's try and backport this for release. cc @noisysocks. |
kjellr
added
the
Backport to WP Beta/RC
|
We're past RC 1 unfortunately so not able to land new functionality. If this isn't technically required, like you say, then It think let's leave it for WP 5.9.1. |
noisysocks
added
Backport to WP Minor Release
|
@noisysocks I think this one is a bit of a special case — it's technically a new feature here in Gutenberg, but it fixes a somewhat obvious bug in Twenty Twenty-Two: the fact that you can't edit the space here via the UI:
If this were something we could fix in the theme, I'd do it, but unfortunately it's not (at least, not in a way that works well for mobile + desktop). We tried to fix it before in #37105, but decided to continue pursue the approach in this PR instead. I understand why we'd avoid pushing it, but I think it's worth a thorough consideration at least. That space is one of the first things a user is going to see when they activate Twenty Twenty-Two, and it's rough that there's no clear way to edit it. |
|
If we decide to backport this, we should also include #37774. |
|
Sorry, it's too late. There's a bit of new functionality here which puts it more towards the enhancement end of the spectrum. Also there are new strings which would not be localised in time as we're now past the RC 1 hard string freeze. We can include it in 5.9.1. We usually do a quick minor release after releasing a version with so many substantial changes (e.g. 5.0.1 and 5.0.2 came pretty quick after 5.0). |
|
👍 No worries, thanks. |
|
Cherry picked for 5.9.1. |
Mamaduka
removed
the
Backport to WP Minor Release
* Add width and height units and use the custom UnitControl * Refactor out the ResizableSpacer and fix resizing in the canvas * Correctly initialize width and height for horizontal spacer * Handle min and max values for px * Apply unit to the dimensions on the frontend * Refactor out the controls * Disable percentage units for Spacer * Only output width or height styles as needed for orientation * Correctly generate styles when width or height is set to 0 * Remove unnecessary parsing, interpolation * Remove separate unit attributes and update type of height and width attrs * Add block deprecation * Update and add fixtures for deprecation * Update snapshots * Ensure that the unit is updated to px when resizing by dragging the handle * Add mobile support for custom units
#22956
Description
This PR adds custom unit support to the height and width controls of the Spacer block. It pulls in much of the logic for the same from the Cover block's minHeight support.
Notes
How has this been tested?
Vertical spacers
Insert a Spacer block into a post.
Horizontal spacers
To test a horizontal spacer, insert the Navigation block and add a Spacer block to it.
Mobile
Test on mobile simulators with
npm run native start:resetto start the packager in dev mode, andnpm run native ios/npm run native android. See screenshots.Screenshots
spacer.mov
mobile-spacer.mp4
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).