[RNMobile] Ensure that blockType is defined when accessing wrapperProps
#56846
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
derekblank
added
[Type] Bug
|
Size Change: 0 B Total Size: 1.71 MB ℹ️ View Unchanged
|
This was referenced Dec 7, 2023
|
Flaky tests detected in 74d5d8e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7178322278
|
fluiddot
approved these changes
Dec 11, 2023
There was a problem hiding this comment.
LGTM 🎊 ! Thanks @derekblank for addressing the crash 🏅 !
I'd like to share a couple of notes that would be great to tackle but that shouldn't be considered blockers to merge this PR:
- Add an entry to the
react-native-editorchangelog and release notes. - Add an integration test in GBM to cover this case.
The following issue is not a blocker and either a pressing issue we should solve now but wanted to share it as I noticed it when testing in the web version. Seems adding a Paywall block on a Page in the web version also results in an unsupported block. However, the message displayed is different from the one shown in the app. In both cases, I think the message is not accurate:
Web version:
Your site doesn’t include support for the "jetpack/paywall" block. You can leave this block intact or remove it entirely.
I presume this message should state that the block is unsupported in Pages, not the entire site.
App version:
Unsupported
'jetpack/paywall' is not fully-supported
As in the web version, we should mention that it's not supported only in Pages.
derekblank
added
the
Mobile App - Automation
|
Thank you for reviewing @fluiddot -- I've updated the CHANGELOG and opened a companion PR at wordpress-mobile/gutenberg-mobile#6458 with an update to release notes as well. As this change resolves an open crash issue, I've made a note to add an integration test and update the Paywall message issue in follow-up PRs, but agree they are not blockers to merging this PR. If you note that the CI checks have passed for the companion PR have passed and are able to merge that PR to keep the refs up to date, I would be most appreciative. Otherwise, I can pick it up when I return the week after next. 🙇 |
Sounds good, thanks 🙇 !
Sure, I'll check the companion PR and merge it once it's ready. |
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?
Ensure that
blockTypeis defined when accessingwrapperPropsin block.native.js.Fixes:
Why?
Prevents a crash in certain circumstances when
blockTypeis not defined, like when the block has been unregistered.How?
Matches the web block.js behavior by adding a check to ensure
blockTypeis defined when accessingwrapperProps.Current web behavior:
gutenberg/packages/block-editor/src/components/block-list/block.js
Lines 151 to 157 in 550966a
Current mobile behavior:
gutenberg/packages/block-editor/src/components/block-list/block.native.js
Lines 245 to 249 in 550966a
Proposed mobile change:
gutenberg/packages/block-editor/src/components/block-list/block.native.js
Lines 245 to 252 in 4d2e453
Testing Instructions