Skip to content
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.

Sign up for GitHub

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

Mobile - Columns block - Fix transforming into a Group block crash #54035

Merged
merged 5 commits into from
Aug 30, 2023
Merged

Mobile - Columns block - Fix transforming into a Group block crash #54035

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f204e13
Mobile - Column block - Fix issue when transforming a Columns block t…
Aug 29, 2023
1ae7d9f
Mobile - Columns block - Add transform Columns block integration test
Aug 29, 2023
cc5efa9
Mobile - Columns Block Tests - Update usage of the screen variable, n…
Aug 30, 2023
28ee737
Merge branch 'trunk' into rnmobile/fix-nested-columns-transform
Aug 30, 2023
7069a35
Mobile - Update changelog
Aug 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions packages/block-library/src/column/edit.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,10 +111,10 @@ function ColumnEdit( {
};

const renderAppender = useCallback( () => {
const { width: columnWidth } = contentStyle[ clientId ];
const isFullWidth = columnWidth === screenWidth;

if ( isSelected ) {
const { width: columnWidth } = contentStyle[ clientId ] || {};
const isFullWidth = columnWidth === screenWidth;
Comment on lines +115 to +116
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const contentWidths = useMemo(
() =>
getContentWidths(
columnsInRow,
width,
columnCount,
innerWidths,
globalStyles
),
[ width, columnsInRow, columnCount, innerWidths, globalStyles ]
);

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.


return (
<View
style={ [
Expand All @@ -133,7 +133,7 @@ function ColumnEdit( {
);
}
return null;
}, [ contentStyle[ clientId ], screenWidth, isSelected, hasChildren ] );
}, [ contentStyle, clientId, screenWidth, isSelected, hasChildren ] );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const contentWidths = useMemo(
() =>
getContentWidths(
columnsInRow,
width,
columnCount,
innerWidths,
globalStyles
),
[ width, columnsInRow, columnCount, innerWidths, globalStyles ]
);

gutenberg/packages/block-library/src/columns/edit.native.js

Line 290 in 1ae7d9f

contentStyle={ contentWidths }

Hence, using this new dependency list won't produce extra renders.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if ( ! isSelected && ! hasChildren ) {
return (
Expand Down
14 changes: 14 additions & 0 deletions packages/block-library/src/columns/test/__snapshots__/edit.native.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,20 @@ exports[`Columns block sets current vertical alignment on new Columns 1`] = `
<!-- /wp:columns -->"
`;

exports[`Columns block transforms a nested Columns block into a Group block 1`] = `
"<!-- wp:group {"layout":{"type":"constrained"}} -->
<div class="wp-block-group"><!-- wp:group {"layout":{"type":"constrained"}} -->
<div class="wp-block-group"><!-- wp:columns -->
<div class="wp-block-columns"><!-- wp:column {"width":"100%"} -->
<div class="wp-block-column" style="flex-basis:100%"><!-- wp:paragraph -->
<p></p>
<!-- /wp:paragraph --></div>
<!-- /wp:column --></div>
<!-- /wp:columns --></div>
<!-- /wp:group --></div>
<!-- /wp:group -->"
`;

exports[`Columns block when using columns percentage mechanism sets custom values correctly 1`] = `
"<!-- wp:columns -->
<div class="wp-block-columns"><!-- wp:column {"width":"90%"} -->
Expand Down
64 changes: 47 additions & 17 deletions packages/block-library/src/columns/test/edit.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,16 @@
import {
act,
addBlock,
dismissModal,
fireEvent,
getBlock,
getEditorHtml,
initializeEditor,
openBlockActionsMenu,
openBlockSettings,
within,
getBlock,
dismissModal,
screen,
waitForModalVisible,
within,
} from 'test/helpers';

/**
Expand Down Expand Up @@ -43,7 +45,7 @@ afterAll( () => {

describe( 'Columns block', () => {
it( 'inserts block', async () => {
const screen = await initializeEditor();
await initializeEditor();

// Add block
await addBlock( screen, 'Columns' );
Expand All @@ -55,7 +57,7 @@ describe( 'Columns block', () => {
} );

it( 'adds a column block using the appender', async () => {
const screen = await initializeEditor( {
await initializeEditor( {
initialHtml: TWO_COLUMNS_BLOCK_HTML,
} );

Expand All @@ -73,7 +75,7 @@ describe( 'Columns block', () => {

describe( 'when using the number of columns setting', () => {
it( 'adds a column block when incrementing the value', async () => {
const screen = await initializeEditor( {
await initializeEditor( {
initialHtml: TWO_COLUMNS_BLOCK_HTML,
} );
const { getByLabelText } = screen;
Expand All @@ -95,7 +97,7 @@ describe( 'Columns block', () => {
} );

it( 'adds at least 15 Column blocks without limitation', async () => {
const screen = await initializeEditor( {
await initializeEditor( {
initialHtml: TWO_COLUMNS_BLOCK_HTML,
} );
const { getByLabelText } = screen;
Expand All @@ -120,7 +122,7 @@ describe( 'Columns block', () => {
} );

it( 'removes a column block when decrementing the value', async () => {
const screen = await initializeEditor( {
await initializeEditor( {
initialHtml: TWO_COLUMNS_BLOCK_HTML,
} );
const { getByLabelText } = screen;
Expand All @@ -142,7 +144,7 @@ describe( 'Columns block', () => {
} );

it( 'reaches the minimum limit of number of column blocks', async () => {
const screen = await initializeEditor();
await initializeEditor();

// Add block
await addBlock( screen, 'Columns' );
Expand Down Expand Up @@ -185,7 +187,7 @@ describe( 'Columns block', () => {
} );

it( 'removes column with the remove button', async () => {
const screen = await initializeEditor( {
await initializeEditor( {
initialHtml: TWO_COLUMNS_BLOCK_HTML,
} );
const { getByLabelText } = screen;
Expand All @@ -210,7 +212,7 @@ describe( 'Columns block', () => {
} );

it( 'removes the only one left Column with the remove button', async () => {
const screen = await initializeEditor( {
await initializeEditor( {
initialHtml: TWO_COLUMNS_BLOCK_HTML,
} );
const { getByLabelText } = screen;
Expand Down Expand Up @@ -247,7 +249,7 @@ describe( 'Columns block', () => {
} );

it( 'changes vertical alignment on Columns', async () => {
const screen = await initializeEditor( {
await initializeEditor( {
initialHtml: TWO_COLUMNS_BLOCK_HTML,
} );
const { getByLabelText } = screen;
Expand All @@ -270,7 +272,7 @@ describe( 'Columns block', () => {
} );

it( 'changes the vertical alignment on individual Column', async () => {
const screen = await initializeEditor( {
await initializeEditor( {
initialHtml: TWO_COLUMNS_BLOCK_HTML,
} );

Expand Down Expand Up @@ -308,7 +310,7 @@ describe( 'Columns block', () => {
} );

it( 'sets current vertical alignment on new Columns', async () => {
const screen = await initializeEditor( {
await initializeEditor( {
initialHtml: TWO_COLUMNS_BLOCK_HTML,
} );
const { getByLabelText } = screen;
Expand Down Expand Up @@ -337,7 +339,7 @@ describe( 'Columns block', () => {

describe( 'when using columns percentage mechanism', () => {
it( "updates the slider's input value", async () => {
const screen = await initializeEditor();
await initializeEditor();

// Add block
await addBlock( screen, 'Columns' );
Expand Down Expand Up @@ -377,7 +379,7 @@ describe( 'Columns block', () => {
} );

it( 'sets custom values correctly', async () => {
const screen = await initializeEditor( {
await initializeEditor( {
initialHtml: TWO_COLUMNS_BLOCK_HTML,
} );
const { getByLabelText, getByTestId } = screen;
Expand Down Expand Up @@ -443,7 +445,7 @@ describe( 'Columns block', () => {
test.each( testData )(
'sets the predefined percentages for %s',
async ( layout ) => {
const screen = await initializeEditor();
await initializeEditor();

// Add block
await addBlock( screen, 'Columns' );
Expand All @@ -463,4 +465,32 @@ describe( 'Columns block', () => {
}
);
} );

it( 'transforms a nested Columns block into a Group block', async () => {
await initializeEditor( {
initialHtml: `<!-- wp:group {"layout":{"type":"constrained"}} -->
<div class="wp-block-group"><!-- wp:columns -->
<div class="wp-block-columns"><!-- wp:column {"width":"100%"} -->
<div class="wp-block-column" style="flex-basis:100%"><!-- wp:paragraph -->
<p></p>
<!-- /wp:paragraph --></div>
<!-- /wp:column --></div>
<!-- /wp:columns --></div>
<!-- /wp:group -->`,
} );

// Get Columns block
const columnsBlock = await getBlock( screen, 'Columns' );
fireEvent.press( columnsBlock );

// Open block actions menu
await openBlockActionsMenu( screen );

// Tap on the Transform block button
fireEvent.press( screen.getByLabelText( /Transform block…/ ) );

// Tap on the Group transform button
fireEvent.press( screen.getByLabelText( 'Group' ) );
expect( getEditorHtml() ).toMatchSnapshot();
} );
} );
1 change: 1 addition & 0 deletions packages/react-native-editor/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ For each user feature we should also add a importance categorization label to i
- [*] Improve horizontal rule styles to avoid invisible lines [#53883]
- [*] Fix horizontal rule style extensions [#53917]
- [*] Add block outline to all Social Link blocks when selected [#54011]
- [*] Columns block - Fix transforming into a Group block crash [#54035]

## 1.102.1
- [**] Fix Voice Over and assistive keyboards [#53895]
Expand Down
4 changes: 4 additions & 0 deletions test/native/__mocks__/styleMock.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,4 +204,8 @@ module.exports = {
},
picker: {},
pickerPointer: {},
columnsContainer: {
marginLeft: 16,
minWidth: 32,
},
};