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

Add Columns template options, support InnerBlock templateOptions #16129

Merged
merged 17 commits into from Jun 24, 2019

Conversation

@aduth
Copy link
Member

commented Jun 12, 2019

Closes #15662
Partially addresses #15663

This pull request seeks to enhance the InnerBlocks component to allow developers to specify a set of template options to allow the user to select from upon inserting the block. As demonstration, this is implemented into the Columns block, allowing the user to select from a number of predefined columns layouts.

template-options

Work In Progress:

  • "Undo" does not work as you would want it to once having selected a block template
  • Needs verification about the purpose of the InnerBlocks templateInProcess state, removed here
  • Needs verification about deprecation behavior for Columns block

Implementation Notes:

There's a question here about if or how we preserve the current default columns count (2 columns). The absence of a columns serves as a useful indicator for considering the block in a placeholder state, similar to the Image block using the absence of its url attribute to determine whether it is in a placeholder state. This has obvious complications for existing content which assumes the default of 2 columns. Included here is a deprecation which is intended to transition a block which assumes the default to one which defines it explicitly. There is some awkwardness here in the fact that if a user chooses to skip template selection (the "Skip" button), we still "default" to 2 columns.

Note below in "Design Notes" the consideration about customizing labels. I'd appreciate feedback about whether this is a reasonable enhancement for InnerBlocks, or if we should consider shifting this responsibility back to the block component to handle (optionally enhancing Placeholder instead, or creating new components to render a set of Button choices).

Design Notes:

There are a few notable differences from the concepts as presented by @kjellr at #15663 (comment). For the most part, these differences come down to consequences of consistency and in considering the technical requirements of these interfaces (e.g. labels specific to "Columns" block vs. generic labels).

  • There's not quite as much padding around the actual implementation as in the concept.
    • This is inherited from the Placeholder styling. If we want more padding, we should consider if it should be applied to all instances of Placeholder.
  • The "Skip" link is underlined
    • This is inherited from the <Button isLink /> styling. If we want no underline, we should again consider if it should apply to all link-styled buttons.
    • Aside: There is some prior discussion in #7534 about the role of links vs. buttons, relevant to whether this non-navigation interaction should appear as a link or as a button.
  • The icon, title, and instructions of the placeholder were made to be generic, unlike the columns-specific verbiage in the concepts.
    • This is a point where I can see how it might be good to have some baseline default, but allow for customization. My worry, however, is that the technical implementation of customizing the template options is already via 3 props, and this would add 3 more (or three properties of an e.g. templateSelectionPlaceholderProps object prop). At that point, it begs the question whether this ought to be a common feature of InnerBlocks vs. custom implemented in each block vs. enhancing the Placeholder component to better support a set of buttons.
  • The style of the icon buttons is not quite the same as proposed in the concept.
    • My choosing of the words "icon buttons" here is intention, since this seems like something we should consider to be a variant of the existing IconButton component, at least as sharing the same semantic purpose. The ones in the concept appear quite similar to those found in the inserter menu and in the block transforms menu, and subsequent screenshots of the concept display similar buttons in the sidebar.
    • Current styling could be considered temporary, and was the minimal effort to have the buttons appear in the general shape as in the concept. I might propose that we consider whether it makes sense to create some generic component for these types of buttons, or adapt the existing IconButton to reuse across these multitude of use-cases.
@@ -90,6 +90,69 @@ const TEMPLATE = [ [ 'core/columns', {}, [

The previous example creates an InnerBlocks area containing two columns one with an image and the other with a paragraph.

### `templateOptions`

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jun 13, 2019

Contributor

Honestly, I'm still uncertain about this prop. It seems like that something that could have easily been built in the columns block itself (not render InnerBlocks until a template is chosen).

Is the idea here to "generalize" this behavior? Is this part of the goal of the original issue?

I can see value just trying to see if we really want to introduce it now, or maybe try to see if we need it in at least two blocks to introduce something like that.

This comment has been minimized.

Copy link
@aduth

aduth Jun 13, 2019

Author Member

Honestly, I'm still uncertain about this prop. It seems like that something that could have easily been built in the columns block itself (not render InnerBlocks until a template is chosen).

Is the idea here to "generalize" this behavior? Is this part of the goal of the original issue?

From the original comment:

I'd appreciate feedback about whether this is a reasonable enhancement for InnerBlocks, or if we should consider shifting this responsibility back to the block component to handle (optionally enhancing Placeholder instead, or creating new components to render a set of Button choices).

From the related issue (and as evidenced by prior art in a number of plugins), there's a desire for a common behavior here in template selection. I worry about inconsistency if we start proposing that it be implemented ad hoc by each block.

As in the text above, I think there might be some way we can still put it on the block to implement, with sufficient component coverage (Placeholder) to at least strongly encourage conforming to a consistent UI.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jun 14, 2019

Contributor

I think I worry more about the growing number of props the InnerBlocks could and will have in the future. Maybe a first step here could be to make the TemplatePicker component, the sharable API, the same way we have MediaPlaceholder...

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Jun 24, 2019

Member

Hi @aduth, sorry, I arrived late to the party (did not checked this PR sooner). I like the idea of providing a consistent UI for this scenario that we predict may become very common. I share the concern of increasing the number of props/responsibilities of InnerBlocks with @youknowriad.
What if we only add a new prop called placeholder to InnerBlocks (it just renders a placeholder when the template is null and no child blocks exist). Then we can expose a TemplatePlaceholder component that handles all the UI? (I think this version was exactly what @youknowriad suggested). It seems InnerBlocks should not receive a prop that is called when a template is selected and it may be good to extract that behavior. Even having a placeholder prop that receives an element rendered when the placeholder should be shown is an unnecessary prop but it is useful otherwise blocks would need to check the number of child blocks they have and conditionally render the placeholder.

@kjellr

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

🎉 This is an excellent start, @aduth!

There's not quite as much padding around the actual implementation as in the concept.
This is inherited from the Placeholder styling. If we want more padding, we should consider if it should be applied to all instances of Placeholder.

Cool. We can leave as is for now. 👍

The "Skip" link is underlined
Aside: There is some prior discussion in #7534 about the role of links vs. buttons, relevant to whether this non-navigation interaction should appear as a link or as a button.

Ah yes — this should've been underlined in my comp. Good call.

Lots of good discussion in that ticket. This is essentially a "Cancel" action, which WordPress typically handles visually as a link button. Depending on the outcome of #7532, if we decide to style it differently, we should also apply a similar style to the "Cancel" link in the image replacement flow.

The icon, title, and instructions of the placeholder were made to be generic, unlike the columns-specific verbiage in the concepts.

For consistency, I do think it's important to have the Columns icon and label in there. The instructions are probably okay to be generic though.

Current styling could be considered temporary, and was the minimal effort to have the buttons appear in the general shape as in the concept. I might propose that we consider whether it makes sense to create some generic component for these types of buttons, or adapt the existing IconButton to reuse across these multitude of use-cases.

Yep, definitely. This feels sort of like a new component to me, but it may be possible to just build upon what we use for the block styles (and for the block inserter buttons). We're thinking through adding a better "active" state to these in #15906 (comment), which would also come in handy here.

@youknowriad
Copy link
Contributor

left a comment

This is great, I feel we're almost there. Let's try to get this into the next release.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2019

@aduth is AFK this week and in an attempt to land this, I'll be making some small tweaks here.


class InnerBlocks extends Component {
constructor() {
super( ...arguments );
this.state = {
templateInProcess: !! this.props.template,
};

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jun 17, 2019

Contributor

The e2e tests show a failure because of this. I think it ensures the InnerBlocks don't focus the placeholder to avoid creating any extra block while the template is being processed (or something like that).

@jorgefilipecosta might now more, I think it would be good to document.

Right now, I'm going to restore it.

@aduth aduth requested review from nerrad and ntwb as code owners Jun 17, 2019

@youknowriad youknowriad force-pushed the add/template-options branch 2 times, most recently from a1320df to 0eced9d Jun 17, 2019

@@ -124,15 +202,13 @@ export default withDispatch( ( dispatch, ownProps, registry ) => ( {
setAttributes( { columns } );

let innerBlocks = getBlocks( clientId );
if ( ! hasExplicitColumnWidths( innerBlocks ) ) {

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jun 17, 2019

Contributor

There's a bug here in master if you try to change the number of columns from 2 to 4 (more than 1). This function only accounted for a single addition/removal.

icon: <SVG width="48" height="48" viewBox="0 0 48 48" xmlns="http://www.w3.org/2000/svg"><Path fillRule="evenodd" clipRule="evenodd" d="M39 12C40.1046 12 41 12.8954 41 14V34C41 35.1046 40.1046 36 39 36H9C7.89543 36 7 35.1046 7 34V14C7 12.8954 7.89543 12 9 12H39ZM39 34V14H25V34H39ZM23 34H9V14H23V34Z" /></SVG>,
template: [
[ 'core/column' ],
[ 'core/column' ],

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jun 17, 2019

Contributor

I removed the explicit widths from the evenly distributed templates.

@youknowriad youknowriad force-pushed the add/template-options branch from 0eced9d to 1a85e20 Jun 17, 2019

@youknowriad youknowriad self-assigned this Jun 17, 2019

@mtias

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2019

Love to see this!

},
{
title: __( 'Two columns; one-third, two-thirds split' ),
icon: <SVG width="48" height="48" viewBox="0 0 48 48" xmlns="http://www.w3.org/2000/svg"><Path fillRule="evenodd" clipRule="evenodd" d="M39 12C40.1046 12 41 12.8954 41 14V34C41 35.1046 40.1046 36 39 36H9C7.89543 36 7 35.1046 7 34V14C7 12.8954 7.89543 12 9 12H39ZM39 34V14H20V34H39ZM18 34H9V14H18V34Z" /></SVG>,

This comment has been minimized.

Copy link
@mtias

mtias Jun 17, 2019

Contributor

Can we imagine a few more cases that go beyond layout columns here to ensure this can scale well? One that comes to mind is for #15623 — imagine adding the "Post" block and choosing from "title only", "title and date", "title and excerpt", "full post" shapes.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jun 18, 2019

Contributor

Makes sense, it also goes beyond the innerBlocks and could potentially be used to apply all kind of attributes.

I'm thinking we should create an issue about that describing the problem (block attributes sets) and maybe include one example in addition to the setup state of the columns block in order to come up with the best API for both use-cases. Do you have an example on mind with the current set of blocks?

Also, I can go ahead and create an issue unless you want to create it yourself to detail your thoughs?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jun 18, 2019

Contributor

Also, these are other reasons to mark the APIs used as experimental for now until we cover the whole feature.

This comment has been minimized.

Copy link
@aduth

aduth Jun 24, 2019

Author Member

Also, these are other reasons to mark the APIs used as experimental for now until we cover the whole feature.

I think this is fair. Considering from the how this scales to other templates, and @mtias comment below at #16129 (comment), it occurs to me that this might not make sense as a prop of InnerBlocks, but rather more similar to the block's registration of styles (both as a setting of the block, and with its own API registerBlockStyle). That being said, doing so leaves open the question of how to configure options like skip links, callback function. Experimental props might be the best compromise for now (maybe in the future we still support it as a prop, but automatically provide it via context using the block's settings).

@aduth

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2019

Ok, I went ahead and removed the "columns" attribute from the columns block. It fixes the "undo" entirely and I also fixed the template selection bug.

What of the backwards-compatibility impact for theme developers who may have come to expect the has-2-columns class? Couldn't we still apply it by checking the length of innerBlocks at save time?

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

Couldn't we still apply it by checking the length of innerBlocks at save time?

I thought about it, but right now the Block API doesn't allow us to do it. (we don't receive the innerBlocks in the save function). I think this is one of those changes that could be handled using a dev note. I also asked @jasmussen about it. and there doesn't seem to be real usage for these classNames, especially with the column widths being customizable.

Remove unnecessary comma
Co-Authored-By: Andrew Duthie <andrew@andrewduthie.com>
@@ -2,8 +2,8 @@
{
"clientId": "_clientId_0",
"name": "core/columns",
"isValid": true,
"attributes": {
"isValid": true,

This comment has been minimized.

Copy link
@aduth

aduth Jun 24, 2019

Author Member

Guessing these were manually edited?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jun 24, 2019

Contributor

Right, I can regenerate using the CLI

migrate( attributes, innerBlocks ) {
attributes = {
...attributes,
columns: innerBlocks.length,

This comment has been minimized.

Copy link
@aduth

aduth Jun 24, 2019

Author Member

If we've dropped the columns attribute, this should no longer apply?

@aduth

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2019

Aside my latest comment, and with future consideration of Dev Note, I think this looks good to land for an initial iteration. Thanks for taking it over in my absence. Given the discussion, I think there's lots of directions we may take with how "selection" applies (both as inner blocks and as a block's own attributes) or is reused (post layouts, etc). Implementing the props as experimental for this iteration should allow us some flexibility for how those iterations are explored.

@aduth

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2019

It's not perfect because it requires two undo to actually undo the template selection. I think this is a bigger and separate problem regarding undos that are composed of more than one atomic change to perform the user action.

While the implementation here has since changed, the general problem is pretty well-summarized/tracked at #8119 (or, at least, a solution there should provide the necessary tools to avoid these situations for synchronous block updates as well).

@youknowriad youknowriad force-pushed the add/template-options branch from 7d8ccc7 to 02c6c04 Jun 24, 2019

@youknowriad youknowriad force-pushed the add/template-options branch from 02c6c04 to 409e48c Jun 24, 2019

@jorgefilipecosta
Copy link
Member

left a comment

I left a comment regarding having an external component for template choosing functionality.
I noticed a possible style problem when I try columns block on Twenty Fourteen theme I get list marks on each template option.
Screenshot 2019-06-24 at 17 14 22
Regarding the functionality, it seems to work correctly

@youknowriad youknowriad merged commit 855be79 into master Jun 24, 2019

1 of 2 checks passed

Filter merged Filter merged
Details
Travis CI - Pull Request Build Passed
Details

@youknowriad youknowriad deleted the add/template-options branch Jun 24, 2019

@github-actions github-actions bot added this to the Gutenberg 6.0 milestone Jun 24, 2019

@@ -26,7 +26,7 @@ function gutenberg_test_templates_register_book_type() {
array( 'core/quote' ),
array(
'core/columns',
array(),
array( 'columns' => 2 ),

This comment has been minimized.

Copy link
@aduth

aduth Jun 24, 2019

Author Member

This shouldn't be here, should it?

(I don't think it's too problematic for merge, if at least the tests still pass)

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jun 24, 2019

Contributor

Right, it's probably ignored.

@klihelp

This comment has been minimized.

Copy link

commented Jun 25, 2019

Thank you!:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.