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

Enables Block Grouping/UnGrouping using core/group #14908

Merged
merged 76 commits into from
Jun 5, 2019

Conversation

getdave
Copy link
Contributor

@getdave getdave commented Apr 10, 2019

This is an experiment for one direction we might take for the ability Provides the ability to group other Blocks within the core/group Block as per suggestion here.

Closes #14601

Screenshots

Screen Capture on 2019-04-12 at 16-16-07

📺 Here is a screencast of this in action.

Todos

  • Allow transforms to/from all Block types - currently limited to manually supplied Block types in the transforms definitions. Probably start here
  • UnGroup functionality
  • Ensure transform functionality is disabled if core/group is not available
  • Ensure options menu items are hidden and disabled if core/group is not available
  • Ensure align widths of Blocks being grouped are respected and the appropriate width alignment set on the core/group so that layout is preserved when grouping.
  • Write e2e tests to cover all functionality

@getdave getdave added the [Block] Group Affects the Group Block label Apr 10, 2019
@getdave getdave self-assigned this Apr 10, 2019
@getdave getdave added the Needs Design Needs design efforts. label Apr 11, 2019
@getdave
Copy link
Contributor Author

getdave commented Apr 11, 2019

@kjellr Might I trouble you for suitable icons for

  1. Group
  2. Ungroup

@jasmussen
Copy link
Contributor

I really dig this. Impressive work for what you call "rudimentary".

My very first instinct was: this should be in the switcher. The action is the same as when you create a quote — select multiple blocks, use switcher, choose quote. Group should be there.

At the same time, I would love to see a ⌘G shortcut for grouping blocks together, and this would be more discoverable if it sits in the More menu with a shortcut tip on the right.

To echo Matías, I could support having this work in both places, just for that reason. But I would also suggest that if we can only have one, or if we need to start small, I would put this in the block switcher first.

@kjellr
Copy link
Contributor

kjellr commented Apr 11, 2019

@kjellr Might I trouble you for suitable icons for

Group
Ungroup

Some light research suggests a fairly standard representation of "grouped" and "ungrouped" elements is the common representation of this idea. Here's Keynote and Sketch:

Screen Shot 2019-04-11 at 12 18 51 PM

Screen Shot 2019-04-11 at 12 19 45 PM

Oddly enough, Material doesn't have icons like these. ¯_(ツ)_/¯

The "layers" icons seem to be the closest:

Screen Shot 2019-04-11 at 12 24 48 PM

We could also do something custom, along the lines of:

Screen Shot 2019-04-11 at 12 39 52 PM

Screen Shot 2019-04-11 at 12 42 40 PM

If you want to plug something in while we think about it, here are the svgs for the bottom option:

https://cloudup.com/cpOhjnDEqBf

@getdave getdave mentioned this pull request Apr 11, 2019
5 tasks
@getdave getdave force-pushed the try/block-grouping-into-container branch from c17e571 to 2b6f334 Compare April 12, 2019 09:19
getdave added a commit that referenced this pull request Apr 12, 2019
@getdave
Copy link
Contributor Author

getdave commented Apr 12, 2019

Update

I have updated this PR to include support for grouping via

  • Block transforms
  • "Group" button (under dropdown)
  • Cmd + Option + g keyboard shortcut

Important note: as it stands the transform on core/group only works on core/paragraph Blocks. As a result the functionality described above will only work when grouping core/paragraph. I'm not sure how to make it apply to all Block types. Advice appreciated.

The keyboard shortcut follows the convention from Google Docs

Screen Shot 2019-04-12 at 11 29 15

@kjellr I've added the Group icon as provided.

We're also missing "UnGroup" but this isn't currently possible as the to transform callback doesn't provide access to innerBlocks on the core/group.

📺 Screencast

You can watch a screencast here.

Feedback Required

I'm looking for guidance on how we could implement wildcard block transforms via transforms from and to. We need to be able to:

  • allow all/any Blocks to be transformed - currently, you have to specify particular Block types but we need a wildcard option
  • have access to the full Block data within the transforms callback in order that we can access the name of the Block and innerBlocks of the Block (both required for UnGroup)

@mtias mtias removed the Needs Design Needs design efforts. label Apr 12, 2019
@getdave getdave added [Status] In Progress Tracking issues with work in progress Needs Technical Feedback Needs testing from a developer perspective. labels Apr 12, 2019
@getdave
Copy link
Contributor Author

getdave commented Apr 12, 2019

Another update 🙈

