Skip to content
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: allow changing the thickness / height #20758

Closed
mtias opened this issue Mar 10, 2020 · 7 comments
Closed

Separator: allow changing the thickness / height #20758

mtias opened this issue Mar 10, 2020 · 7 comments
Assignees
Labels
[Block] Separator Affects the Separator Block [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi [Type] Enhancement A suggestion for improvement.

Comments

@mtias
Copy link
Member

mtias commented Mar 10, 2020

The separator block should allow setting a different height / thickness:

image

Should also apply somehow to the dots version:

image

@mtias mtias added [Type] Enhancement A suggestion for improvement. [Block] Separator Affects the Separator Block [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi labels Mar 10, 2020
@aaronrobertshaw aaronrobertshaw self-assigned this Jan 19, 2021
@apeatling
Copy link
Contributor

I think it would be good to explore using the padding control to manage this.

@mtias
Copy link
Member Author

mtias commented Jan 21, 2021

I think there are two different things here. Height makes sense to follow the spacer and cover treatment, with the resize handle at the bottom. Thickness is something that seems specific to this block.

@apeatling
Copy link
Contributor

This is the thought @aaronrobertshaw and I came to when chatting yesterday, going to concentrate on height first using the resize handle.

@apeatling
Copy link
Contributor

Latest #29363

@aaronrobertshaw
Copy link
Contributor

The latest PR addressing separator height is ready for some extra eyes and testing.

A few approaches have been trialled for this so I'll give a brief recap below.

First Approach: #28409

Single height attribute with drag resizing

  • Used single height attribute
  • Used ResizableBox to allow drag resizing on web
  • Had to wrap the separator's <hr> in a wrapper to include ResizableBox as a sibling due to issues with how the separator block is being rendered
  • Native omitted the ResizableBox and used a simple UnitControl field
  • While the user experience was ok, the edit component was more complex than seemed warranted

Second Approach: #28451

Adding margins to spacing block support and opt in for separator block

  • Updated spacing block support to include margins
  • No drag resizing
  • Application of left/right margins breaks alignments
  • Did appear to be a direction we'd like to go in the future
  • Likely to be slow to get merged given the wide-spread implications of arbitrary margins
  • No native support at present

Third Approach: #28688

Separate top/bottom margin attributes and drag resizing

  • Split height attribute into separate top and bottom margin attributes
  • Top/bottom margins were stored in the same attributes a future implementation of margins block support would use
  • Still offered the user friendly drag resizing and visual feedback of changes
  • Suffered same issues as the first approach resulting in more complicated edit component than seemed acceptable
  • Native controls were simply two independent UnitControl fields to set top/bottom margins

Fourth Approach (preferred): #29363

Modified BoxControl, separate top/bottom margins without drag resizing

  • Separate top and bottom margin attributes
  • Shouldn't require a deprecation when possible future implementation of margins block support lands
  • Simplifies the separator edit component greatly by not using the ResizableBox
  • Updates the BoxControl so that arbitrary sides can be opted out of
  • Updated BoxControl should help margins block support later down the road
  • The BoxControl used in the separator controls is configured to allow only top/bottom margins to be set
  • Box visualizer provides comparable visual feedback to the ResizableBox
  • Also fixes all the errors spat out in console when using a drag gesture within the box control's unit control
  • Native controls have been updated and are two simple UnitControl fields

Hopefully that helps give some extra context to the PR when reviewing. I think the last option is definitely the best option now and with a view to the future.

@mtias mtias mentioned this issue Jul 15, 2021
65 tasks
@aaronrobertshaw
Copy link
Contributor

The last PR attempting to address this has been closed in favour of leveraging block gap support and the spacer block. Should we close this issue as well or is the functionality still desired?

@apeatling
Copy link
Contributor

Let's close this because it seems spacer block is the direction going forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Separator Affects the Separator Block [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi [Type] Enhancement A suggestion for improvement.
3 participants