refactor: BlockList uses Hooks #50293
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
Adopt modern React paradigms prior to refactoring the components composition. Completed to more closely mirror other code in the code base.
Adopt modern React paradigms prior to refactoring the components composition. Completed to more closely mirror other code in the code base.
dcalhoun
added
[Feature] Blocks
|
Size Change: 0 B Total Size: 1.38 MB ℹ️ View Unchanged
|
Adopt modern React paradigms prior to refactoring the components composition. Completed to more closely mirror other code in the code base. Additionally, this removes the `withPreferredColorScheme` higher-order component as it appeared unused.
`VirtualList` expects a React Component (e.g. `SomeComponent`), or a React element (e.g. `<SomeComponent` />) passed to the `ListFooterComponent` prop. Passing a function instead resulted in the block inserter never opening when tapping the block appender button within the footer; this appeared to be due to excessive re-renders resetting the `isOpen` state for the inserter.
The typo led to an undefined callback function.
This prop was originally accessed via the `withSelect` higher-order component. When converting to Hooks, passing this prop was overlooked.
dcalhoun
commented
May 5, 2023
There was a problem hiding this comment.
I generated installable iOS and Android builds for testing purposes.
There may be opportunity to optimize performance. I have not yet measured with React Profiler to see if there are differences between the previous class component.
I attempted to create a Reassure test, but abandoned it for now after encountering some difficult errors.
packages/block-editor/src/components/block-list/block-list-item-cell.native.js
Show resolved
Hide resolved
| return { | ||
| clearSelectedBlock, | ||
| insertBlock, | ||
| replaceBlock, |
There was a problem hiding this comment.
replaceBlock appeared unused and was removed.
| accessibilityLabel={ __( 'Add paragraph block' ) } | ||
| testID={ __( 'Add paragraph block' ) } | ||
| onPress={ () => { | ||
| const paragraphBlock = createBlock( 'core/paragraph' ); |
There was a problem hiding this comment.
In order to satisfy React Native VirtualizedList's expectation of a React class component or render function, the creation of the Paragraph block was moved to the event hander, rather than in the render.
|
I used the React Profiler to observe render count and duration for a few interactions. It appears to be comparable to the previous class component.
|
geriux
reviewed
May 5, 2023
packages/block-editor/src/components/block-list/block-list-item-cell.native.js
Show resolved
Hide resolved
Running all timers could create testing failures in scenarios wheres where inner blocks were inserted, e.g. Social Icons. It is not known why, but scoping from "all" to "pending" timers resolved the following errors, without breaking any existing tests. Unknown origin: ``` Warning: You seem to have overlapping act() calls, this is not supported. Be sure to await previous act() calls before making a new one. ``` Originating from `waitForStoreResolvers`: ``` Ran 100000 timers, and there are still more! Assuming we've hit an infinite recursion and bailing out... ``` Originating from `packages/block-editor/src/components/block-list/test/index.native.js`: ``` thrown: "Exceeded timeout of 5000 ms for a test. ```
Added to align with sibling tests.
|
Flaky tests detected in af77614. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4915356759
|
geriux
reviewed
May 8, 2023
geriux
approved these changes
May 8, 2023
There was a problem hiding this comment.
These changes look good to me, nice work migrating the code from a class component into a functional one 👏
I have used the test builds for both iOS and Android to test: Block insertion, Drag&Drop, nesting blocks, color customization, auto-scroll, and VoiceOver/Talkback. All working well 🚀
NOTE: This branch doesn't include the latest changes from trunk so some visual regression tests are failing over this PR it'd be nice to update this branch and rerun the full tests before merging just in case.
…block-list-uses-hooks
4 tasks
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?
Refactor the mobile
BlockListcomponent to use Hooks.Why?
Relates to wordpress-mobile/gutenberg-mobile#5617.
Adopt modern React paradigms prior to refactoring the components
composition. Completed to more closely mirror other code in the code
base.
How?
Replace
BlockListandEmptyListclass components with React Hooks. Remove an apparent unused inclusion ofwithPreferredColorScheme.Testing Instructions
Given this changes foundational code for the block list, we should likely test
adding/editing/relocating/removing blocks in various ways, including VoiceOver
and TalkBack. We should also be mindful of any performance regressions.
Testing Instructions for Keyboard
n/a
Screenshots or screencast
n/a