Preserve text inside code fences

[marktoman]

Details

Pulls in changes from updated Expensimark code on Expensify/expensify-common#496. This makes it so that we preserve text within code fences instead of stripping out extra new lines at the bottom. This is because we feel it makes more sense to preserve what a user has entered vs trying to be "helpful" and remove text they may or may not have wanted.

Fixed Issues

$ #14694
$ #14694 (comment)

Tests

Steps (validation + regressions)

1. Add a comment:

```
text

```

2. Verify that it has an empty line below text once added.
3. Edit it and check that it looks the same as 1.
4. Repeat 1-3 for the below, but instead, make sure that it has no added line below text:

```
text
```

• Verify that no errors appear in the JS console

Offline tests

QA Steps

Steps (validation + regressions)

1. Add a comment:

```
text

```

2. Verify that it has an empty line below text once added.
3. Edit it and check that it looks the same as 1.
4. Repeat 1-3 for the below, but instead, make sure that it has no added line below text:

```
text
```

• Verify that no errors appear in the JS console

PR Author Checklist

• I linked the correct issue in the ### Fixed Issues section above
• I wrote clear testing steps that cover the changes made in this PR
  • I added steps for local testing in the Tests section
  • I added steps for the expected offline behavior in the Offline steps section
  • I added steps for Staging and/or Production testing in the QA steps section
  • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• I included screenshots or videos for tests on all platforms
• I ran the tests on all platforms & verified they passed on:
  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop
• I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• I followed proper code patterns (see Reviewing the code)
  • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
  • I verified that comments were added to code that is not self explanatory
  • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
  • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
  • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • I verified the JSDocs style guidelines (in STYLE.md) were followed
• If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• I followed the guidelines as stated in the Review Guidelines
• I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• I verified that if a function's arguments changed that all usages have also been updated correctly
• If a new component is created I verified that:
  • A similar component doesn't exist in the codebase
  • All props are defined accurately and each prop has a /** comment above it */
  • The file is named correctly
  • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • The only data being stored in the state is data necessary for rendering and nothing else
  • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
  • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
  • All JSX used for rendering exists in the render method
  • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
• If any new file was added I verified that:
  • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• If a new CSS style is added I verified that:
  • A similar style doesn't already exist
  • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
• If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

mac-chrome.mov

mac-safari.mov

Mobile Web - Chrome

android-chrome.mp4

Mobile Web - Safari

ios-safari.mov

Desktop

desktop.mov

iOS

ios-native.mov

Android

android-native.mp4

[melvin-bot]

@bondydaa @mananjadhav One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

[bondydaa]

👍 Please make sure you fill out the checklist as well and test / screenshot for all devices.

[mananjadhav]

@marktoman Quick bump on @bondydaa's comment to fill out the checklist and update the screenshots for all platforms.

[marktoman]

On iOS, when entering:

```
Test
```

the converted result (commentText here) is <pre>Test<br /></pre>, which is correct and the same for other platforms.

Editing works as expected:

But, it is rendered like this:

So I'm not sure if I should try to change how it is rendered somehow.

[bondydaa]

Hmm that's strange, it only happens on iOS (native or mWeb?) that it's still rendered with the extra new line?

[marktoman]

Only native, Safari works the same as desktop. I'll dig into the rendering.

[marktoman]

Apparently, the HTML standard renders the lines the way we settled on (the web/desktop videos), but this behavior is behind a flag on native:

Recommended value is true on non-web platforms.
https://meliorence.github.io/react-native-render-html/api/renderhtmlprops#enableexperimentalbrcollapsing

I've tested it by adding props.sharedProps.enableExperimentalBRCollapsing = true; here and it solves the problem, i.e., it makes the lines consistent between web and native (both ios and android).

[bondydaa]

Hmm the other half of that recommendation is slightly concerning

Also note that this is an experimental feature, thus subject to behavioral instability.

but I think you're right that's what we want to provide consistent behavior across all platforms.

Go ahead and commit that change then and we can go from there.

[bondydaa]

😬 though package*.json conflicts hopefully they're not too bad

[marktoman]

I've merged new changes and tested it everywhere, and it works as expected.

[marktoman]

Just checking if I haven't accidentally removed something in the issue description, the PR reviewer checklist will be added by a reviewer, correct?

[mananjadhav]

