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

Conversation

Projects
None yet
@aduth
Member

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.

@chrisvanpatten

This comment has been minimized.

Contributor

chrisvanpatten commented Jun 8, 2018

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

This comment has been minimized.

Contributor

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

This comment has been minimized.

Contributor

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

This comment has been minimized.

Contributor

chrisvanpatten commented Jun 15, 2018

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

@eddr

This comment has been minimized.

eddr commented Jun 20, 2018

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

@aduth

This comment has been minimized.

Member

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

This comment has been minimized.

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

This comment has been minimized.

Member

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

This comment has been minimized.

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

This comment has been minimized.

Contributor

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

This comment has been minimized.

Contributor

mtias commented Jun 21, 2018

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

image

@ZebulanStanphill

This comment has been minimized.

Contributor

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 changed the title from WIP: Blocks: Render columns as nested column (wrapper) to Blocks: Render columns as nested column (wrapper) Jun 25, 2018

@aduth

This comment has been minimized.

Member

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

This comment has been minimized.

Contributor

ZebulanStanphill commented Jun 26, 2018

@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

This comment has been minimized.

Member

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

This comment has been minimized.

GlennMartin1 commented Jun 27, 2018

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.

@youknowriad

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

This comment has been minimized.

Member

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.

@westonruter

This comment has been minimized.

Member

westonruter commented Jun 28, 2018

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.

That's right. I tested a configuration where I had nested blocks with parent restrictions like A > B > C. At the A level I should only have been able to inserted B blocks, but I recall being able to insert A blocks as well. Come to think of it, maybe it's because A doesn't have a parent bottom-up restriction and I should be using allowedBlocks top-down restriction instead? Or maybe that is only for the inserter and allowed as a block-level configuration.

The PR I was testing with: https://github.com/Automattic/amp-wp/pull/1215/files

@mtias

This comment has been minimized.

Contributor

mtias commented Jun 29, 2018

@aduth

This comment has been minimized.

Member

aduth commented Jul 2, 2018

You both covered it well @jasmussen and @chrisvanpatten . In its current form, I don't think expanding the column to the full height adds much value, because there are no settings for a column. And the obvious one (width) may be managed by the Columns wrapper. I could imagine column-specific settings at some point (e.g. background color?), though in its current form I don't think it should expand. Also worth noting this is a block-specific styling that doesn't apply generally to InnerBlocks, and can be tweaked for the Columns block specifically in the future.

I do think it's a general design issue we'll need to work through though where there are multiple levels of nesting, and each "level" may have its own settings and need to be reasonably selectable.

For now I'm inclined to drop the styling to expand the column to full height of its Columns wrapper. Does that sound reasonable?

@aduth

This comment has been minimized.

Member

aduth commented Jul 2, 2018

This behavior isn't very useful to a user. Given that a user can't do anything on a "column" block, it feels like we should hide this block to the user completely. To put it differently, it feels like the behavior we have in master is the desirable one here. Can we combine the best of both worlds?

Re-reading this, I get the sense that we'd prefer for it to not be present at all. I'm not entirely sure how we do this in such a way that is generic for all use of InnerBlocks, where as noted above sometimes the intermediary blocks are useful. This seems more in line with what @chrisvanpatten is suggesting with the transparent block in his comment at #6895 (comment) .

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Jul 2, 2018

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

@aduth

This comment has been minimized.

Member

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

This comment has been minimized.

Contributor

jasmussen commented Jul 3, 2018

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

This comment has been minimized.

Member

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.

@jorgefilipecosta

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

2 checks passed

codecov/project Absolute coverage decreased by -0.06% but relative coverage increased by +0.92% compared to 47d6537
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@aduth aduth deleted the try/columns-wrapper branch Jul 3, 2018

@mtias

This comment has been minimized.

Contributor

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

Update nested templates to new columns format
WordPress#7234 introduced a new column format.  This updates the documentation to match the new format.

@belcherj belcherj referenced this pull request Jul 3, 2018

Merged

Update nested templates to new columns format #7700

4 of 4 tasks complete
@samikeijonen

This comment has been minimized.

Contributor

samikeijonen commented Jul 4, 2018

Wuhuu!!

aduth added a commit that referenced this pull request Jul 4, 2018

Update nested templates to new columns format (#7700)
#7234 introduced a new column format.  This updates the documentation to match the new format.

@mtias mtias added this to the 3.2 milestone Jul 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment