Global style revisions: move change summary code and tests to block editor package #57411
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
ramonjd
added
[Type] Task
12 tasks
|
Size Change: +299 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 45b965e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7344938633
|
…Types and use useMemo
Change function signature to something like getGlobalStylesChangelist( next, previous, options = {} )
Move max-length functionality from the revision buttons to the util.
…hed results where necessary so truncation can occur on these too.
Exporting a single function with the option to truncate.
ramonjd
force-pushed
the
update/move-global-style-changes-compare-to-block-editor
branch
from
e6994d1
to
8634a6a
Compare
andrewserong
reviewed
Jan 1, 2024
|
|
||
| describe( 'getRevisionChanges', () => { | ||
| jest.mock( '@wordpress/blocks', () => { |
There was a problem hiding this comment.
Just a quick question while skimming through: why is this being mocked instead of calling registerBlockType to register a block for use in the tests? It looks like a lot of the current tests will register a test block and then unregister via:
afterEach( () => {
getBlockTypes().forEach( ( block ) => {
unregisterBlockType( block.name );
} );
} );
So was just wondering if it's worth doing that here, too?
There was a problem hiding this comment.
Thanks for checking it out!
Do you see an advantage to registering/unregistering a block over a mock implementation?
I don't have a preference, only to return the bare minimum required for the tests.
There was a problem hiding this comment.
This could just be a personal preference, but I like it when tests can use the "real" system as much as possible without having to mock or override behaviour in order to get tests passing. In that way, the tests follow the overall APIs in place rather than having to have something custom for a set of tests (so, perhaps I lean a little more to the integration-style tests over more unit-style tests). I see there are a few other tests using jest.mock( '@wordpress/blocks', but most seem to be using registerBlockType as far as I can tell.
My main thought whenever I see mocks, though, is "Oh, I wonder why that's mocked, why does a separate API need to be overridden?", so it sticks out a little to me, but for these particular tests, I'm not sure it matters either way. Not a blocker for me, happy for us to go with what you think works best here! 🙂
There was a problem hiding this comment.
That's convincing enough for me and the outcome is the same. 😄
Thanks for explaining. I will update.
andrewserong
approved these changes
Jan 2, 2024
There was a problem hiding this comment.
This is testing just as on trunk for me!
While testing I found a bug in translationMap that exists on trunk. We don't have mapping for the individual heading levels (i.e. h1, h2, etc), so if someone goes to update h1 heading size in global styles it currently outputs undefined element in the summary of changes:
Logged out here:
Should we try to fix that up here, or do it in a separate PR? Either way sounds fine!
This consolidation looks good to me, though 👍
| blockNames | ||
| ) { | ||
| const cacheKey = JSON.stringify( { revision, previousRevision } ); | ||
| function getGlobalStylesChangelist( next, previous ) { |
There was a problem hiding this comment.
Tiniest of nits: is change list / changelist one or two words? If two, should we camel case it?
This function isn't being exported so no need to change it if you don't want to!
There was a problem hiding this comment.
I actually searched this and found out that both are used all over the place. So I might leave it in the codebase for someone else to decide 😄
Eeeek! I will fix in a separate PR right away. Thanks for helping test this @andrewserong ! |
ramonjd
deleted the
update/move-global-style-changes-compare-to-block-editor
branch
This was referenced Jan 2, 2024
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?
getRevisionChangesand tests from the edit site package to the block editor packages.getGlobalStylesChangelistTODO
getBlockTypestransformationRelated to:
Why?
So that the editor can use this utility elsewhere, namely, in the save entity panel to describe global styles changes in the sidebar.
Testing Instructions
Check that there are no regressions in global styles revisions changes summaries.
The original PR was #56577
Testing instructions from that PR: