[Mobile] - RichText - Use parsed font size values when comparing new changes #37951
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
…anges to avoid not matching values that generate a render loop
geriux
added
the
Mobile App - i.e. Android or iOS
|
Size Change: +674 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
2 tasks
fluiddot
reviewed
Jan 14, 2022
There was a problem hiding this comment.
I've added locally a test case to the integration test for the RichText component (reference) for checking the issue and I managed to reproduce the infinite loop issue:
it( 'should update the font size with provided style', () => {
// Arrange
const fontSize = '10';
const style = { fontSize: '12' };
// Act
const screen = render( <RichText fontSize={ fontSize } /> );
screen.update(
<RichText fontSize={ fontSize } style={ style } />
);
// Assert
expect( screen.toJSON() ).toMatchSnapshot();
} );
I tested a different flow where the font size could be updated when the style prop with a fontSize property is passed once the component is already mounted, not sure if this case is possible but I bumped into the same loop issue:
Maximum update depth exceeded. This can happen when a component repeatedly calls setState inside componentWillUpdate or componentDidUpdate. React limits the number of nested updates to prevent infinite loops.
Thank you @fluiddot! I'll check it out! 🙇 |
I was also wondering if we could cover the test case you described checking the following HTML via an integration test: I could help with that in case you think it's worth it 🙇 . |
In case it helps, here is a diff patch that adds a couple of test cases: Click here to show diff patchdiff --git a/packages/rich-text/src/test/__snapshots__/index.native.js.snap b/packages/rich-text/src/test/__snapshots__/index.native.js.snap
new file mode 100644
index 0000000000000000000000000000000000000000..eb684a9a01299ba4eb9f435fd81bb6654b8c6b0a
--- /dev/null
+++ b/packages/rich-text/src/test/__snapshots__/index.native.js.snap
@@ -0,0 +1,7 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`<RichText/> Font Size renders component with style and font size 1`] = `
+"<!-- wp:paragraph {\\"style\\":{\\"color\\":{\\"text\\":\\"#fcb900\\"},\\"typography\\":{\\"fontSize\\":35.56}}} -->
+<p class=\\"has-text-color\\" style=\\"color:#fcb900;font-size:35.56px\\">Lorem ipsum dolor sit amet, consectetur adipiscing elit. Sed imperdiet ut nibh vitae ornare. Sed auctor nec augue at blandit.</p>
+<!-- /wp:paragraph -->"
+`;
diff --git a/packages/rich-text/src/test/index.native.js b/packages/rich-text/src/test/index.native.js
index b48d8828326494e3acebd8a944cc6471f4dfc030..a6ce5a233fb3e55d4ca1a911934a2867834eee66 100644
--- a/packages/rich-text/src/test/index.native.js
+++ b/packages/rich-text/src/test/index.native.js
@@ -1,17 +1,27 @@
-jest.mock( '@wordpress/data/src/components/use-select' );
/**
* External dependencies
*/
import { Dimensions } from 'react-native';
-import { render } from 'test/helpers';
+import { getEditorHtml, render, initializeEditor } from 'test/helpers';
+
/**
* WordPress dependencies
*/
-import { useSelect } from '@wordpress/data';
+import { select } from '@wordpress/data';
+import { store as blockEditorStore } from '@wordpress/block-editor';
+import { coreBlocks, registerBlock } from '@wordpress/block-library';
+import {
+ getBlockTypes,
+ setDefaultBlockName,
+ unregisterBlockType,
+} from '@wordpress/blocks';
+import '@wordpress/jest-console';
+
/**
* Internal dependencies
*/
import RichText from '../component/index.native';
+import { store as richTextStore } from '../store';
/**
* Mock `useSelect` with various global application settings, e.g., styles.
@@ -25,16 +35,13 @@ const mockGlobalSettings = (
const DEFAULT_GLOBAL_STYLES = {
__experimentalGlobalStylesBaseStyles: { typography: { fontSize } },
};
- const selectMock = {
- getFormatTypes: jest.fn().mockReturnValue( [] ),
- getBlockParents: jest.fn(),
- getBlock: jest.fn(),
- getSettings: jest.fn().mockReturnValue( DEFAULT_GLOBAL_STYLES ),
- };
- useSelect.mockImplementation( ( callback ) => {
- return callback( () => selectMock );
- } );
+ jest.spyOn( select( blockEditorStore ), 'getSettings' ).mockReturnValue(
+ DEFAULT_GLOBAL_STYLES
+ );
+ jest.spyOn( select( richTextStore ), 'getFormatTypes' ).mockReturnValue(
+ []
+ );
};
describe( '<RichText/>', () => {
@@ -51,6 +58,13 @@ describe( '<RichText/>', () => {
[ '1.42vh', 19 ],
];
+ beforeAll( () => {
+ // Register Paragraph block
+ const paragraph = coreBlocks[ 'core/paragraph' ];
+ registerBlock( paragraph );
+ setDefaultBlockName( paragraph.name );
+ } );
+
beforeEach( () => {
mockGlobalSettings( {} );
} );
@@ -59,6 +73,13 @@ describe( '<RichText/>', () => {
Dimensions.set( { window } );
} );
+ afterAll( () => {
+ // Clean up registered blocks
+ getBlockTypes().forEach( ( block ) => {
+ unregisterBlockType( block.name );
+ } );
+ } );
+
describe( 'Font Size', () => {
it( 'should display rich text at the DEFAULT font size.', () => {
// Arrange
@@ -193,5 +214,27 @@ describe( '<RichText/>', () => {
const actualFontSize = getByA11yLabel( 'editor' ).props.fontSize;
expect( actualFontSize ).toBe( expectedFontSize );
} );
+
+ it( 'should update the font size when style prop with font size property is provided', () => {
+ // Arrange
+ const fontSize = '10';
+ const style = { fontSize: '12' };
+ // Act
+ const screen = render( <RichText fontSize={ fontSize } /> );
+ screen.update( <RichText fontSize={ fontSize } style={ style } /> );
+ // Assert
+ expect( screen.toJSON() ).toMatchSnapshot();
+ } );
+
+ it( 'renders component with style and font size', async () => {
+ // Arrange
+ const initialHtml = `<!-- wp:paragraph {"style":{"color":{"text":"#fcb900"},"typography":{"fontSize":35.56}}} -->
+ <p class="has-text-color" style="color:#fcb900;font-size:35.56px">Lorem ipsum dolor sit amet, consectetur adipiscing elit. Sed imperdiet ut nibh vitae ornare. Sed auctor nec augue at blandit.</p>
+ <!-- /wp:paragraph -->`;
+ // Act
+ await initializeEditor( { initialHtml } );
+ // Assert
+ expect( getEditorHtml() ).toMatchSnapshot();
+ } );
} );
} );Note that I changed the way the store selectors are mocked, instead of mocking the entire |
added 2 commits
January 14, 2022 13:01
…there's a font size set from the styles
…integration tests
Awesome! Thanks for the diff Carlos, I'll apply that and remove the E2E test 👍 |
geriux
requested review from
fluiddot
and removed request for
cameronvoell and
ellatrix
|
@fluiddot Thank you for the suggestions, I've updated the code and PR descripton. This is ready for another review whenever you have some time 🙇 |
fluiddot
approved these changes
Jan 14, 2022
There was a problem hiding this comment.
LGTM 🎊
I performed tests on both platforms for covering the three described tests cases, all of them passed ✅ .
However, I spotted a couple of issues when testing on Android that I think they're not related to this change, as it works on iOS, but that are worth it to share:
Text overlaps on H1 headings
How to reproduce it:
- Add a Heading block.
- Set H1 level.
- Type a text long enough for being displayed in two or more lines.
- Observe that part of the text is overlapped.
Specified font size is not preserved when changing heading level
I verified that the behavior on both web and iOS when setting a font size is that changing the heading level doesn't modify the size. However, on Android, this is working differently (see attached video capture).
android-heading-font-size.mp4
Tested on Samsung Galaxy S20 FE 5G (Android 10) and Simulator - iPhone 12 Pro Max (iOS 15.0).
|
Thank you for testing and reviewing! 🎉 I'll investigate those Android issues regarding the line height and the Heading block to make sure those are fixed before releasing it to production 👍 |
mkevins
pushed a commit
that referenced
this pull request
Jan 17, 2022
…changes (#37951) * Mobile - RichText - Use parsed font size values when comparing new changes to avoid not matching values that generate a render loop * Mobile - E2E - Move typography test to full tests instead of canary * Mobile - RichText - Avoid using the fontSize value from the props if there's a font size set from the styles * Mobile - E2E - Remove test and move some test cases into the current integration tests * Mobile - RichTest - Update tests
mkevins
added a commit
that referenced
this pull request
Jan 19, 2022
* Release script: Update react-native-editor version to 1.69.0 * Release script: Update with changes from 'npm run core preios' * Update native editor changelog for v1.69.0 * Release script: Update react-native-editor version to 1.69.1 * Release script: Update with changes from 'npm run core preios' * Mobile - Gallery block - Fix issue migrating old format and missing fixedHeight context, use of images prop instead of the attributes (#37889) * [Mobile] - RichText - Use parsed font size values when comparing new changes (#37951) * Mobile - RichText - Use parsed font size values when comparing new changes to avoid not matching values that generate a render loop * Mobile - E2E - Move typography test to full tests instead of canary * Mobile - RichText - Avoid using the fontSize value from the props if there's a font size set from the styles * Mobile - E2E - Remove test and move some test cases into the current integration tests * Mobile - RichTest - Update tests * Fix LinkPicker freeze when virtual keyboard is hidden (#37782) * Fix LinkPicker freeze when virtual keyboard is hidden When a devices virtual keyboard is hidden, e.g. when a hardware keyboard is connected, dismissing the `LinkPicker` resulted in the application freezing. The freeze likely originates from an unconsumed `LayoutAnimation` registered during resizing of the `BottomSheet`. To address this issue, we now avoid registering a `LayoutAnimation` whenever the changes to the header are sub-pixel values, which can result in the `LayoutAnimation` going unconsumed. https://git.io/J9q2G Long-term, we should likely consider refactoring the `BottomSheet` to holistically avoid stacking `LayoutAnimations`: https://git.io/J9q2l * Support unique BottomSheet header testID This allows individual tests to pass a unique, top-level testID for the BottomSheet and have it propagate to the header as well, which may make querying easier. Co-authored-by: Carlos Garcia <fluiddot@gmail.com> * Fix indentation issue in bottom sheet component * Add change log entry * Add pull request reference to change log entry Co-authored-by: Carlos Garcia <fluiddot@gmail.com> * Update release notes * Add back missing dep in package-lock Co-authored-by: Gerardo Pacheco <gerardo.pacheco@automattic.com> Co-authored-by: David Calhoun <438664+dcalhoun@users.noreply.github.com> Co-authored-by: Carlos Garcia <fluiddot@gmail.com>
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.
Description
This PR fixes a bug within
RichText, where in some cases it would generate a render loop when setting a new font-size value and this didn't match the parsed value.With this change it'll parse the
currentFontSizeStyleandprevFontSizeStylebefore comparing values to update the state. As well as not using the font size value from its props if there's a font size provided from the styles.Note: The font size picker is behind the
__DEV__flag so it'll only show up with metro running for block-based themes. But it will render the font-size value correctly for both default and block-based themes.How has this been tested?
Test case 1
Paragraphblock to show correctly.Test case 2
Precondition: Using a block-based theme.
Paragraphblock type some textTest case 3
Precondition: Using a block-based theme.
Headingblock and type some textScreenshots
Types of changes
Bug fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).