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

Refactor cover background controls #18718

Closed
wants to merge 6 commits into from

Conversation

noahtallen
Copy link
Member

Disclaimer: since this is one of my first bigger PRs for Gutenberg, I approached this as a learning experience and hope that the changes are useful. Folks may want to take a different approach with this, which I absolutely understand :)

Description

@paaljoachim asked me to take a look at #14744, and as I was looking around at related issues to understand the context, I came across #16479. In order to add some better background tools to the Group block, it seems like the first step would be refactoring the background controls out of the cover block into a place where they could be used more generically.

The goal of this PR is to refactor the background controls out of the cover block into a new __experimentalWithBackgroundControls component located at block-editor/background-controls. I separated many of the intertwined aspects of the Cover block's edit component into separate pieces so that it will be easier to hide parts which might not be needed for other types of blocks like the group block.

I think this is a good start to #16479 since it gives us a space to start thinking about how we should compose the background controls so that they can be easily used by other blocks. I expect a lot of this will change as we go forward, particularly with the design, but I hope this could let us abstract the changes behind this component so that other things which use it (for now, just the Cover block) will benefit from future changes.

Note that I did my best to avoid functionality changes. Most of the components and functions have been copy/pasted from the cover block's edit file and then wired together.

I took some inspiration from the new useColors hook by @epiqueras, but did not quite take the final steps of converting everything into a hook. For now, it remains a functional component :)

Some things that still need consideration:

  1. How do we handle the save component? Currently, the Cover block's save component still works great, but we need to refactor those pieces out into this package so that it's easy to reuse them.
  2. How do we handle styles? What styles should belong in the block, and what styles should belong here? Currently, I left all the styles in the cover block, which means the class names used here still reference the cover block styles.
  3. I imagine tests could break with this, so I'll have to fix those :)

How has this been tested?

On my local gutenberg/docker environment, I confirmed that the cover block works exactly as it did before. The UI and controls are in the same places, and changes like gradient cover, images, focal point, etc. still work as expected.

Types of changes

Refactor, non-breaking change.

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.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .

@noahtallen noahtallen added [Type] Enhancement A suggestion for improvement. [Feature] Blocks Overall functionality of blocks Needs Technical Feedback Needs testing from a developer perspective. [Block] Cover Affects the Cover Block - used to display content laid over a background image [Package] Block editor /packages/block-editor labels Nov 24, 2019
@noahtallen noahtallen self-assigned this Nov 24, 2019
@paaljoachim
Copy link
Contributor

paaljoachim commented Mar 25, 2020

Hi Noah @noahtallen

How should we move this PR forward?
Right now it seems like other issues surrounding it are also on a stand still waiting for someone to pick them up again. It seems like a couple of issues need to be tackled before this PR can go forward (I am open for being wrong). I brought up the PR during todays Core Editor meeting. Hopefully a few will drop by and share feedback.

@noahtallen
Copy link
Member Author

Thanks @paaljoachim. I fully expect my approach in this PR to be wrong, as it was my first try at a refactor in Gutenberg. I wasn't sure how to tackle just adding a background image to the block, but reusable background controls in general seems like a great way to accomplish it.

I bet a lot of stuff has changed in the past 4 months as well, so we might need to abandon this PR. At any rate, if folks have feedback about where to start when it comes to background controls, I'm happy to take a look.

@noahtallen
Copy link
Member Author

This is far enough out of date that I don't think it is useful any more.

@noahtallen noahtallen closed this May 19, 2020
@paaljoachim
Copy link
Contributor

paaljoachim commented May 19, 2020

Understandable.
It was probably a good learning process!

@noahtallen
Copy link
Member Author

it definitely was at the time! I'm sure there is a lot that have been done better. I hope/imagine some other things related to it might be improved now as well :)

@paaljoachim
Copy link
Contributor

It is slowly getting there...:)

@youknowriad youknowriad deleted the refactor/cover-background-controls branch May 20, 2020 09:06
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 [Feature] Blocks Overall functionality of blocks Needs Technical Feedback Needs testing from a developer perspective. [Package] Block editor /packages/block-editor [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants