-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[RNMobile] Fix crash when using the delete key to remove a single button #51435
Conversation
Size Change: +3.97 kB (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
Flaky tests detected in 2e4fa04. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5284647482
|
@@ -327,7 +327,7 @@ function ButtonEdit( props ) { | |||
function onRemove() { | |||
const { onDeleteBlock, onReplace } = props; | |||
|
|||
if ( numOfButtons === 1 ) { | |||
if ( numOfButtons === 1 && onDeleteBlock ) { |
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.
It looks like we missed passing this prop in a recent refactor, this onDeleteBlock
prop should be added here.
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.
Can confirm that passing the onDeleteBlock
as a prop in block.native.js
resolves the issue. 👍 I've updated and removed this boolean check.
|
||
expect( getEditorHtml() ).toMatchSnapshot(); | ||
|
||
const component = getTestComponentWithContent(); |
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 think once we update the missing prop as mentioned here we can just check for the editor's HTML snapshot. Since deleting the last Button block will remove the block completely, what do you think?
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.
Updated after moving the onDeleteBlock
prop to block.native.js
.
packages/block-library/src/buttons/test/__snapshots__/edit.native.js.snap
Outdated
Show resolved
Hide resolved
packages/block-library/src/buttons/test/__snapshots__/edit.native.js.snap
Outdated
Show resolved
Hide resolved
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.
Test plan succeeded for me on an iPhone 14 Pro simulator. Thank you for fixing this!
I left one non-blocking comment to consider.
// Trigger inner blocks layout | ||
const innerBlockListWrapper = await within( | ||
buttonsBlock | ||
).findByTestId( 'block-list-wrapper' ); | ||
fireEvent( innerBlockListWrapper, 'layout', { | ||
nativeEvent: { | ||
layout: { | ||
width: 300, | ||
}, | ||
}, | ||
} ); |
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.
We now have a triggerBlockListLayout
test helper to perform this frequently required action for inner blocks. It may be beneficial to use the helper for the sake of consistency and conciseness. WDYT?
diff --git a/packages/block-library/src/buttons/test/edit.native.js b/packages/block-library/src/buttons/test/edit.native.js
index fc666d97bf..a586ec2b46 100644
--- a/packages/block-library/src/buttons/test/edit.native.js
+++ b/packages/block-library/src/buttons/test/edit.native.js
@@ -242,17 +242,7 @@ describe( 'Buttons block', () => {
// Get block
const buttonsBlock = await getBlock( screen, 'Buttons' );
- // Trigger inner blocks layout
- const innerBlockListWrapper = await within(
- buttonsBlock
- ).findByTestId( 'block-list-wrapper' );
- fireEvent( innerBlockListWrapper, 'layout', {
- nativeEvent: {
- layout: {
- width: 300,
- },
- },
- } );
+ triggerBlockListLayout( buttonsBlock );
// Get inner button block
const buttonBlock = await getBlock( screen, 'Button' );
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.
Great suggestion. We have a lot of helpers. 😅 I've updated. 👍
…ton (#51435) * Fix crash when using the delete key to remove a single button * Add onDeleteBlock prop to block.native.js * Update button edit.native.js snapshots * Update button delete test to focus button input * Update button test to use triggerBlockListLayout
* Release script: Update react-native-editor version to 1.97.0 * Release script: Update with changes from 'npm run core preios' * Update CHANGELOG.md * Update CHANGELOG.md * Update CHANGELOG.md Fix white space * Release script: Update react-native-editor version to 1.97.1 * Release script: Update with changes from 'npm run core preios' * [RNMobile] Fix crash when using the delete key to remove a single button (#51435) * Fix crash when using the delete key to remove a single button * Add onDeleteBlock prop to block.native.js * Update button edit.native.js snapshots * Update button delete test to focus button input * Update button test to use triggerBlockListLayout * [RNMobile] Ensure text input field is not editable when Bottom sheet cell is disabled (#51567) * Update `react-native-editor` changelog --------- Co-authored-by: jhnstn <jhnstn@pm.me> Co-authored-by: Jason Johnston <jhnstn@users.noreply.github.com> Co-authored-by: Derek Blank <derekpblank@gmail.com>
…ton (WordPress#51435) * Fix crash when using the delete key to remove a single button * Add onDeleteBlock prop to block.native.js * Update button edit.native.js snapshots * Update button delete test to focus button input * Update button test to use triggerBlockListLayout
* Release script: Update react-native-editor version to 1.97.0 * Release script: Update with changes from 'npm run core preios' * Update CHANGELOG.md * Update CHANGELOG.md * Update CHANGELOG.md Fix white space * Release script: Update react-native-editor version to 1.97.1 * Release script: Update with changes from 'npm run core preios' * [RNMobile] Fix crash when using the delete key to remove a single button (WordPress#51435) * Fix crash when using the delete key to remove a single button * Add onDeleteBlock prop to block.native.js * Update button edit.native.js snapshots * Update button delete test to focus button input * Update button test to use triggerBlockListLayout * [RNMobile] Ensure text input field is not editable when Bottom sheet cell is disabled (WordPress#51567) * Update `react-native-editor` changelog --------- Co-authored-by: jhnstn <jhnstn@pm.me> Co-authored-by: Jason Johnston <jhnstn@users.noreply.github.com> Co-authored-by: Derek Blank <derekpblank@gmail.com>
What?
Fixes a crash when using the delete key to remove a single button:
Why?
After typing text into a button and using the delete key to remove the text, it should also be possible to remove the button itself using the delete key. Currently, using the delete key to remove a button results in a fatal exception. This occurs when the
onDeleteBlock
is sent asundefined
to this code block:gutenberg/packages/block-library/src/button/edit.native.js
Lines 327 to 335 in c402cde
How?
Adds missing
onDeleteBlock
prop toblock.native.js
. Adds test to check the button removal with the delete key (in addition to the existing test to use the "Remove Block" menu option).Testing Instructions
Screenshots or screencast
Button.Block.-.Before.mov
Button.Block.-.After.mov