Separator: Add means to control height #28409
Conversation
aaronrobertshaw
added
[Block] Separator
In this interation the separator isn't showing the box around it as the resizable box is resize like the coblocks variant. The resizing is smoother and CSS is tweaked without markup changes such that there should be no breakage. Still needs work though.
|
Size Change: -13.8 kB (-1%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
This approach has the downside of needed to overcome theme CSS to be able to display the separator style in a different way for it to be vertically centered.
This change keeps block selection styling and visual cues without losing the normal styling of the `hr`. It also allows the ResizableBox control to naturally represent the separator's height. The downside is it is a little hacky to get around themes styling separators using bottom borders, background colors and pseudo elements.
|
As far I remember this blocks is cross-platform and works in React Native. I anticipate this refactoring will break it in the mobile app. @hypest, I'm flagging it so the mobile team could advise on how to move forward in a seamless way. |
|
Thanks for the ping @gziolo , I opened wordpress-mobile/gutenberg-mobile#3041 on the gutenberg-mobile side to have the extended native mobile tests run. We can look into this deeper if any test fails 👍 |
|
Hey 👋
Indeed, it's broken using this PR. One issue is related to accessing an undefined variable:
Second, is because it's using
We had similar issues for this block before. I'd suggest avoiding using Please let us know if there's something we can help with to not block this PR, even if it's just testing any new changes. Thanks! |
|
@aaronrobertshaw I think this should also include an input field so the user knows what height has been set, and adds the ability set a specific value. Much like the search block input minus the percentages?
|
|
Or we can also use the same setup as the Spacer block.
@ItsJonQ and @jasmussen might also want to know about this PR. |
* Add sidebar for height control * Add optional chaining to call to strip class from `className` to prevent error in mobile * Replace div with View component for mobile use
|
Thank you everyone for the testing and feedback 👍
A control for the separator's height has been added to the sidebar matching the control in the spacer block as suggested.
I've added optional chaining to the call to replace the problematic class from the Regarding the use of @geriux it would be good to take you up on that offer to give this PR another test. I also attempted simply adding the height control to the Open to any suggestions on that.
|
|
Another approach to adding margins or height to the separator block can be found in #28451 created today. |
aaronrobertshaw
changed the title
|
#28451 contains a very fascinating suggestion about how to better control where the line is seen in the Separator block! |
|
Just my 2c: On my FSE theme I have responsive/adaptive typography (see theme.json). If the height is limited to |
|
This is one reason to use the same control as the search block. You can then select different units, including ems. |
|
I've updated this to use a I'd say it would be possible to combine this with the approach using the margins-based spacing support. The PR description has been updated with a new GIF of the current UI. |
|
After evolving this PR to handle different units, Notes:
Next steps:
|
@aaronrobertshaw related to the things you mentioned for the vertical alignment issue and the settings, I pushed some changes to another branch and I was able to have it working like this:
One thing I had to change was the default height for the main container, not sure if that would break your current work. Let me know! Thanks! |
|
@geriux Thanks for all the effort working on the mobile side to this! 👍
Unfortunately, that does break the functionality allowing non-pixel units such as I haven't been able to spend much time exploring this yet but will be able to take a deeper dive on Monday. My guess is we need a dedicated Using the cover block as an example, it allows non-pixel units for its minimum height, but requires converting those non-pixel values via gutenberg/packages/block-library/src/cover/edit.native.js Lines 106 to 109 in 37feae1 |
|
I've taken another run at updating the display and control of the separator block's height on mobile. It's a bit rough at the moment and could do with a refactor, which I'll do if the general approach gets the nod.
|
That looks great! Thanks for making the changes for mobile!
Yeah, I agree. It made more sense to share the same file before since it was a very simple component.
Sounds good 👍 |
I think there's been some recent regression around block selection that has something to do with that. I haven't seen it reported but was meaning to get to that. On an implementation note, it's interesting that the height is applied through margins. Say this lands and then later margin is added to spacing block support. We'd have to avoid enabling margin for the Separator block (and forgo control of individual sides). Or, if enabled, we'd have to remove the height control (or maybe have the height update as the margins update, which seems funky). I wonder about the need to support this feature for now. Padding support is stable, so putting a Separator in a Group to be able to apply padding seems like an easy enough workaround for what I believe is the infrequent need to space out the Separator (if the need is frequent for some particular user, making that combo a reusable block is also an option). It does seem like a good thing that the mobile version would support color settings though. |
|
Thanks @stokesman, I appreciate your time taking a look at this PR and the work on the alternative approach.
The current situation is less than ideal, it definitely shouldn't be so hard to click to select the block. Given this is the primary means of block selection, I'd say it's pretty important to address.
Given the current methods of styling the separator's I don't have very strong opinions on how a custom separator height is implemented behind the scenes. One big issue though is the UX around how the user achieves that custom height. I think the ability to click a drag handle in the editor itself to visually resize the height of the block is a must. It is the most intuitive means of doing this and is consistent with the Spacer block. The Being able to adjust the top and bottom margins independently had been considered however I thought it best to leave to a future iteration. Left and right margins I think will interfere too much with block alignments.
We have a number of block patterns driving the need for this rather soon. I think you are right, wrapping the separator in a group block could achieve a similar outcome. For the average user though, I do not think that is going to be very intuitive or even thought of. |
|
Thanks for the thorough reply @aaronrobertshaw
I'd been concerned about the feasibility of a future iteration, at least with regards to integration with spacing block supports. Akin to what is brought up in #28518 (which you may have good suggestions for given your work on the PRs mentioned within). After further consideration, any concern of mine is gone. I've realized that even if an ad-hoc implementation here could make difficult any later block spacing integration, the features that could be enabled by that (and are probably more niche uses anyway) can be had with the idea I'd suggested previously. E.g. If it's desired to adjust the spacing on the sides it can put it inside a Group to provide the padding. One idea I had for this PR is that maybe it wouldn't hurt to label the setting “Vertical margins” since that's actually what gets adjusted. Plus it makes sense for splitting out as top and bottom in a later iteration. |
At this stage, I don't know what the outcome of #28518 will be. It seems to me that a lot of things we need to accomplish are blocked by that. Leaving the user to go without a feature or suffer an inferior experience to what we could provide. Even though the preferred way to add styles is via block supports, the current proposed approach to spacing doesn't provide the usability of the resizable box. This block could be argued to add a new category to the linked issue where the block support controls are insufficient to a block's needs. The downside is the inability to leverage global styles or the theme.json for this first iteration. Themes can already target the separator block with standard CSS so the sacrifice appears to be a user not being able to override that via global styles.
In the case of this PR, despite the complexity around the UI, it really comes down to setting an attribute representing the height and then using that to apply margins when the block is saved. That attribute could be Given the wide spread implications of the spacing controls using margins, I didn't see that being something that landed in the near future. |
|
A couple of issues I saw in testing this and #28688 were that the first attempt to drag the handle doesn't work (unless height was already established by setting it in the inspector controls) and the height does not update while dragging (unlike in the Spacer block). |
The issue preventing drag resizing on initial insertion has been fixed. The height isn't currently set in the attributes until the resize is complete in the |
Not surprising but I'd guess no worse than other blocks' resizing operations. Also, I do find any dragging-based interaction in Gutenberg to get much chunkier when the dev tools are open. |
- Adjusts height minimums so they are effectively the same regardless of block style such as Dots. - Height is now set on resize not resize stop - Refactored edit component and settings controls to be clearer
|
This now sets the height |
|
I think we should concentrate on #29363 and look to add the handle in a future iteration. |
|
Thanks @apeatling , I'll close this PR in favour of #29363 |
Description
How has this been tested?
Manually.
Testing Instructions
px,emorrem. Switching units will reset the height. Adjust it again via the sidebar control and confirm visual height of block adjusts.Native Testing Instructions
npm run native startandnpm run native iosScreenshots
Previous pixel height only iteration screenshot
iOS
Types of changes
New feature.
Checklist: