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

Framework: Deprecate grouped inner blocks layouts #7841

Merged
merged 1 commit into from
Jul 20, 2018

Conversation

aduth
Copy link
Member

@aduth aduth commented Jul 9, 2018

Related: #7234

This pull request seeks to deprecate the grouped layouts option for the InnerBlocks components. It does not eliminate layouts altogether, but keeps only the concept of an ungrouped layout. The Columns / Column conversion should be used as reference for representing what would have been achieved with grouped layouts, now expected through intermediary nested block types. This deprecation should allow for simpler implementations of block layouts, and a simpler rendering of the BlockList component.

Testing instructions:

Nothing has been removed as of yet, but a warning should be logged if attempting to assign an array value as the layouts prop for an InnerBlocks component.

@aduth aduth added Framework Issues related to broader framework topics, especially as it relates to javascript [Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P labels Jul 9, 2018
@chrisvanpatten
Copy link
Member

chrisvanpatten commented Jul 9, 2018

I think this is a great step, and reiterate my notes in #6895, about removing all layouts: #6895 (comment)

And also, if this is merged, it closes #6895.

@gziolo gziolo added this to In Progress in API freeze via automation Jul 10, 2018
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.

I checked that when grouped layouts are passed we see a deprecation message when ungrouped layouts are passed we don't see a message.
When grouped layouts are passed we continue to have multiple layout areas.

When testing ungrouped layouts with the sample we have I don't see any class added.
Test block I used:
https://gist.github.com/jorgefilipecosta/adccbb67d5bcfb3308d06304b8940648
In fact, I don't see any difference when passing an ungrouped layout and not passing anything. Did we regressed or should we also deprecate ungrouped layouts? I think I may be missing something in the ungrouped layout concept.

@aduth
Copy link
Member Author

aduth commented Jul 11, 2018

@jorgefilipecosta Hmm, so the issue is that we don't have UI to control which layout is assigned to a particular block. It was intended to be implemented and thus documented / abstracted as such. I'm becoming increasingly motivated to think we should just remove the layouts concept altogether for now, and revisit it in the future with parts of the current implementation serving as precedent.

@jorgefilipecosta
Copy link
Member

Yes, I think the best option we have now is to totally remove layouts. Having the UI to manage ungrouped layouts does not seem like a priority now and I'm not seeing a concrete use case for it that we can not achieve in other ways.

@chrisvanpatten
Copy link
Member

chrisvanpatten commented Jul 16, 2018

I want to bring up some additional concerns now that I've spent some time trying to adapt to a post-InnerBlocks-layouts world.

I posted a thread in #core-editor here: https://wordpress.slack.com/messages/C02QB2JS7/convo/C02QB2JS7-1531771401.000248/

But the main gist is that with layouts going away, there's no easy way to target specific nested blocks and provide different styles to them.

A columns block is the easiest to parse analogue, so I'll go with that.

In the old implementation, you'd have a single Columns block, and InnerBlocks layout for each column, and each layout area would get a CSS class based on the layout name. So you could theoretically use names like "column-50" or "column-25" or whatever to assign widths to that layout area.

Now, without layouts, you would create a Columns block that accepts nested Column blocks. That's all well and good, but the markup makes it effectively impossible to treat each nested block area differently.

For instance, in an example like the one below, it's not possible to target each nested .editor-block-list__block separately and assign it a different width based on the block's classes:

.editor-block-list__layout
  > .editor-block-list__block
    > .editor-block-list__block-edit
      > .my-cols
        > .editor-inner-blocks
          > .editor-block-list__layout
             > .editor-block-list__block
               > .editor-block-list__block-edit
                 > .my-col-3
             > .editor-block-list__block
               > .editor-block-list__block-edit
                 > .my-col-6
             > .editor-block-list__block
               > .editor-block-list__block-edit
                 > .my-col-3

If CSS ever supports parent selectors this would be a non-issue, but that's unlikely to happen.

Is it possible to look at a way to propagate the block's classNames up the chain so the nested sibling .editor-block-list__blocks have those classes and can be targeted in styling?

And for reference, the current core block only offers equal-width columns, so it's not subject to this issue. If you are treating each child block the same way, you can just target .editor-inner-blocks .editor-block-list__block and be done with it. Unfortunately, for many use-cases, that won't be enough.

EDIT: To be clear, this is only an issue in the editor context. On the front-end, this solution is awesome and super great.

@aduth aduth changed the title Framework: Deprecate grouped inner blocks layouts Framework: Deprecate inner blocks layouts Jul 17, 2018
@aduth
Copy link
Member Author

aduth commented Jul 17, 2018

@chrisvanpatten This seems like something that would be easily solved with an attribute on the intermediary block. It's something I even considered for the Column block in #7234, where the Column has a width attribute which assigns a class to the generated markup based on its value; no layout classes needed.

@aduth
Copy link
Member Author

aduth commented Jul 17, 2018

I have no idea what happened with the commit history here. Will try to sort it out...

@aduth aduth added this to the 3.3 milestone Jul 17, 2018
@chrisvanpatten
Copy link
Member

@aduth If an attribute is added, does that attribute become available on the .editor-block-list__block element in the editor? Without an attribute or class in the markup in the editor, it's still not styleable.

@aduth
Copy link
Member Author

aduth commented Jul 17, 2018

There is the block's getEditWrapperProps, which is used by blocks such as Image, originally to assign itself into the "full" and "wide" width regions; coincidentally (or not), this was the other main reason for wanting something like layouts.

Let me think on this a bit more. It does seem like we could have a need for it, but the UI is completely absent so it's a useless feature as-is. I would like to see the Image widths refactored to take advantage of it.

How can we proceed ahead of the next version release? Should we just stick with the grouped layouts deprecation, assuming that one is relatively safe to remove now?

@chrisvanpatten
Copy link
Member

chrisvanpatten commented Jul 17, 2018

@aduth For now we'll have to continue using grouped layouts in prod until this is resolved, but our edge case is no reason to not deprecate, and we can lock to an older version of Gutenberg until there's a path forward.

EDIT: Seems like maybe deprecating ungrouped layouts would be a safer deprecation? Not sure they're used anywhere at this point.

@aduth
Copy link
Member Author

aduth commented Jul 17, 2018

@chrisvanpatten I still don't see that this would require the grouped layout. You could use the intermediary block and either getEditWrapperProps or the ungrouped layout to assign a class for the Column block which surfaces up to the wrapper in the editor.

@chrisvanpatten
Copy link
Member

@aduth Interesting… I think I finally understand what you're saying (it's been a long month). Going to give it a quick spin.

@chrisvanpatten
Copy link
Member

chrisvanpatten commented Jul 17, 2018

@aduth yep, that works!

It's not 100% perfect — a wrapperClassName const is generated with various state classes and such, but I don't see an easy way to grab it so you can append your custom className instead of overwriting it — but I can hack around that for now!

(Hack around = I'll probably just add a data-column-class attribute and cheat, but I could still see this being useful to plugins, to add their own state classes to the block edit wrapper.)

@aduth
Copy link
Member Author

aduth commented Jul 18, 2018

I'm going to rebase this to drop cce8a0a, which will effectively deprecate grouped layouts (the original intent of the pull request), while leaving ungrouped layouts as an open question. I'll probably update the documentation to at least avoid misleading developers into thinking that anything automated will happen by using layouts. In fact, I might omit layouts from the documentation altogether until we can sort through the implementation.

@aduth aduth force-pushed the deprecate/grouped-layout branch 2 times, most recently from f7ea91e to 629074e Compare July 19, 2018 15:32
@aduth
Copy link
Member Author

aduth commented Jul 19, 2018

I still have some doubts in the ungrouped layouts, and am even considering whether we could leverage Slot/Fill behaviors to enable a block implementer to apply additional attributes on its block wrapper (equivalent of getEditWrapperProps):

edit() {
	return (
		<BlockEditWrapper className="extra-class-name">
			{ /* ... */ }
		</BlockEditWrapper>
	);
}

That said, we're pretty well certain that grouped layouts will not remain, and the pull request has been rebased to reflect this. I'd still like this deprecation for 3.3, as its removal will allow for some dramatic simplification even if ungrouped layouts do stick around.

@chrisvanpatten
Copy link
Member

@aduth I love that — feels much cleaner than getEditWrapperProps.

@tofumatt tofumatt self-requested a review July 19, 2018 18:14
Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

This makes senes to me and I think the docs are good. I think we should do this; I made a minor tweak to the docs as I understood them for InnerBlocks.

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.

I'm correctly seeing the deprecation notices for grouped layouts, but not seeing them for ungrouped layouts 👍 Functionality looks the same as before.

I would prefer if we totally remove layouts but this is a good step!

@aduth aduth merged commit 6cee18e into master Jul 20, 2018
API freeze automation moved this from In Progress to Done Jul 20, 2018
@aduth aduth deleted the deprecate/grouped-layout branch July 20, 2018 13:30
@aduth aduth changed the title Framework: Deprecate inner blocks layouts Framework: Deprecate grouped inner blocks layouts Jul 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P Framework Issues related to broader framework topics, especially as it relates to javascript
Projects
No open projects
API freeze
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants