[RNMobile] - test break tags removal #19976
Conversation
cameronvoell
changed the title
There was a problem hiding this comment.
Nice job @cameronvoell ! Those tests make the intention here sooo much clearer imo! 🙇
I left a couple of minor comments, but they're really just suggestions/thoughts. This looks good to me! 🎉
| expect( richText.countBreaksBeforeParagraphTag( html ) ).toBe( 0 ); | ||
| } ); | ||
|
|
||
| it( 'Counts zero breaks when no break is not directly in front of ending paragraph tag', () => { |
There was a problem hiding this comment.
I think we have an unintended double negative here ("no break is not"). Maybe something like, "Count zero breaks when breaks do not directly precede an ending paragraph tag"?
| @@ -649,6 +650,16 @@ export class RichText extends Component { | |||
| return value; | |||
| } | |||
|
|
|||
| countBreaksBeforeParagraphTag( html ) { | |||
There was a problem hiding this comment.
What do you think about specifying that it's an end tag: countBreaksBeforeParagraphEndTag? I don't feel strongly about this.
| } | ||
| selection = { start: newSelectionStart, end: newSelectionEnd }; |
There was a problem hiding this comment.
I really like how you have the regex broken out into a separate function with tests (those tests you added are 😻).
I would have probably taken this a bit further and extracted out all or most of 707-718 into some kind of pure 'calculateSelectionBasedOnAztecModifications( html, { originalStartSelection, originalEndSelection } )function that took in the html and old selection and output the updated selection for the anticipated Aztec-output text. We would then end up with just having something like this in therender` method:
...
} else {
selection = calculateSelectionBasedOnAztecModifications( html, selection );
}
With that said, I think extracting that out at this point falls well within the realm of personal preference, and the changes you have made are already a really great improvement, so feel free to leave this as-is if you prefer. 😄
cameronvoell
added
the
Mobile App - i.e. Android or iOS
|
This PR looks like it's stale. Can you circle back @cameronvoell and either land it or close it? Thanks! |
gziolo
added
the
[Status] Stale
|
This PR looks to be abandoned, I marked it as stale. If it won’t get updated in the next few weeks it might get closed. It was approved with minor comments so it seems beneficial to rebase it and take to the finish line. |
|
@cameronvoell This issue was marked as stale in 2020 it has seen no movement since then, closing this PR for now. If anyone feels it should still be looked at, please add a comment and I can take another look. Thanks! |
Description
Fixes part of wordpress-mobile/gutenberg-mobile#1543 addressing some comments from here: #18138 (review)
This changes adds more visibility into selection adjustments that we are making in gutenberg mobile code to ensure compatibility with react-native-aztec.
The code tested in this change is related to addressing a crash that was surfaced by the WordPressAndroid app which is using this code: wordpress-mobile/WordPress-Android#9832
How has this been tested?
Tested using steps from the PR that originally added this code:
Also you can run the new tests using the following:
yarn test gutenberg/packages/rich-text/src/component/test/index.native.jsTypes of changes
This change is only introducing some new unit tests and a small refactor to accommodate those tests.
Checklist: