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.

Sign up for GitHub

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

Jump to bottom

Components: ToolsPanel: fix deregister/register on type #56770

Merged
merged 5 commits into from
Dec 6, 2023
Merged

Components: ToolsPanel: fix deregister/register on type #56770

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0861b0d
Components: ToolsPanel: fix deregister/register on type
ellatrix Dec 4, 2023
17a7dcb
Fix for registerResetAllFilter
ellatrix Dec 4, 2023
086accc
Fix unit test + add comment
ellatrix Dec 5, 2023
a010c66
Changelog
ellatrix Dec 5, 2023
c601a63
Adjust comment
ellatrix Dec 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
### Bug Fix

- `ToggleGroupControl`: react correctly to external controlled updates ([#56678](https://github.com/WordPress/gutenberg/pull/56678)).
- `ToolsPanel`: fix a performance issue ([#56770](https://github.com/WordPress/gutenberg/pull/56770)).

## 25.13.0 (2023-11-29)

Expand Down
31 changes: 10 additions & 21 deletions packages/components/src/tools-panel/tools-panel-item/hook.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,14 @@ export function useToolsPanelItem(
__experimentalLastVisibleItemClass,
} = useToolsPanelContext();

const hasValueCallback = useCallback( hasValue, [ panelId, hasValue ] );
const resetAllFilterCallback = useCallback( resetAllFilter, [
panelId,
resetAllFilter,
] );
// hasValue is a new function on every render, so do not add it as a
// dependency to the useCallback hook! If needed, we should use a ref.
// eslint-disable-next-line react-hooks/exhaustive-deps
const hasValueCallback = useCallback( hasValue, [ panelId ] );
// resetAllFilter is a new function on every render, so do not add it as a
// dependency to the useCallback hook! If needed, we should use a ref.
// eslint-disable-next-line react-hooks/exhaustive-deps
const resetAllFilterCallback = useCallback( resetAllFilter, [ panelId ] );
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add some comments to explain why we're disable the eslint rule.

This is the perfect use-case for the useEvent hook they proposed in React. not sure what happened to that proposal.

Copy link
Member Author

Choose a reason for hiding this comment

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

Wasn't aware of useEvent, but we have lots of cases in rich text and elsewhere too where we have to put stuff in a ref to access it in callbacks later.

const previousPanelId = usePrevious( currentPanelId );

const hasMatchingPanel =
Expand Down Expand Up @@ -126,27 +129,13 @@ export function useToolsPanelItem(
const newValueSet = isValueSet && ! wasValueSet;

// Notify the panel when an item's value has been set.
//
// 1. For default controls, this is so "reset" appears beside its menu item.
// 2. For optional controls, when the panel ID is `null`, it allows the
// panel to ensure the item is toggled on for display in the menu, given the
// value has been set external to the control.
useEffect( () => {
if ( ! newValueSet ) {
return;
}

if ( isShownByDefault || currentPanelId === null ) {
flagItemCustomization( label, menuGroup );
}
}, [
currentPanelId,
newValueSet,
isShownByDefault,
menuGroup,
label,
flagItemCustomization,
] );
flagItemCustomization( label, menuGroup );
}, [ newValueSet, menuGroup, label, flagItemCustomization ] );

// Determine if the panel item's corresponding menu is being toggled and
// trigger appropriate callback if it is.
Expand Down
Loading