Cover block: Refactor controls #40832
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
glendaviesnz
added
[Status] In Progress
|
Size Change: +338 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
|
@youknowriad, @jorgefilipecosta, @aristath, @gziolo, @andrewserong, @ramonjd - just pinging a few people that have worked on the Cover block in past to see what the general thoughts are on splitting this now reasonably complex block's edit method up to try and make future refactoring/enhancement/testing easier. This PR splits the block and inspector controls out to separate files, and while this does reduce the size and complexity of the We found this approach of splitting out the controls useful in the Jetpack blocks for adding better test coverage, but it isn't currently a common pattern in the core blocks, so just checking if there are any reasons why we wouldn't/shouldn't take this approach? Personally, I would see this as a 'do it as and when it seems to make sense due to size and complexity of a block', rather than a 'This is best practice so let's insist that every block is set up like this from now on' 😃 |
I haven't tested it yet, but it makes sense to me. And having worked on the Jetpack blocks, event split between edit logic and controls was helpful. Thanks for working on this @glendaviesnz I can't help but wonder about the half-life of this particular block. As others have said, maybe once we get background block supports in order this block might become redundant 🤷 |
|
@glendaviesnz, this is always a great idea to split monolithic files into smaller chunks. In particular, for React components, it often brings additional benefits when you can decrease the reasons for the component to re-render. The only thing I would suggest is to group all those newly introduced files in the I wouldn't mind if that would be the codified standard for all core blocks in the https://github.com/WordPress/gutenberg/tree/trunk/packages/block-library#contributing-to-this-package section. |
Good idea, have done that. Will sort out the conflict tomorrow, and will see if the root dir can be tidied up any more re native edit files. |
|
|
|
There are some merge conflicts to resolve but otherwise we should plan to land it as soon as possible 👍 |
added 7 commits
May 11, 2022 10:09
…s allows block-level unit tests to be written
…ng, rather than unwrapping the children in the component
glendaviesnz
force-pushed
the
refactor/cover-block-controls
branch
from
c8bca70 to
cf8c83b
Compare
|
@fluiddot just copying you into this as you recently made some changes to the mobile side of the Cover block. I have moved the web side of the edit method into a subfolder, and split it out into separate files. I don't think this will affect the mobile side, but can you have a quick look and just double check that please? It probably makes sense to move the mobile edit related files into the same edit/ subfolder, but I am thinking that maybe we should do that as a separate PR, what do you think? |
andrewserong
approved these changes
May 10, 2022
There was a problem hiding this comment.
This is such a nice improvement @glendaviesnz! I'm finding the refactored files much easier to navigate and browse, and putting them together in the edit directory makes good sense.
✅ The cover block behaves just as it did before in the editor (I manually tested all the block and inspector controls).
✅ The cover block still behaves as expected in the iOS simulator (refactoring the native side in a separate PR sounds good to me)
✅ Sample tests pass.
LGTM! 🎉
ramonjd
approved these changes
May 11, 2022
Thanks for the ping @glendaviesnz 🙇 ! I've done a quick check on the code, as well as tested it locally, and can confirm that the changes didn't affect the native version of the block 🎊 .
I totally agree, now that we have the edit-related files in that subfolder, it would be great to have the native version files there too 👍 . I'll add a reminder to myself to do this in the next few days. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Extracts the Cover block Block Control and Inspector Control components into separate files, along with a couple of other components and utils.
Why?
The Edit component file is currently large and hard to work with and almost impossible to test. This structure should hopefully make it easier to work with long term as well as make it possible to write unit/integration level tests for some of the block control functionality.
How?
Mostly as straight move of code into separate files and a couple of variable/method renames, should be no change to functionality
Testing Instructions
npm run test-unit packages/block-library/src/cover/test/block-controls.jssample unit test runs