This commit has modified the logic around Block transformations logic to enable wildcard Block transforms. The following is a high level summary:

  • Pass Block names into transform callback functions - this enables dynamic Block creation in wildcard transforms (see core/group). Previously only had access to attributes and innerBlocks (question why didn't we just pass the entire Block in here?).
  • Add edge cases to Block transformation logic to account for specifying * (all) Block types as valid for a transformation
  • Remove unwanted test that checks all Blocks are of the same type before allowing a transform on a Multi Block selection. This is probably the first transform that would ever have required different types of Blocks to be transformed together.

The last item, in particular, will need some experienced 👀 on it. I suspect we may need to restore the test but with extra logic to account for wildcard.

Also, this entire commit may be better being pulled into its own separate PR (although that could make manual testing quite tricky).

Screenshots

You can now Group multiple different Block types together via:

  • Transforms
  • Group dropdown button
  • Keyboard shortcut

📺 https://cld.wthms.co/lNNWAy

@mtias
Copy link
Member

mtias commented Apr 12, 2019

Good progress.

Remove unwanted test that checks all Blocks are of the same type before allowing a transform on a Multi Block selection. This is probably the first transform that would ever have required different types of Blocks to be transformed together.

Yes, this is something that we would like to do. This is the first transform that is applicable regardless of the block types involved.

@getdave
Copy link
Contributor Author

getdave commented Jun 5, 2019

@aduth Thanks for the approving review! 🎉

Since there seems to be a number of these follow-on tasks, can we make sure to summarize them somewhere, ideally in individual issues?

I've created follow-on tasks as issues and noted these in your comments.

I go back and forth on whether I like the name convert. If there's opposition raised between now and Monday's RC, we can always revise it without too much trouble.

I will add this to the Core Editor meeting notes. I may not be able to attend due to family commitments. In which case it would be great if you could bring this up.

Accidentally got lost during renaming of “UnGroup” to “Ungroup”!
@getdave getdave merged commit a488920 into master Jun 5, 2019
@aduth aduth deleted the try/block-grouping-into-container branch June 5, 2019 12:35
getdave added a commit that referenced this pull request Jun 5, 2019
Enables the ability register a given block as the block which handles “grouping” interactions within the editor.

Related #14908
@jwold
Copy link

jwold commented Jun 6, 2019

Can't wait to see this live. So excited about it! I've started working with groups in the past few days, and something like this will be amazing.

@youknowriad youknowriad removed the [Status] In Progress Tracking issues with work in progress label Jun 7, 2019
@youknowriad youknowriad added this to the Gutenberg 5.9 milestone Jun 7, 2019
@ktmn
Copy link

ktmn commented Jun 13, 2019

This is a cool feature, thank you.

Some thoughts: Perhaps core/group shouldn't necessarily be singled out. I have many blocks that act as a group/container and sometimes I would like to wrap already existing block(s) with them as an afterthought. Current workflow is to create the container and then drag and drop which is fiddly or switch to text mode and wrap it manually.

I like the option to group under "More options" button on the block toolbar, but Transform to -> Group seems odd because intuitively transform should change the current block to something else.

Ideally, instead of More options -> Group there could be More options -> Wrap that then allows to choose from any blocks whose InnerBlocks allow all currently selected blocks, core/group included.

Less ideally, I can register transforms for the core/group to manually turn them into any other container block after grouping, so theres that :)

Edit: Also More options -> Unwrap would be something that can't be easily done right now.

getdave added a commit that referenced this pull request Jun 19, 2019
…uping" interactions (#15774)

* Adds grouping interaction to state

Enables the ability register a given block as the block which handles “grouping” interactions within the editor.

Related #14908

* Registers grouping block

* Auto updated README

* Adds test for get/set Grouping block

* Update documentation with clearer descriptions

* Adds reducer and selector tests

* Updates “Blog” for correct “Block” in docs
@getdave
Copy link
Contributor Author

getdave commented Jun 25, 2019

This is a cool feature, thank you.

@ktmn You are welcome. Glad you like it. 😄

Perhaps core/group shouldn't necessarily be singled out.

Agreed, which is why I've added the ability to register your own Block as the Block to be used for "Grouping" #15774

This doesn't completely cover the use-case you outline. However, it will (once this PR is completed #16278) allow you to define (in code) which Block is actually used for the act of "Grouping". In theory, you'll then be able to register your own Block with InnerBlocks support and a supporting transform and everything will work as expected. At least that's the theory.

Also, I'd advise that if you have a feature request you raise an Issue in this repo. This will ensure it is given visibility, as it was only by chance that I came across your comment on this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Group Affects the Group Block
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create group block by 'group' interaction