Reviewer Checklist

• I have verified the author checklist is complete (all boxes are checked off).
• I verified the correct issue is linked in the ### Fixed Issues section above
• I verified testing steps are clear and they cover the changes made in this PR
  • I verified the steps for local testing are in the Tests section
  • I verified the steps for Staging and/or Production testing are in the QA steps section
  • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
• I checked that screenshots or videos are included for tests on all platforms
• I included screenshots or videos for tests on all platforms
• I verified tests pass on all platforms & I tested again on:
  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop
• If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
• I verified proper code patterns were followed (see Reviewing the code)
  • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
  • I verified that comments were added to code that is not self explanatory
  • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
  • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
  • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • I verified the JSDocs style guidelines (in STYLE.md) were followed
• If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• I verified that this PR follows the guidelines as stated in the Review Guidelines
• I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
• I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• If a new component is created I verified that:
  • A similar component doesn't exist in the codebase
  • All props are defined accurately and each prop has a /** comment above it */
  • The file is named correctly
  • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • The only data being stored in the state is data necessary for rendering and nothing else
  • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
  • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
  • All JSX used for rendering exists in the render method
  • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
• If any new file was added I verified that:
  • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• If a new CSS style is added I verified that:
  • A similar style doesn't already exist
  • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
• If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
• I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

web-code-fence-trailing-new-line.mov

Mobile Web - Chrome

mweb-chrome-code-fence-trailing-new-line.mov

Mobile Web - Safari

mweb-safari-code-fence-trailing-new-line.mov

Desktop

desktop-code-fence-trailing-new-line.mov

iOS

ios-code-fence-trailing-new-line.mov

Android

android-code-fence-trailing-new-line.mov

[mananjadhav]

Just checking if I haven't accidentally removed something in the issue description, the PR reviewer checklist will be added by a reviewer, correct?

Thanks for the PR and the thorough updates @marktoman. I've reviewed the PR and completed the checklist. All good here @bondydaa.

[marktoman]

Can somebody please explain the process? Does this wait for somebody to assign an internal engineer?

[mananjadhav]

Waiting on @bondydaa to validate the checklist and merge the PR. Quick bump here @bondydaa

[bondydaa]

sorry was OOO on Friday, taking a look.

[github-actions]

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
App start TTI	711.020 ms → 744.060 ms (+33.040 ms, +4.6%)
App start runJsBundle	196.563 ms → 203.250 ms (+6.688 ms, +3.4%)
Open Search Page TTI	619.174 ms → 621.002 ms (+1.828 ms, ±0.0%)
App start nativeLaunch	19.700 ms → 20.581 ms (+0.881 ms, +4.5%)
App start regularAppStart	0.021 ms → 0.016 ms (-0.005 ms, -24.8%) 🟢

Show details

