Separator Block: Add top and bottom margins without resizable box #29363
Conversation
aaronrobertshaw
added
[Type] Enhancement
|
Size Change: +24 kB (+2%) Total Size: 1.41 MB
ℹ️ View Unchanged
|
|
Tested and this works for me. I think this approach makes the most sense in terms of a first iteration. However I think we'd need to follow up and replace this custom margin UI with the margins component (and allow it to disable certain margins so only top and bottom margins are editable). |
|
@aaronrobertshaw Could this seamlessly switch to using the margins component in a future PR? My other thought is we spend the time on the margins component now to allow disabling of specific margins, so we can use it here before merging this. |
Thanks for taking a look at this @apeatling. Yes, I believe it will be a simple switch in the future to use the margins component. Especially if it used via the block support spacing feature which would store the margin values in the same
I'm happy to take a look at the BoxControl and see how we might approach allowing opting out of particular sides. It would definitely be the cleanest approach for the UI. Perhaps being able to opt out of particular sides would also help to offer margins via the spacing block support feature. It could then have left/right margin support disabled by default so they wouldn't interfere with block alignments. |
aaronrobertshaw
added
the
[Status] In Progress
aaronrobertshaw
changed the title
aaronrobertshaw
added
[Status] In Progress
|
Added box visualizer to web controls. Native still uses individual fields for top and bottom margins. |
aaronrobertshaw
changed the title
aaronrobertshaw
removed
the
[Status] In Progress
|
Nice work @aaronrobertshaw, this is testing well for me manually in the web editor (I need to fix some thing on my laptop before I can get the iOS simulator running properly to test RN). One thing I noticed in the UI for the BoxControl is that the All four sides are highlighted
One side is highlighted after selecting "Unlink sides"
|
Thanks @andrewserong, that would definitely be an improvement. I'll take a look at making it happen 👍 |
|
The
|
|
That's looking really good, thanks for updating the I noticed one slight issue with the
I couldn't quite work out why it wouldn't be updating Other than that, this is looking good to me (though I must admit this is the first time I've looked at any of the React Native code!) |
|
Thanks @andrewserong for the thorough testing.
The If the It's worth noting that pressing the up/down arrows on your keyboard to increment/decrement the value will immediately trigger the |
|
Thanks for getting to the bottom of that @aaronrobertshaw! It sounds like it's by design, then. I couldn't see if we've used the I can see the value of preventing My personal preference would be to go for consistency with keyboard input behaviour across these sorts of inputs, but I think I'd lean toward leaving the discussion about the best UX for the BoxControl component itself to another PR, and leave it as-is in this one, if it doesn't seem like an issue for anyone else. |
There was a problem hiding this comment.
Nice work @aaronrobertshaw, this is testing nicely for me in web and in the iOS simulator, and I think we can defer the BoxControl onChange UX to a separate issue / discussion.
It'd be good to get another pair of eyes from someone with more React Native experience than I have, but otherwise LGTM!
Description
This adds the ability to set top and bottom margins on the separator block.
HorizontalRuleto allow margin styles.How has this been tested?
Manually.
Testing Instructions
Web
Native
Screenshots
Types of changes
New Feature
Next Steps
Update controls for Native.
Checklist: