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 transform for group block to cover block. #30890

Merged

Conversation

getsource
Copy link
Member

@getsource getsource commented Apr 16, 2021

Description

  • Copies inner blocks.
  • Uses existing background color or gradient if it's available. Otherwise, defaults to a white background to avoid prompt for image/color.

Initial review props @Mamaduka
Closes: #30445

(edit: removed background default color due to feedback)

How has this been tested?

Tested manually in wp-env, and with existing automated tests with npm run test.

Screenshots

Screen Shot 2021-04-16 at 15 53 22

Types of changes

Enhancement.
Adds ability to transform a group block into a cover block, maintaining existing background color or gradient.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@getsource getsource requested a review from ajitbohra as a code owner April 16, 2021 07:01
@getsource getsource added [Block] Cover Affects the Cover Block - used to display content laid over a background image [Block] Group Affects the Group Block (and row, stack and grid variants) [Type] Enhancement A suggestion for improvement. labels Apr 16, 2021
@getsource
Copy link
Member Author

getsource commented Apr 16, 2021

Thought it was worth noting that I'm not 100% sure about the flow here.
I think it's very useful to allow folks to move from a group to a cover, to allow customizing more.

I decided to have a default background color because otherwise, both the block preview before the transform, and final appearance after transform makes it look like it has removed the group's contents.

Example screenshot, if background color default were removed from the PR, and there is no background or gradient set:
(this is not how it works in the current PR, and only an explanation of the reasoning)
Screen Shot 2021-04-16 at 16 13 18

For me, ideally, the placeholder / prompt for the additional data in this case would make it clear that the contents have been migrated -- while still requesting that input. But, not sure if that's something for this issue or not.

Feedback welcome!

Copy link
Contributor

@ntsekouras ntsekouras left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @getsource, really appreciate it!

I personally believe though that this change will produce more confusion than being an enhancement, and involves opinionated style changes as you already expressed your concerns.

I'll comment on the issue as well, but this functionality doesn't seem to work well. Maybe by having extra checks for having an Image block in group's InnerBlocks and use that as the Cover's background 🤔 ? Even that would need more changes since the isMatch from transforms API would need to be extended to provide the innerBlocks as well and not only the attributes.

Reference: https://developer.wordpress.org/block-editor/reference-guides/block-api/block-transforms/#block

@getsource getsource force-pushed the add/group-transform-to-cover/30445 branch from 29039e8 to ed9cef1 Compare April 22, 2021 14:27
@getsource
Copy link
Member Author

@ntsekouras Updated this to only offer transformation when an existing background color or gradient is available.

Copy link
Contributor

@ntsekouras ntsekouras left a comment

Choose a reason for hiding this comment

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

Thanks for following this @getsource - nice work 💯

I've left a comment and then is ready to land.

blocks: [ 'core/group' ],
isMatch: ( { backgroundColor, gradient, style } ) => {
/*
* Make sure the group has a background, since if the cover block is
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a rewording here would be good - something like:

Make this transformation available only if the Group has background or gradient set, because otherwise Cover block displays a Placeholder. This helps us avoid arbitrary decisions about the Cover block's background and user confusion about the existence of previous content.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed in the most recent commit.

packages/block-library/src/cover/transforms.js Outdated Show resolved Hide resolved
@getsource getsource force-pushed the add/group-transform-to-cover/30445 branch from ed9cef1 to c62159a Compare May 11, 2021 09:47
Copy link
Contributor

@ntsekouras ntsekouras left a comment

Choose a reason for hiding this comment

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

Sorry for all the confusion here @getsource , but this is the final comment and this will be ready to land 😄

packages/block-library/src/cover/transforms.js Outdated Show resolved Hide resolved
getsource added 3 commits May 13, 2021 13:39
- Copies inner blocks.
- Uses existing background color or gradient if it's available. Otherwise, defaults to a white background to avoid prompt for image/color.
@getsource getsource force-pushed the add/group-transform-to-cover/30445 branch from c62159a to 92125d7 Compare May 13, 2021 05:03
Copy link
Contributor

@ntsekouras ntsekouras left a comment

Choose a reason for hiding this comment

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

Great work! 💯

@ntsekouras ntsekouras merged commit 0c9814e into WordPress:trunk May 13, 2021
@ntsekouras ntsekouras added the [Feature] Block Transforms Block transforms from one block to another label May 13, 2021
@github-actions github-actions bot added this to the Gutenberg 10.7 milestone May 13, 2021
@getsource getsource deleted the add/group-transform-to-cover/30445 branch May 13, 2021 06:54
@getsource
Copy link
Member Author

Thanks so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Cover Affects the Cover Block - used to display content laid over a background image [Block] Group Affects the Group Block (and row, stack and grid variants) [Feature] Block Transforms Block transforms from one block to another [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow Group blocks to transform into Cover blocks
2 participants