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

Blocks: Render columns as nested column (wrapper) #7234

Merged
merged 7 commits into from
Jul 3, 2018
Merged

Conversation

aduth
Copy link
Member

@aduth aduth commented Jun 8, 2018

Closes #5351

This pull request seeks to address an issue where the current Columns block implementation using grid is insufficient for supporting columns of varying height where it is expected for blocks within a column to stack atop each other, thus resulting in undesirable empty space.

Before After
Before After

Implementation notes:

This explores one option of addressing #5351 which was considered at #5351 (comment), where in order to introduce a wrapper element for each column, the columns themselves are an intermediary block. This is possible through new APIs for supportedBlocks of a block type (#6753) and allowedBlocks and template of an InnerBlocks (#6546). Thus, a "Column" block is only valid within a Columns wrapper, and conversely a Column block is the only valid child of a Columns block. This will require that the Columns block fully manages its own template, which is only partially implemented thus far.

This may allow for the eventual removal of layouts concept altogether.

The primary alternative here is to keep the distinction of "grouped" and "ungrouped" layouts (documentation), and automatically insert a wrapper element for grouped layouts. This is challenging both in the further inconsistency between the two types of layouts, and in the current unavailability of layouts anywhere outside of the edit implementation where it is passed (it would likely need to become a top-level property of the block).

Testing instructions:

Verify that you can create columns, insert blocks within (via slash insertion), and that within columns, the individual blocks stack neatly atop each other when previewed on the front of your site.

@aduth aduth added [Feature] Blocks Overall functionality of blocks [Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P [Status] In Progress Tracking issues with work in progress labels Jun 8, 2018
@chrisvanpatten
Copy link
Member

The UX with the nested blocks is a little funky, and changing the number of columns doesn't work yet, but it does feel like a step in the right direction.

Excited to see this evolve further!

@ZebulanStanphill
Copy link
Member

ZebulanStanphill commented Jun 8, 2018

I made an issue suggesting this sort of thing a while back: #6461. I backed off the idea after looking into the Layout block mockup in #5351, which I thought would be a better solution that could replace the Columns block entirely. I am a bit surprised the each-column-is-a-block direction is actually being pursued.

Some questions I have about this approach:

  • Is it possible to implement unequal-width columns? This is essential for making the Columns block useful.
  • Is it possible to implement responsive columns? This is even more essential.
  • Is it possible to have a horizontal sibling inserter to insert additional Column blocks? Currently, the UI/UX for inserting a Column into Columns is pretty confusing, because you insert it via the sibling inserter above one of the existing columns, but when you do so, the Column is inserted horizontally. I imagine a sibling inserter rotated 90 degrees and usable between two Column blocks would be more ideal.

Notably, the Columns slider in the Columns block does nothing now. If this implementation is used, would this slider be removed and replaced with something like a horizontal sibling inserter?

Also, due to the current way the sibling inserter works, you can insert Paragraph blocks directly into the Columns block, which produces very interesting results. Obviously, this is not intentional and probably not desirable either, but I thought I would point it out because of the interesting way the widths of the blocks are determined based on the content.
image

@chrisvanpatten
Copy link
Member

chrisvanpatten commented Jun 11, 2018

The more I use it, the more concerned I am about the UX. I think the conceptual idea of a "Columns" block with "Column" children makes a sort of academic sense, but I think just introduces more confusion.

Do new columns get added via the inserter somehow? How does that inserter look, especially in more complex column/row layouts? Or if not, and the Columns block manages its own children, how is that articulated to users (there are nested blocks, but the blocks aren't really like other blocks, because you don't manage them the way other blocks are managed, etc.)? How do you coordinate the relationship from block to block? Would there need to be new APIs to help blocks manage their children and keep them in sync?

I definitely can be convinced there's a way to make this work, and I can appreciate the sort of "semantic purity" that a "Columns > Column > Your Blocks Here" solution brings, but I think it introduces more points of confusion from an end-user perspective, and creates more complexity for devs working with InnerBlocks.

The primary alternative here is to keep the distinction of "grouped" and "ungrouped" layouts (documentation), and automatically insert a wrapper element for grouped layouts. This is challenging both in the further inconsistency between the two types of layouts, and in the current unavailability of layouts anywhere outside of the edit implementation where it is passed (it would likely need to become a top-level property of the block).

Inconsistency between the two types of layouts (e.g. actually grouping grouped layouts), and layouts as a top-level block property, both seem like good ideas :)