Name	Duration
App start TTI	Baseline Mean: 711.020 ms Stdev: 34.548 ms (4.9%) Runs: 646.2581420000643 658.4561639996246 664.6506549995393 671.2016899995506 679.6660909997299 680.6735420003533 680.7574399998412 683.0992289995775 683.2621309999377 690.8971379995346 692.0467199999839 698.0691579999402 701.4536269996315 701.6502499999478 702.2028170004487 710.8857370000333 711.7097530001774 713.184905000031 713.9781459998339 718.6859729997814 718.8425569999963 719.5311660002917 724.39708300028 725.6922129997984 731.0451579997316 734.7347160000354 745.8894859999418 748.9415389997885 751.0290769999847 767.0307740001008 785.0414129998535 797.6656809998676 Current Mean: 744.060 ms Stdev: 31.255 ms (4.2%) Runs: 697.1795159997419 706.2336870003492 708.5385950002819 711.7375830002129 714.4056029999629 714.5583429997787 714.9399720001966 716.4837109996006 716.9401639997959 718.3151939995587 721.9098020000383 729.1899229995906 730.6501029999927 731.5963460002095 734.977253000252 735.2438260000199 735.372507000342 735.5368079999462 744.9177390001714 751.0067109996453 753.9453560002148 759.9832340003923 760.7103749997914 763.2471129996702 766.5717010004446 774.51639300026 776.6515709999949 779.803549000062 784.3125769998878 791.8432879997417 796.6513029998168 831.9497229997069
App start runJsBundle	Baseline Mean: 196.563 ms Stdev: 22.853 ms (11.6%) Runs: 163 168 168 169 173 174 175 176 177 177 178 180 182 187 188 192 193 199 199 205 210 210 210 210 214 220 222 229 232 235 236 239 Current Mean: 203.250 ms Stdev: 19.232 ms (9.5%) Runs: 166 177 180 183 185 185 188 191 191 192 192 196 197 199 199 200 202 203 203 204 205 207 208 211 214 214 217 224 241 241 242 247
Open Search Page TTI	Baseline Mean: 619.174 ms Stdev: 13.147 ms (2.1%) Runs: 594.7272140001878 604.0690510002896 604.3973390003666 607.2174890004098 607.9179699998349 608.7488609999418 608.8706459999084 609.5496009998024 609.7531739994884 609.9184980001301 610.0963949998841 612.5826829997823 613.8948570005596 614.221516999416 614.2884940002114 615.3194989999756 615.8261719997972 617.4527590004727 618.5625 619.784994000569 620.9243569998071 625.0638830000535 626.1177979996428 628.7558190003037 631.0703529994935 631.8639319995418 637.660605000332 639.7180989999324 641.1832689996809 645.6620690003037 649.178222999908 Current Mean: 621.002 ms Stdev: 21.401 ms (3.4%) Runs: 586.2611499996856 588.8201510002837 592.7483320003375 593.1431080000475 593.4117029998451 596.478231000714 598.2098390003666 605.0117189995944 605.2066249996424 609.7136240005493 610.5273850001395 611.3129879999906 611.4617929998785 613.6605230001733 618.4485679995269 621.1558440001681 622.6337489997968 623.2713219998404 625.5866290004924 626.8615319998935 628.3117269994691 628.7832440007478 631.0598550001159 636.9826260004193 639.774902000092 639.7912200000137 640.3003740003332 641.0524499993771 645.446981000714 648.9714759998024 668.6184900002554 669.0450440002605
App start nativeLaunch	Baseline Mean: 19.700 ms Stdev: 1.487 ms (7.5%) Runs: 16 18 18 18 18 19 19 19 19 19 19 19 19 19 19 20 20 20 20 20 20 20 21 21 21 21 21 22 23 23 Current Mean: 20.581 ms Stdev: 2.122 ms (10.3%) Runs: 18 18 18 18 18 18 19 19 19 19 19 19 20 20 20 20 20 21 21 21 21 22 22 22 22 23 23 24 24 25 25
App start regularAppStart	Baseline Mean: 0.021 ms Stdev: 0.002 ms (7.6%) Runs: 0.018595000728964806 0.018718000501394272 0.019247000105679035 0.019694000482559204 0.019733999855816364 0.019898000173270702 0.019898000173270702 0.019979000091552734 0.020100999623537064 0.02018200047314167 0.020263000391423702 0.020263999700546265 0.020344999618828297 0.020508000627160072 0.02058899961411953 0.0206300001591444 0.020711000077426434 0.020752000622451305 0.021403000690042973 0.021525000222027302 0.0215659998357296 0.021769000217318535 0.021849999204277992 0.022094999440014362 0.022420000284910202 0.0224609998986125 0.02323400042951107 0.02355899941176176 0.024007000029087067 0.024291999638080597 0.025105999782681465 Current Mean: 0.016 ms Stdev: 0.001 ms (6.1%) Runs: 0.014161000028252602 0.014241000637412071 0.01464799977838993 0.0147299999371171 0.014933000318706036 0.015015000477433205 0.015135999768972397 0.015300000086426735 0.015461999922990799 0.015502999536693096 0.01550300046801567 0.015543999150395393 0.015625 0.015625 0.01574699953198433 0.015747000463306904 0.015868999995291233 0.015990999527275562 0.0159919997677207 0.016112999990582466 0.016316999681293964 0.01647899951785803 0.0165200000628829 0.01664199959486723 0.016885999590158463 0.016968000680208206 0.017008000053465366 0.017008000053465366 0.017741000279784203 0.018432999961078167

[OSBotify]

🚀 Deployed to staging by https://github.com/bondydaa in version: 1.2.75-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

[OSBotify]

🚀 Deployed to production by https://github.com/melvin-bot[bot] in version: 1.2.75-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