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

Nested blocks: Show the side inserter on empty paragraphs #5139

Merged
merged 1 commit into from
Feb 26, 2018

Conversation

youknowriad
Copy link
Contributor

When checking if a block is empty or not, we should ignore the "layout" attribute as it's not really a block attribute but more something related to the position of the block inside the parent block. (which makes me think it should be best stored separately cc @aduth )

While this PR fix the issue where we needed at least one paragraph block inside the columns block to be able to insert another type of block type, it surfaces the styling issues in the columns block where there's not enough room for the side-inserter.

A workaround could be to hide this inserter in the columns block, you'd still be able to use the global inserter to replace the first block instead of appending a new block.

Testing instructions

  • Add a columns block
  • Select the first block in a column
  • Use the inserter
  • It should replace the block and not insert a new one.

closes #5134

@youknowriad youknowriad self-assigned this Feb 19, 2018
@youknowriad youknowriad added the [Type] Bug An existing feature does not function as intended label Feb 19, 2018
@jasmussen
Copy link
Contributor

jasmussen commented Feb 19, 2018

This just makes the empty-paragraph in-canvas inserter show up inside columns also, right? I think that's a superb solution.

By the way I'm working, right now, on a refactor of both floats and block boundaries. This should improve the nested UI also, allowing us to use negative margins. It's difficult, but it's overdue.

Forgot to mention, 👍 👍 for showing this in-canvas inserter on ANY empty paragraph, not just those that are non-nested.

@aduth
Copy link
Member

aduth commented Feb 19, 2018

which makes me think it should be best stored separately cc @aduth

Have anything in mind? Earlier implementations of the columns block took the approach of having the parent manage its child locations, but in changing to target CSS Grid, inherited behavior of element assigning itself into particular layout (corresponding to grid-column, grid-row assignments).

We still could do something along these lines, except managed by the parent component, where the parent tracks which child indices are assigned into which layout:

https://codepen.io/aduth/pen/ZamBWO

In this example, the block markup might look more like:

<!-- wp:columns {"layouts":{"column-1":[0,1,2],"column-2":[3,4,5,6,7]}} -->
<style>
.wp-block-columns.is-block-lkj1anzl > :nth-child( 1 ),
.wp-block-columns.is-block-lkj1anzl > :nth-child( 2 ),
.wp-block-columns.is-block-lkj1anzl > :nth-child( 3 ) {
  grid-column: 1;
}

.wp-block-columns.is-block-lkj1anzl > :nth-child( 4 ),
.wp-block-columns.is-block-lkj1anzl > :nth-child( 5 ),
.wp-block-columns.is-block-lkj1anzl > :nth-child( 6 ),
.wp-block-columns.is-block-lkj1anzl > :nth-child( 7 ),
.wp-block-columns.is-block-lkj1anzl > :nth-child( 8 ), {
  grid-column: 2;
}
</style>
<div class="wp-block-columns is-block-lkj1anzl">
  <span>one</span>
  <span>two</span>
  <span>three</span>
  <span>four</span>
  <span>five</span>
  <span>six</span>
  <span>seven</span>
  <span>eight</span>
</div>
<!-- /wp:columns -->

This would require some form of inline styles support as explored in #3669, #2862

aduth
aduth previously requested changes Feb 19, 2018
@@ -25,10 +25,11 @@ export function isUnmodifiedDefaultBlock( block ) {
}

const newDefaultBlock = createBlock( defaultBlockName );
const attributeKeys = [
// Omit the layout attribute because it's a special attribute used for nested blocks.
Copy link
Member

Choose a reason for hiding this comment

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

Could we generalize this with a filter so we can keep the logic for layout consolidated in blocks/hooks/layout.js?

@youknowriad youknowriad force-pushed the fix/side-inserter-columns-block branch from 6ee1082 to abd2509 Compare February 20, 2018 08:30
@youknowriad youknowriad dismissed aduth’s stale review February 20, 2018 08:31

Updated to use a filter instead :)

@youknowriad
Copy link
Contributor Author

We still could do something along these lines, except managed by the parent component, where the parent tracks which child indices are assigned into which layout:

Yes, that's what I was thinking, I don't know if it's totally legit though :)

@@ -25,10 +30,11 @@ export function isUnmodifiedDefaultBlock( block ) {
}

const newDefaultBlock = createBlock( defaultBlockName );
const attributeKeys = [

const attributeKeys = applyFilters( 'blocks.unmodifiedBlockAttributes', [
Copy link
Member

Choose a reason for hiding this comment

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

Need some naming convention for these. I've been doing roughly [module].[function].[modifier]. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this convention, updating

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Impossible to add a block to Columns without also adding a Paragraph block, using the Inserter menu
3 participants