Mobile - Columns block - Fix transforming into a Group block crash #54035
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
added 2 commits
August 29, 2023 19:13
…hat's nested in a container block like Group. By avoid accessing the width value outside of the isSelected and adding a fallback value.
geriux
added
[Type] Bug
|
Size Change: 0 B Total Size: 1.51 MB ℹ️ View Unchanged
|
|
Flaky tests detected in 7069a35. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6027262663
|
fluiddot
approved these changes
Aug 30, 2023
Comment on lines
+115
to
+116
| const { width: columnWidth } = contentStyle[ clientId ] || {}; | ||
| const isFullWidth = columnWidth === screenWidth; |
There was a problem hiding this comment.
The value passed as contentStyle is contentWidths, which is calculated in:
gutenberg/packages/block-library/src/columns/edit.native.js
Lines 147 to 157 in 1ae7d9f
I checked the parameters passed in getContentWidths when transforming the block to a Group block, and noticed that columnCount is 0 in the first render after the transformation. In the second render, the column count is set to back the proper value and populates contentWidths with the expected data. Seems that upon transformation there's an initial state that doesn't have the columns parameter ready. That said, this approach makes sense and will definitely address the crash.
| @@ -133,7 +133,7 @@ function ColumnEdit( { | |||
| ); | |||
| } | |||
| return null; | |||
| }, [ contentStyle[ clientId ], screenWidth, isSelected, hasChildren ] ); | |||
| }, [ contentStyle, clientId, screenWidth, isSelected, hasChildren ] ); | |||
There was a problem hiding this comment.
The value passed as contentStyle (i.e. contentWidths) is memoized here:
gutenberg/packages/block-library/src/columns/edit.native.js
Lines 147 to 157 in 1ae7d9f
Hence, using this new dependency list won't produce extra renders.
There was a problem hiding this comment.
Yup! Since it's still a dependency I left it to also avoid the ESLint warning, although it can be disabled it's best to leave it.
|
@geriux I forgot to mention that it would be great to add an entry to the |
added 3 commits
August 30, 2023 17:59
…ow its imported from the helpers instead of the initializedEditor
Thanks for the reminder! I've added a changelog entry for this change. |
geriux
removed
the
Mobile App - Automation
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.
Fixes wordpress-mobile/gutenberg-mobile#6100
Related PRs:
What?
This PR fixes a crash when transforming a nested Columns block into a Group block.
By checking the error it was happening around:
Checking the breadcrumbs shows the last UI change before the crash:
Why?
To fix some crashes in production related to transforming Columns blocks into Group blocks.
How?
By moving part of the logic of
renderAppenderwithin theisSelectedif condition and also by adding a default value tocontentStylein case ofundefinedvalues.Testing Instructions
Precondition: Use the following HTML code:
Transform block...GroupScreenshots or screencast
ColumnsBefore.mov
ColumnsAfter.mov