-
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
Columns mobile block: avoid returning unstable innerWidths from useSelect #46403
Conversation
Size Change: 0 B Total Size: 1.32 MB ℹ️ View Unchanged
|
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.
Looking great code-wise 👍
Would be nice to get a ✅ from the RN team.
@@ -2,7 +2,7 @@ | |||
* External dependencies | |||
*/ | |||
import { View, Dimensions } from 'react-native'; | |||
import { map } from 'lodash'; | |||
|
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.
😍
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 knew you'd like this 😉
@@ -463,38 +463,35 @@ const ColumnsEdit = ( props ) => { | |||
getBlockAttributes, | |||
} = select( blockEditorStore ); | |||
const { isEditorSidebarOpened } = select( 'core/edit-post' ); | |||
const innerBlocks = getBlocks( clientId ); |
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 fixing the naming collision 👍
const isContentEmpty = innerBlocksList.every( | ||
( innerBlock ) => innerBlock.innerBlocks.length === 0 |
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.
Much better 👍 Previously, isContentEmpty
was an array of booleans, which didn't make much sense.
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.
Thank you for improving the code! LGTM. I also tested the changes on both iOS and Android.
Fixes a performance issue when editing the "Columns" block on mobile. I discovered this when improving React Native unit tests after the React 18 migration, removing "update not wrapped in
act()
" warnings.A
useSelect
call in theColumnsEdit
component returns aninnerWidth
array. This array is a different instance on every call, because it's a result of.map
, no matter whether the sourceinnerBlocks
have changed or not. That leads to forcedColumnsEdit
rerender on every change in theblockEditorStore
, relevant or not.I'm fixing this by returning the stable
innerBlocks
value from theuseSelect
, and doing theinnerWidth
calculation and memoization outsideuseSelect
. That leads to rerenders only wheninnerBlocks
or other selected property really change.I also simplified the calculation of
isContentEmpty
.