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

Rename section block to group #14920

Merged
merged 3 commits into from
Apr 11, 2019
Merged

Rename section block to group #14920

merged 3 commits into from
Apr 11, 2019

Conversation

talldan
Copy link
Contributor

@talldan talldan commented Apr 11, 2019

Description

Closes #14898

Renames the Section block to Group.

How has this been tested?

  • Manual testing
  • Updated E2E tests

See also #13964 (comment) for further details on testing (but replace the word section with group in your mind).

Screenshots

Screen Shot 2019-04-11 at 4 23 54 pm

Types of changes

Breaking change (but one that should be ok if it's merged before the next release)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@talldan talldan added [Type] Task Issues or PRs that have been broken down into an individual action to take [Block] Group Affects the Group Block labels Apr 11, 2019
@talldan talldan added this to the 5.5 (Gutenberg) milestone Apr 11, 2019
@talldan talldan self-assigned this Apr 11, 2019
@talldan talldan requested review from kjellr and getdave April 11, 2019 08:32
@talldan
Copy link
Contributor Author

talldan commented Apr 11, 2019

@kjellr - I think you might have been working on some patches for themes so that they work with what was the section block. I suppose this change affects that and the classes will have to be renamed?

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

It looks like a lot of work to rename a single block 😅

@@ -25,7 +25,7 @@
@import "./quote/editor.scss";
@import "./rss/editor.scss";
@import "./search/editor.scss";
@import "./section/editor.scss";
@import "./group/editor.scss";
Copy link
Member

Choose a reason for hiding this comment

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

Can it be moved to be sorted alphabetically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 done

@@ -45,7 +45,7 @@ import * as pullquote from './pullquote';
import * as reusableBlock from './block';
import * as rss from './rss';
import * as search from './search';
import * as section from './section';
import * as group from './group';
Copy link
Member

Choose a reason for hiding this comment

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

Can it be moved to be sorted alphabetically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 done

@@ -303,8 +303,8 @@ export const EXPECTED_TRANSFORMS = {
originalBlock: 'Search',
availableTransforms: [],
},
core__section: {
originalBlock: 'Section',
core__group: {
Copy link
Member

Choose a reason for hiding this comment

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

Can it be moved to be sorted alphabetically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 done

@gziolo gziolo added [Priority] High Used to indicate top priority items that need quick attention [Release] Do Not Punt Used to indicate the issue or pull request must not be moved from the assigned milestone labels Apr 11, 2019
@kjellr
Copy link
Contributor

kjellr commented Apr 11, 2019

@kjellr - I think you might have been working on some patches for themes so that they work with what was the section block. I suppose this change affects that and the classes will have to be renamed?

Yep, that's right. None of the patches have been merged in yet, so this is a great time to make the change. 🙂

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

The block itself can be inserted into post properly 👍

@chrisvanpatten
Copy link
Contributor

I love this. When I had taken my stab at the container block, this was the approach I was trying to take, along with the "Group" and "Ungroup" functionality. I think this makes so much sense, makes it more intuitive for users, and really opens the door for a better user experience!

@gziolo
Copy link
Member

gziolo commented Apr 11, 2019

What it's confusing at the moment is that when you insert Group block, what you actually see is Paragraph block:

Screen Shot 2019-04-11 at 17 17 37

Screen Shot 2019-04-11 at 17 18 21

Can we at least do something like:

Screen Shot 2019-04-11 at 17 21 44

or what @mtias suggested in our private chat with reusing the bits of Block Navigation feature:

Screen Shot 2019-04-11 at 17 27 51

Those aren't final design proposals by any means 😃

@aduth
Copy link
Member

aduth commented Apr 11, 2019

What it's confusing at the moment is that when you insert Group block, what you actually see is Paragraph block:

To be clear, should we consider this a separate task from the rename itself happening here?

I expect the work from #14241 should help to improve the experience of the default block appender.

@getdave
Copy link
Contributor

getdave commented Apr 11, 2019

I expect the work from #14241 should help to improve the experience of the default block appender.

Indeed my work on core/section was exactly what prompted me to start work on the placeholder/appender PR. I very much recommend we utilise this in both core/group and core/column once I can get that branch merged.

@getdave
Copy link
Contributor

getdave commented Apr 11, 2019

I love this. When I had taken my stab at the container block, this was the approach I was trying to take, along with the "Group" and "Ungroup" functionality. I think this makes so much sense, makes it more intuitive for users, and really opens the door for a better user experience!

@chrisvanpatten Here's my first pass at "Group" #14908

@aduth aduth merged commit 0b93ad4 into master Apr 11, 2019
@aduth aduth deleted the update/rename-section-to-group branch April 11, 2019 17:39
@gziolo
Copy link
Member

gziolo commented Apr 11, 2019

I was sharing my experience from testing. I didn’t mean it should block this PR 👍

@aduth
Copy link
Member

aduth commented Apr 11, 2019

I was sharing my experience from testing. I didn’t mean it should block this PR 👍

Do you know if there's a related issue for tracking these improvements? Is #9628 relevant?

@gziolo
Copy link
Member

gziolo commented Apr 12, 2019

Do you know if there's a related issue for tracking these improvements? Is #9628 relevant?

I'm not 100 sure but I commented regardless :) I also cross-posted to #14935 to keep the discussion going.

@getdave
Copy link
Contributor

getdave commented Apr 12, 2019

Great to see this rename get done so quickly @talldan 👍

mchowning pushed a commit to mchowning/gutenberg that referenced this pull request Apr 15, 2019
* Rename section block to group

* Update transforms test for rename of section block to group

* Alphabeticalize
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 [Priority] High Used to indicate top priority items that need quick attention [Release] Do Not Punt Used to indicate the issue or pull request must not be moved from the assigned milestone [Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Renaming the new "Section" block to "Group"
6 participants