-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Separator: Trial custom separator height with independent top/bottom margins #28688
[WIP] Separator: Trial custom separator height with independent top/bottom margins #28688
Conversation
Size Change: +528 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
I like that this approach should ease and make cleaner (no attribute deprecation) a transition to stock spacing controls for margins if they do ever land and as you said in the other PR:
Since there are two levels of uncertainty there and because I was curious to try it, I messed around with alternative inspector controls (basing off this PR). I made something that goes like this: separator-margin-splitter.mp4I think some of it may be good (probably not the arrow icons). It does mean sharing the unit across top and bottom, unlike the PR at present. The odd thing implementation-wise is it's not clear that such customization of inspector controls is precedented in blocks (I'm not sure that the CSS for such is acceptable in the block editor styles) so maybe the A newer idea I had was maybe two resize handles could be employed. Though that'd probably want the presence of the second to be controlled via a lock/unlock toggle for the side controls in the inspector. All this could be taking the exercise further than needed for a feature that I still imagine to be niche. If the use cases you have in mind @aaronrobertshaw, can all be met with the simple evenly distributed margin I'd say your former PR is fine. Though maybe it'd be finer also using the style/spacing attributes 🤷♂️ . |
Thanks for the extra feedback 👍 There's definitely an opportunity to refine the controls once a decision is made on general approach. We need to consider the controls for native as well.
I'm not sure I follow, this PR does store different top and bottom margin values. If there is a single control field, that value can be split into the two attributes, similar to the way that the singular height value from the resizable box gets split now.
It could be possible to do something here like that. I had avoided it previously in an attempt to keep more consistency with the spacer block. Another possible concern is adding another layer of complexity to the usability and intuitiveness of the controls. As you suggested, perhaps if the second drag handle was locked behind a control toggle or something it might help.
I believe there will be plenty of use cases for designers to utilise custom separator heights within block patterns to be used across themes. That said, I agree adding a second drag handle for now might be a step too far. It could easily be revisited in the future.
I've fixed the issue preventing drag resizing on first insertion of the block as mentioned in #28409 (comment). At this stage, I had opted out of updating the height |
Right, I was contrasting this PR with the version seen in the video that has only one |
When dev tools is open this is pretty jumpy but others feels smooth enough.
- Adjusts margin minimums so they are the same regardless of block style such as Dots. - Refactored edit component moving helpers into shared file to clean up
This has been updated so that height is adjusted on resize rather than when the resizing stops. In addition, I've made the minimum margin defaults the same regardless of block style. Following that this PR has had a bit of a refactor to clean things up and make it a little easier to follow. One issue still outstanding is with the |
I think we should concentrate on #29363 and look to add the handle in a future iteration. |
Closing this in favour of getting #29363 merged as first iteration. |
Description
Trial exploring splitting a custom separator height into both top and bottom margins, while still providing the ability to drag a resizable box handle to visually set the height.
This PR is based off #28409
style.spacing.margin.top
andstyle.spacing.margin.bottom
attributes to store the valuesNext Steps
How has this been tested?
Manually.
Testing Instructions
TBA
Screenshots
Types of changes
New feature.
Checklist: