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

Block Library: Implement drag handles for columns resizing #15927

Closed
wants to merge 2 commits into from

Conversation

aduth
Copy link
Member

@aduth aduth commented May 30, 2019

Closes #15659
Cherry-picks 9688deb from #15920

This pull request seeks to enable columns resizing using draggable handles on the Columns block.

columns-drag-resize

Note: This pull request is currently in progress.

Progress notes:

  • The current implementation of columns margins are actually mathematically incorrect in most cases, due to the browser's distribution of widths from its flexbox styling. It became exceedingly obvious when implementing the drag handles, as the "intended" size (by visual placement at time of drag release) would not be reflected correctly.
    • Temporarily, I work around this by disabling flex resizing (flex-grow: 0, flex-shrink: 0), which obviously causes undesirable content overflow, but allows for verification of the attributes math and general behavior.
    • It might be possible to refactor the flex styling so that even if margins exist and the columns must shrink, they do so proportionally. In master, the implementation is off in part because the columns are imbalanced (left-margins on all but the first column).
    • cc @jasmussen in case you have advice here.
  • This may be considered blocked by both Block Library: Column: Move resizing controls to Columns block #15660 and Blocks: Consider a "passthrough" supports for nested block wrappers #7694, if the intention would be that these resizing controls are only available at the parent Columns block. Existing text controls for assigning width must be preserved (currently at Column inspector, presumably desired at Columns inspector).

Implementation / Code Review Notes:

Review with white space changes hidden (columns/editor.scss are largely de-indentation).

The changes from 9688deb are temporary until #15920 to facilitate styling development. (#15920 was merged)

Much of the code from the resizer's onResizeStop callback is copied directly from the implementation in the Column block. I expect most of this could be consolidated back up to the Columns parent component, notably as part of #15660.

A change was included for blocks selection state handling of selection toggling. It's expected this was recently changed to an unexpected behavior. Without the revision, the Columns block becomes deselected when starting to use the drag handles. See also #14640 (comment) (cc @ellatrix).

Testing Instructions:

Verify that upon selecting a Columns block, you can see and use drag handles between columns. Verify across a multitude of columns in a block.

  1. Navigate to Posts > Add New
  2. Insert a Columns block
  3. Press ArrowUp to select the parent Columns block
  4. Note the drag resizer
  5. Drag the resizer
  6. Note that the Column blocks are resized upon drag release

@aduth aduth added [Package] Block library /packages/block-library [Block] Columns Affects the Columns Block labels May 30, 2019
@aduth aduth added the [Status] In Progress Tracking issues with work in progress label May 30, 2019
@jasmussen
Copy link
Contributor

This is nice work. Thank you for taking a stab. I can't imagine how technically complicated this must've been.

I wonder, though, if it makes sense to look at the passhtrough feature for nested blocks first, though, and a different way to set alignments for individual column blocks. Acknowledging that the clickthrough PR that I have in queue will certainly mitigate the need to select the individual column block (if clickthrough were in master now, clicking the columns block would select that first, immediately surfacing the resize handle) — the fact that individual columns are still selectable seems like an added complexity that adds still more complexity to the resizing math.

I note that because in my experience, the challenge with the flex rules here are squarely in the editor, and to a vastly lesser extent on the frontend. It's the multiple additional div elements that the innerblocks container classes add, that makes it difficult to know what to target with which flex rules to map to the frontend. Whereas with fewer CSS classes and a flatter hierarchy, I imagine the flex rules get much simpler.

I don't know how helpful that was, sorry — I'm sure we can make the current approach work. It just appears much more complicated.

@aduth
Copy link
Member Author

aduth commented Jun 3, 2019

@jasmussen Thanks for the feedback! And I definitely agree that as implemented in this branch, the experience becomes quite disjointed and convoluted. I predicted that this pull request probably shouldn't be merged as-is until those parallel efforts can help improve the situation. I pushed it in-part hoping to try to resolve some of the flex issues. It hadn't occurred to me that by flattening the hierarchy we might be able to simplify these styles. So in that regard, your comment was definitely helpful.

As action items then, I'd propose:

  • Evaluating the behavior of the the draggable interactions in mind that other improvements to column selection and resize fields are set to be addressed separately
  • Separately address those behaviors
  • Try to consider ahead of this how we might position ourselves to solve the issues with Flexbox styling calculations

@ZebulanStanphill
Copy link
Member

What's the status of this PR? I would imagine that this should be at least slightly easier to implement now thanks to all the improvements that have been made to... well, everything.

@aduth
Copy link
Member Author

aduth commented Apr 3, 2020

@ZebulanStanphill If my memory serves correctly, I think the root of the issues here (with regards to calculating in consideration of paddings, etc) won't be especially improved by the other related changes to columns.

I definitely think there's high value in these changes and would like to revisit it if I can dedicate some time to better understand how to work within and to improve those calculations. The paddings is pretty delicate (fragile) styling in the editor, which is part of the reason this one lost steam. It may very well be in a better state as of some of the bigger recent styling changes, like #19344.

@aduth
Copy link
Member Author

aduth commented Jun 4, 2020

While it's still something which would be very good to have, the branch has fallen quite out of date and will likely need a refresh in the form of a new pull request. I know @ItsJonQ has been looking at this separately as well.

Closing in the meantime.

@aduth aduth closed this Jun 4, 2020
@aduth aduth deleted the update/columns-drag-resize branch June 4, 2020 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Columns Affects the Columns Block [Package] Block library /packages/block-library [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Block Library: Column: Add draggable resize control
3 participants