(Should also note that although this is a Columns block issue, I'm also trying to consider it from the perspective of other non-column implementations of InnerBlocks. Columns are a convenient/understandable implementation of that API, but not the only one.)

@chrisvanpatten
Copy link
Member

Left some additional thoughts/ideas over on #6895 on a path where this could work, with a new concept: the "transparent" block.

@eddr
Copy link

eddr commented Jun 20, 2018

Also, please consider allowing for a flexbox layout instead of grid (as a selectable option)

@aduth
Copy link
Member Author

aduth commented Jun 20, 2018

The changes here include revisions for the columns block to use Flex instead of Grid.

.wp-block-columns {
display: flex;
}
.wp-block-column {
flex: 1;
}

@paulwilde
Copy link
Contributor

paulwilde commented Jun 20, 2018

This would fix #5935.

My only concern is that it's very time-consuming to build a layout if you are required to individually add a column block for each column before you can add other blocks within that column. If this is handled automatically then disregard this comment as I haven't tested this PR out.

@aduth
Copy link
Member Author

aduth commented Jun 20, 2018

Rebased and pushed a few changes:

  • The Column block was previously not restricted to its Columns parent, and therefore would be allowed to be inserted standalone outside this context. This has been resolved in b725803.
  • InnerBlocks now continuously keeps its template in sync. When the user uses the Columns range slider to change the number of columns, the block view updates accordingly.

There's still a couple awkwardnesses:

  • If you select the Column block (by pressing ArrowUp from a paragraph within it), then use the inserter, you have the option to insert a Column block. This doesn't do anything, and is not expected to, since column count is managed by the inspector control, but it may be unexpected for a user to encounter.

@robbisy
Copy link

robbisy commented Jun 21, 2018

Is there any plan to add a wrapper option for innerblock that could wrap each of this columns in a component. I've seen there's a plan to add wrapping grouped layout in innerblocks but I think add the option for wrapping each column could be useful too

@mtias
Copy link
Member

mtias commented Jun 21, 2018

Unrelated to this, but I'm seeing child blocks in the quick block inserter outside the parent.

image

When inserting an item within a column, but not focusing the column block first, the block is appended at the end of the post:

image

@mtias
Copy link
Member

mtias commented Jun 21, 2018

We should make each column block have the visual height of the container columns block.

image

@ZebulanStanphill
Copy link
Member

ZebulanStanphill commented Jun 21, 2018

@mtias

When inserting an item within a column, but not focusing the column block first, the block is appended at the end of the post

I think that issue is #5772.

@aduth aduth removed the [Status] In Progress Tracking issues with work in progress label Jun 25, 2018
@aduth aduth changed the title WIP: Blocks: Render columns as nested column (wrapper) Blocks: Render columns as nested column (wrapper) Jun 25, 2018
@aduth
Copy link
Member Author

aduth commented Jun 25, 2018

Pushed a few more updates.

  • Fixed an issue where the initial Column blocks were not added when creating a new Columns (6dfe2af)
  • Updated end-to-end tests per revised behavior (42e9201)
  • Stretch Column to occupy full height of Columns parent (2050aa0)
    • Not entirely happy with the CSS here, but struggled for a while with various other approaches. Suggestions welcome.

@ZebulanStanphill
Copy link
Member

@aduth I noticed that the Column block is not always the height of its parent, depending on what block you have selected:
image
image

Is there any reason why you cannot just use some flex property like align-items: stretch to make the Column blocks fill the height of the Columns block?

@aduth
Copy link
Member Author

aduth commented Jun 27, 2018

@SuperGeniusZeb The styling challenge is that the border is applied to .editor-block-list__block-edit, which is only one of many children within the column itself (along with toolbar, movers, sibling inserter, etc). I wasn't able to reproduce your original issue, though pushed a more Flexbox-y implementation in 003fab1 that may help your issue.

Another significant challenge I encountered is that with these changes, all existing Columns blocks would become invalid; worse, their content would disappear. This is because nested blocks were exempted from validation due to technical decisions around its unawareness of its own inner HTML (see #3745 (comment), #3745 (comment)). Our current deprecation behavior only takes effect for blocks which are strictly invalid. As a workaround, I added a new isEligible property function to a deprecation to allow a block to opt-in to a deprecated migration under specific circumstances even if the block is considered valid. With this, I was able to add a deprecated migration for old Columns content to the new format.

(cc @youknowriad)

@GlennMartin1
Copy link

I'm not a coder, but for what it's worth:

It's arguable if you should feel compelled to make columns backward-compatible, considering that it's been clearly designated as "beta."

Columns need a lot of development. Don't compromise development by attempting backward-compatibility.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code wise, this looks good. A noticed that it's possible to drag and drop a column block inside another column block. I guess this should be forbidden?

@aduth
Copy link
Member Author

aduth commented Jun 28, 2018

@youknowriad For intents and purposes of this pull request, I'm relying on <InnerBlocks allowedBlocks={ /* ... */ } /> and the block parent APIs to enforce the restriction. If there are failings there, I might be inclined to address separately, though I'll certainly look into what might be the cause of this. It may be something with drag-and-drop specifically, though I seem to also recall @westonruter mentioning similar issues with blocks being allowed to insert themselves as a child of its own type, regardless of the allowedBlocks configuration.

@jorgefilipecosta
Copy link
Member

The need for "transparent blocks" or a similar concept also appeared in the layout blocks PR #7414 (comment).

@aduth
Copy link
Member Author

aduth commented Jul 2, 2018

I toyed with it a bit in the branch, and I think the general idea could be fairly "simple" but not "easy" to implement, where BlockListBlock's rendering has a shortcut condition to render only the contents of its BlockCrashBoundary when it's intended to be a passthrough. This is made complicated by various event handlings, ref assignment (e.g. focusTabbable), and styling considerations (e.g. expectation of a root div with data-type, className, etc). If it's something worth exploring, I'd suggest we do so as a separate task.

@jasmussen
Copy link
Contributor

Great discussion here.

I think there are two aspects to it — one is ensuring that blocks that ship with core have a good user experience, not only because they ship with the software, but because they will serve as inspiration for other blocks to come. In this case, like I've noted, I think it would be nice to reduce the cognitive overhead by requiring a user to only configure the columns block itself, as well as any content inside each column.

I know this is way more difficult than it sounds, and I think it's fine to explore separately. This goal is implementation-agnostic and markup-agnostic, it's purely an ideal to consider.

But this discussion has also made clear that no matter how good a user experience we can build, there will be a need to handle situations with very deep nesting. This is a point that @SuperGeniusZeb has made a couple of times in the past, and it's increasingly clear that we need to accommodate less than ideal nesting situations. In that vein, we might want to look at #6459 again.

@aduth
Copy link
Member Author

aduth commented Jul 3, 2018

Unless there's opposition, my plan is to merge this as-is about an hour from now. I've created #7694 to expand upon the idea of a passthrough block supports, have commented at #6895 (comment) with respect to block nesting wrapper implementations, and have referenced to #6459 which I think may be worth reopening as it relates to navigating between deep nesting of blocks.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is working correctly in my tests. The fact the columns block is not transparent adds some weight to the UI, but also adds the useful feature of being able to set a class in each column.
Awesome work bringing this changes in 👍

@aduth aduth merged commit b88141a into master Jul 3, 2018
@aduth aduth deleted the try/columns-wrapper branch July 3, 2018 17:16
@mtias
Copy link
Member

mtias commented Jul 3, 2018

Thanks @aduth for driving this update!

belcherj added a commit to belcherj/gutenberg that referenced this pull request Jul 3, 2018
WordPress#7234 introduced a new column format.  This updates the documentation to match the new format.
@samikeijonen
Copy link
Contributor

Wuhuu!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks [Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P
Projects
None yet
Development

Successfully merging this pull request may close these issues.