-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Fix Reset All button in the typography panel of the block inspector #48123
Conversation
Size Change: +232 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
Flaky tests detected in c570cc5. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4240270986
|
I haven't worked directly on
I'll take a look at this PR, but I'd feel better if folks who worked directly on the component also took a look (cc'ing @aaronrobertshaw , @andrewserong, @ramonjd and @glendaviesnz ). Although there's a chance they may be slow to respond, since they're probably attending WCAsia. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I separated the registration of "resetAll" filters from the registration of "ToolsPanelItem" (the different controls within a panel) that way we can register a global "base" resetAll for all the TypographyPanel.
Prior to these changes, it looks like a ToolsPanel
wound run the resetAllFilter
callbacks of each of its associated/registered ToolsPanelItem
s.
With these changes, even non-registered ToolsPanelItems
s would be able to have their resetAllFilter
run when the resetAll
function from ToolsPanel
is run.
Could this have any implications?
It makes it very hard to build a "reusable component" that is used in several contexts (inspector control of a given block and global styles) like we did with
TypographyPanel
and where we can't assume that we have an attributes object, all we have is a "style" object and "onChange" callback.
Can't that be achieved by:
- using
resetAllFilter
on eachToolsPanelItem
- in the
ToolsPanel
'sresetAll
function, calling all of theresetAllFilter
functions passed as an argument to the function?
👍 Thanks for the ping. I'll happily take a closer look when I'm back online in a few days.
This was my first thought after taking a quick look at this PR as well. To prevent some edge cases, where fills were rendered into a panel when they no longer should, the Perhaps we could do with an extra unit test to certify the behaviour before and after the refactor? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the ping!
Perhaps we could do with an extra unit test to certify the behaviour before and after the refactor?
+1 it would be good to see if we can add extra unit test coverage. Just to add to Aaron's comment, we previously ran into tricky edge cases with the inspector controls panel open, and changing the block selection. Without the panelId
logic, we encountered fills being rendered when they shouldn't, but also stale values appearing in ToolsPanelItems.
In practice, though, so far this PR appears to be testing well for me, and I haven't encountered any issues with the reset all button so far.
packages/block-editor/src/components/global-styles/typography-panel.js
Outdated
Show resolved
Hide resolved
I'm not really clear what behavior you want to cover with a unit test, I'm happy to add a unit test but need more clarity here. Yes, this PR allows to add a global "resetAllFilter" for a "group of items" but that's intended and don't impact individual items. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really clear what behavior you want to cover with a unit test, I'm happy to add a unit test but need more clarity here. Yes, this PR allows to add a global "resetAllFilter" for a "group of items" but that's intended and don't impact individual items.
As far as I can tell, things appear to be working well for me locally, and other panels like Dimensions (and injected ToolsPanelItem
s like the Aspect Ratio controls in the Featured Image block) also appear to be working well. I can see that in the ToolsPanelItem
hook, it looks like the resetAllFilterCallback
function should be registered / deregistered when the panelId
changes, because resetAllFilterCallback
is defined via useCallback
and it lists panelId
as one of its dependencies. But I think this might be a good thing to try to unit test, both to confirm the behaviour is working as expected, and so that if folks attempt to make subsequent changes, the desired behaviour is documented.
In general with some of these sorts of changes, I wouldn't usually lean so hard toward unit testing to confirm (a good bug fix is a good bug fix), but I find reviewing ToolsPanel changes quite challenging due to the complexity of the API, and we've unfortunately introduced a few tricky to diagnose edge cases in the past, so it's good to try to avoid that if we can.
A possible / potential unit test might be something like the following:
- Render a ToolsPanel with panelId
a
and a ToolsPanelItem with panelIda
- Fire the resetAll on the ToolsPanel (e.g.
selectMenuItem( 'Reset all' );
) - ToolsPanelItem registered to panelId
a
'sresetAllFilter
callback should be fired - Change the ToolsPanel panelId to be set to
b
- Fire the resetAll on the ToolsPanel
- ToolsPanelItem registered to panelId
a
'sresetAllFilter
callback should not be fired
In terms of the before/after behaviour that Aaron mentions, I think that might help hone in on the target there?
Overall, though, I do like the direction of this PR, in that it allows for the parent ToolsPanel
to perform a final level of filtering over the reset behaviour. Nice work figuring out a way to balance the individual control over each ToolsPanelItem
's resetting behaviour with the desire for the parent to have more control!
The ToolsPanel
is a very complex component, and I agree with your notes that it'd be great to see if we can simplify some of the logic further down the track.
packages/block-editor/src/components/inspector-controls/fill.js
Outdated
Show resolved
Hide resolved
I've added a unit test and was actually able to fix a test failure thanks to it. Good call there. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for implementing all the changes and adding the test @youknowriad. I've re-tested the following:
✅ Typography panel in the block editor, resetting individual controls (including drop cap) and resetting all
✅ Typography panel in global styles, resetting individual controls and resetting all
✅ Dimensions panel in the block editor, including controls set via fills (e.g. min height of cover, aspect ratio on post featured image), resetting individual controls and resetting all
✅ Dimensions panel in global styles, resetting individual controls and resetting all
All looks good to me! ✨
Prior to merge, it looks like the tests will need a rebase now that the tests' refactor to TypeScript has been merged (#48275). Also it'd be good to add a changelog entry for the component change over in packages/components/CHANGELOG.md
.
02d49d1
to
c570cc5
Compare
+1 on the thanks for adding the extra unit test and improving the ToolsPanel component. |
What?
The Reset All button in the "Typography" panel within block inspectors is not working as expected. This is a regression that we missed in the refactor from #47356
How?
The way "reset all" works is very weird: Each "ToolsPanelItem" registers a "filter" to apply when the "resetAll" is called and the wrapping
ToolsPanel
receives the registered filters as an argument to pass to itsresetAll
callback prop.The reason we have this instead of a regular callback, is because we want third-parties to be able to hook into the "resetAll" function call to register their own resets that can run in addition to the "core one".
The way we register these callbacks now is by passing "registerAllFilter" prop to the
ToolsPanelItem
but the problem is that these filters assume that they receive a "block attributes" object and filter it. It makes it very hard to build a "reusable component" that is used in several contexts (inspector control of a given block and global styles) like we did withTypographyPanel
and where we can't assume that we have an attributes object, all we have is a "style" object and "onChange" callback.The solution here is a bit elaborate but allows us to have both:
I couldn't think of any other solution, please let me know if you have any other ideas.
Testing Instructions
1- Open the block inspector of a paragraph block
2- Edit some typography settings
3- Click "reset all" within the "..." menu.
4- It should reset all typography changes.