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

[RNMobile] Fix losing undo/redo history when using non-breaking space HTML entity #57652

Merged
merged 7 commits into from
Jan 10, 2024
Merged

[RNMobile] Fix losing undo/redo history when using non-breaking space HTML entity #57652

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3991db3
Stop removing non-breaking space HTML entity
fluiddot Jan 8, 2024
a2a7b9a
Use `render` from test helpers in RichText unit tests
fluiddot Jan 8, 2024
e27f0e9
Rename describe section in RichText unit tests
fluiddot Jan 8, 2024
d80f70c
Use `RichTextData.empty` to create empty value in RichText unit tests
fluiddot Jan 8, 2024
307e15e
Add test case to cover non-breaking space HTML entity
fluiddot Jan 8, 2024
870a99a
Update `describe` description in RichText unit tests
fluiddot Jan 9, 2024
ef2802e
Update snapshots of `RichText` unit tests
fluiddot Jan 9, 2024
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
10 changes: 3 additions & 7 deletions packages/block-editor/src/components/rich-text/native/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,6 @@ import { getFormatColors } from './get-format-colors';
import styles from './style.scss';
import ToolbarButtonWithOptions from './toolbar-button-with-options';

const unescapeSpaces = ( text ) => {
return text.replace( / | /gi, ' ' );
};

// The flattened color palettes array is memoized to ensure that the same array instance is
// returned for the colors palettes. This value might be used as a prop, so having the same
// instance will prevent unnecessary re-renders of the RichText component.
Expand Down Expand Up @@ -318,7 +314,7 @@ export class RichText extends Component {
}

const contentWithoutRootTag = this.removeRootTagsProducedByAztec(
unescapeSpaces( event.nativeEvent.text )
event.nativeEvent.text
);
// On iOS, onChange can be triggered after selection changes, even though there are no content changes.
if ( contentWithoutRootTag === this.value.toString() ) {
Expand All @@ -333,7 +329,7 @@ export class RichText extends Component {

onTextUpdate( event ) {
const contentWithoutRootTag = this.removeRootTagsProducedByAztec(
unescapeSpaces( event.nativeEvent.text )
event.nativeEvent.text
);

this.debounceCreateUndoLevel();
Expand Down Expand Up @@ -660,7 +656,7 @@ export class RichText extends Component {

// Check and dicsard stray event, where the text and selection is equal to the ones already cached.
const contentWithoutRootTag = this.removeRootTagsProducedByAztec(
unescapeSpaces( event.nativeEvent.text )
event.nativeEvent.text
);
if (
contentWithoutRootTag === this.value.toString() &&
Expand Down
6 changes: 3 additions & 3 deletions ...ages/block-editor/src/components/rich-text/native/test/__snapshots__/index.native.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`<RichText/> Font Size renders component with style and font size 1`] = `
exports[`<RichText/> when applying the 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 -->"
`;

exports[`<RichText/> Font Size should update the font size when style prop with font size property is provided 1`] = `
exports[`<RichText/> when applying the font size should update the font size when style prop with font size property is provided 1`] = `
<View
style={
[
Expand Down Expand Up @@ -42,7 +42,7 @@ exports[`<RichText/> Font Size should update the font size when style prop with
</View>
`;

exports[`<RichText/> Font Size should update the font size with decimals when style prop with font size property is provided 1`] = `
exports[`<RichText/> when applying the font size should update the font size with decimals when style prop with font size property is provided 1`] = `
<View
style={
[
Expand Down
58 changes: 50 additions & 8 deletions packages/block-editor/src/components/rich-text/native/test/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,20 @@ import { Dimensions } from 'react-native';
import {
fireEvent,
getEditorHtml,
render,
initializeEditor,
render,
screen,
} from 'test/helpers';

/**
* WordPress dependencies
*/
import { select } from '@wordpress/data';
import { store as richTextStore, RichTextData } from '@wordpress/rich-text';
import {
store as richTextStore,
RichTextData,
__unstableCreateElement,
} from '@wordpress/rich-text';
import { coreBlocks } from '@wordpress/block-library';
import {
getBlockTypes,
Expand Down Expand Up @@ -83,11 +88,11 @@ describe( '<RichText/>', () => {
} );
} );

describe( 'when changes arrive from Aztec', () => {
Copy link
Member

Choose a reason for hiding this comment

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

It is fine if you'd like to change this formatting, but, for my own education, may I ask the rationale behind dropping the "when..." format?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed it mainly to make the section more open to other test cases like the one added in the PR. The formatting change was mainly driven by using the same we have in the other describe section:

gutenberg/packages/block-editor/src/components/rich-text/native/test/index.native.js

Line 148 in 307e15e

describe( 'Font Size', () => {

In any case, I don't have a strong opinion on the formatting. We could still use when..., but if prefer this option, it would be great to make it consistent across the file.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for sharing. In short, I support you if you would like to change this format. Expanding the section, more specifically removing from Aztec, makes complete sense. From my perspective, that expanded group description is a logical test case grouping to avoid repetition.

My longer response, if you are interested... 😄

I am all for consistency. That said, at this time, I would argue that we do not have a consistent approach established. For this specific example, even Font Size and Value changes differ on casing. Admittedly, casing is not an important thing by any means, but I figure it is relevant nonetheless.

My original goal with my inquiry was to avoid a situation where we (contributors) swap code back and forth. Without automated enforcement via a linter, it is difficult to ensure consistency. There are examples of the "when" formatting in the code base as well. That is not to say we must adhere to that, but, again, I inquired to understand the rationale for modifying this one location.

For context, I was taught the "when" pattern as an approach to make test output read as a sentence. I.e., [subject] when [context] should [expectation]. Again, not advocating that we adhere to this format, merely sharing my rationale.

Thank you for considering my perspective. 🙇🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @dcalhoun for elaborating on your response 🙇 !

I am all for consistency. That said, at this time, I would argue that we do not have a consistent approach established. For this specific example, even Font Size and Value changes differ on casing. Admittedly, casing is not an important thing by any means, but I figure it is relevant nonetheless.

Ah, good point. I'll update the description to match the casing.

My original goal with my inquiry was to avoid a situation where we (contributors) swap code back and forth. Without automated enforcement via a linter, it is difficult to ensure consistency. There are [examples of the "when" formatting](https://github.com/search?q=repo%3AWordPress%2Fgutenberg%20describe(%20%27when&type=code) in the code base as well. That is not to say we must adhere to that, but, again, I inquired to understand the rationale for modifying this one location.

For context, I was taught the "when" pattern as an approach to make test output read as a sentence. I.e., [subject] when [context] should [expectation]. Again, not advocating that we adhere to this format, merely sharing my rationale.

Yep, we don't have a clear guideline about the describe section. I checked the Gutenberg documentation about testing but there's no explicit reference to the describe statement, apart from the following:

Use a describe block to group test cases.

Using when, although a bit more verbose, I agree it's a format more aligned with the description in test cases. I'll update it for this file. Thanks 🙇 !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using when, although a bit more verbose, I agree it's a format more aligned with the description in test cases. I'll update it for this file. Thanks 🙇 !

I applied this suggestion in 870a99a.

describe( 'when the value changes', () => {
it( 'should avoid updating attributes when values are equal', async () => {
const handleChange = jest.fn();
const defaultEmptyValue = new RichTextData();
const screen = render(
const defaultEmptyValue = RichTextData.empty();
render(
<RichText
onChange={ handleChange }
value={ defaultEmptyValue }
Expand All @@ -101,9 +106,46 @@ describe( '<RichText/>', () => {

expect( handleChange ).not.toHaveBeenCalled();
} );

it( 'should preserve non-breaking space HTML entity', () => {
const onChange = jest.fn();
const onSelectionChange = jest.fn();
// The initial value is created using an HTML element to preserve
// the HTML entity.
const initialValue = RichTextData.fromHTMLElement(
__unstableCreateElement( document, '&nbsp;' )
);
render(
<RichText
onChange={ onChange }
onSelectionChange={ onSelectionChange }
value={ initialValue }
__unstableIsSelected
/>
);

// Trigger selection event with same text value as initial.
fireEvent(
screen.getByLabelText( /Text input/ ),
'onSelectionChange',
0,
0,
initialValue.toString(),
{
nativeEvent: {
eventCount: 0,
target: undefined,
text: initialValue.toString(),
},
}
);

expect( onChange ).not.toHaveBeenCalled();
expect( onSelectionChange ).toHaveBeenCalled();
} );
} );

describe( 'Font Size', () => {
describe( 'when applying the font size', () => {
it( 'should display rich text at the DEFAULT font size.', () => {
// Arrange.
const expectedFontSize = 16;
Expand Down Expand Up @@ -259,7 +301,7 @@ describe( '<RichText/>', () => {
const fontSize = '10';
const style = { fontSize: '12' };
// Act.
const screen = render( <RichText fontSize={ fontSize } /> );
render( <RichText fontSize={ fontSize } /> );
screen.update( <RichText fontSize={ fontSize } style={ style } /> );
// Assert.
expect( screen.toJSON() ).toMatchSnapshot();
Expand All @@ -281,7 +323,7 @@ describe( '<RichText/>', () => {
const fontSize = '10';
const style = { fontSize: '12.56px' };
// Act.
const screen = render( <RichText fontSize={ fontSize } /> );
render( <RichText fontSize={ fontSize } /> );
screen.update( <RichText fontSize={ fontSize } style={ style } /> );
// Assert.
expect( screen.toJSON() ).toMatchSnapshot();
Expand Down