-
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
Separator Block: Add top/bottom margins via block support and modified BoxControl #30609
Conversation
Size Change: +13 B (0%) Total Size: 1.09 MB
ℹ️ View Unchanged
|
8ae61ea
to
faa04c8
Compare
89348c2
to
d27643d
Compare
faa04c8
to
c09835c
Compare
d27643d
to
c3f44dd
Compare
c09835c
to
2029c03
Compare
2029c03
to
0fe1917
Compare
c3f44dd
to
10e6f60
Compare
0fe1917
to
dfe3ff8
Compare
10e6f60
to
e5db3b8
Compare
This has been rebased after the changes on trunk to padding.js and spacing-panel.js fixing custom CSS units. |
011895b
to
badcb34
Compare
e5db3b8
to
496b99a
Compare
badcb34
to
07b49e7
Compare
496b99a
to
09d30db
Compare
07b49e7
to
d8bf09e
Compare
09d30db
to
92abe52
Compare
d8bf09e
to
f976e9e
Compare
92abe52
to
40c82c9
Compare
f976e9e
to
a3c0200
Compare
0f57553
to
6f56746
Compare
Rebased after recent updates to the margin support. |
6f56746
to
fe6032a
Compare
} } | ||
/> | ||
</View> | ||
<SeparatorSettings { ...props } /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason we need all these changes to the block's edit function. The idea of the block supports is to "automatically" add support without changes, what's preventing us from doing that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hr
element used for the separator presents a couple of problems in the editor, mainly the ability to select it and visualize the changes made to margins.
The original issues related to this block expressed a desire to control the "height" which really related to margins instead. If we don't have a true height for the separator it makes the separator rather hard to select by click on it with the mouse. Adding a wrapper around the hr
allows the margins to expand within a clickable container drastically improving selection.
Earlier iterations on this included a resizable box so the user could drag to resize the separator. While it was decided the drag resizing overcomplicated the block, keeping the visualizer was still desirable. The visualizer couldn't be added within the hr
so also benefits from being in the new wrapper element.
The additional changes to the application of the styles were to make the separator display as expected.
In terms of saving the margins, that works as per normal "automatic" block support. The frontend is still only a simple hr
with top/bottom margins. The bulk of the other changes in this PR are to offer the same functionality in native for the separator block as it has on the web.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the challenges here, though the extra wrapper creates a diversion between the frontend and backend that we tried to illiminate in blocks over time because it makes styling the editor for themes very hard and create a lot of inconsistencies.
It's maybe fine for this particular block but would be good to have themers perspective. cc @scruffian @carolinan
The visualizer couldn't be added within the hr so also benefits from being in the new wrapper element.
I'm aware of this and it's the reason we removed the visualizer temporarily from other blocks like the "group" block. I think it's be good to have the visualizer work a bit differently to solve this (don't rely on a parent element but maybe a "ref"/popover)
Do you think these issues: visualizer and block selection are absolutely necessary in this PR or do you think we can iterate on them separately while still keeping the front-backend parity (maybe using alternative solutions like the proposal above)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think these issues: visualizer and block selection are absolutely necessary in this PR or do you think we can iterate on them separately while still keeping the front-backend parity
The visualizer and easier block selection are not non-negotiable.
I've updated this PR to remove the visualizer and minimise changes. The majority of the updates are now related to Native and adding color and margin controls there. The PR has been rebased as well.
6c0c304
to
f116adf
Compare
d98a0bf
to
f4be82a
Compare
@mtias do you have any thoughts on whether we should proceed with this PR or close it for now? This has been through several evolutions to arrive at the current state. It no longer adds a visualizer for the margins, nor includes a resizable box to adjust the height/margins. It simply applies top and bottom margins to the Separator via block supports along with updating the native controls to manage margins there also. Given the most common approach to managing spacing between blocks appears to be adding a spacer block. Does it make sense to continue with this PR? Demos of Separator block in this PRWeb: Screen.Recording.2021-11-05.at.2.48.22.pm.mp4Native: Screen.Recording.2021-11-05.at.2.45.03.pm.mp4 |
Closing this PR as there is a general movement towards using block gap support or the spacer block over margins. |
Description
Allows control of the separator block's top and bottom margin via the new margin support added in #30608. This in turn leverages changes made to the BoxControl in #30606 to allow configurable sides.
How has this been tested?
Manually.
Test Instructions - Web
spacing.customMargin
Test Instructions - Native
Screenshots
Types of changes
New feature.
Checklist:
*.native.js
files for terms that need renaming or removal).