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.

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

update expensify-common #15241

Merged
merged 1 commit into from
Feb 20, 2023

Conversation

eh2077
Copy link
Contributor

@eh2077 eh2077 commented Feb 17, 2023

Details

This PR fixes line break handling for HTML h1 to markdown conversion

Fixed Issues

$ #14668
PROPOSAL: #14668 (comment)

Tests

  1. Open a chat and send this message
    # heading
    
  2. Edit the sent message and verify it's displayed in one line in editing mode
  3. Edit and change the sent message to
    # heading test
    
    and save it
  4. Verify the edited header bold text is still displayed in one line
  • Verify that no errors appear in the JS console

Offline tests

N/A

QA Steps

Same as above tests

  • 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
14668-web.mp4
Mobile Web - Chrome
14668-mobile-chrome.mp4
Mobile Web - Safari
14668-mobile-safari.mp4
Desktop
14668-desktop.mp4
iOS
14668-ios.mp4
Android
14668-android.mp4

@eh2077 eh2077 requested a review from a team as a code owner February 17, 2023 07:33
@melvin-bot melvin-bot bot requested review from danieldoglas and eVoloshchak and removed request for a team February 17, 2023 07:33
@MelvinBot
Copy link

@danieldoglas @eVoloshchak 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]

@eVoloshchak
Copy link
Contributor

eVoloshchak commented Feb 20, 2023

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
Screen.Recording.2023-02-20.at.19.20.52.mov
Mobile Web - Chrome
Screen_Recording_20230220-192944_Chrome.mp4
Mobile Web - Safari
Screen.Recording.2023-02-20.at.19.41.50.mov
Desktop
Screen.Recording.2023-02-20.at.19.30.11.mov
iOS
Screen.Recording.2023-02-20.at.19.37.52.mov
Android
Screen_Recording_20230220-202426_New.Expensify.mp4

Copy link
Contributor

@eVoloshchak eVoloshchak left a comment

Choose a reason for hiding this comment

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

Tests well!
cc: @danieldoglas

@danieldoglas danieldoglas merged commit adbe5f7 into Expensify:main Feb 20, 2023
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@github-actions
Copy link
Contributor

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries
Name Duration
App start TTI 690.403 ms → 729.560 ms (+39.156 ms, +5.7%)
App start runJsBundle 187.313 ms → 199.194 ms (+11.881 ms, +6.3%)
App start regularAppStart 0.022 ms → 0.015 ms (-0.007 ms, -31.9%) 🟢
App start nativeLaunch 20.613 ms → 19.966 ms (-0.647 ms, -3.1%)
Open Search Page TTI 617.309 ms → 606.896 ms (-10.413 ms, -1.7%)
Show details
Name Duration
App start TTI Baseline
Mean: 690.403 ms
Stdev: 31.490 ms (4.6%)
Runs: 640.99160400033 646.5574030000716 647.118257000111 654.755971999839 657.6313270004466 658.2138689998537 659.2472529998049 668.7838789997622 668.796532000415 672.8146749995649 675.2454709997401 676.2220200002193 677.9551959997043 679.8610619995743 680.4877869999036 680.7875570002943 685.0389390001073 691.8611279996112 696.2139339996502 696.2642919998616 697.4063769998029 697.9146889997646 711.2778939995915 714.5738450000063 721.8139720000327 727.4467609999701 733.0424159998074 737.3876759996638 740.7390679996461 748.8383470000699 757.2107309997082

Current
Mean: 729.560 ms
Stdev: 30.307 ms (4.2%)
Runs: 682.7107520001009 688.1668279999867 688.4214220000431 690.2370939999819 691.909737999551 702.4407209996134 703.446779999882 704.5393669996411 707.8275570003316 708.044893999584 709.6187840001658 713.3677719999105 722.4564929995686 723.3400729997084 725.4952619997784 726.9873529998586 728.602200999856 730.5603949995711 734.4940440002829 738.7841189997271 742.0477870004252 742.3298970004544 747.1994920000434 753.5999309998006 754.8698979998007 756.8237539995462 769.1668750001118 771.8767489995807 774.9962069997564 775.4341940004379 806.5524359997362
App start runJsBundle Baseline
Mean: 187.313 ms
Stdev: 19.585 ms (10.5%)
Runs: 158 158 164 166 168 168 170 174 175 175 177 177 179 179 180 182 182 184 184 188 190 192 198 200 204 206 208 211 219 219 224 235

Current
Mean: 199.194 ms
Stdev: 19.108 ms (9.6%)
Runs: 172 173 175 175 176 178 187 188 188 189 189 190 191 192 193 195 197 202 202 203 206 207 208 211 216 217 218 218 228 239 252
App start regularAppStart Baseline
Mean: 0.022 ms
Stdev: 0.002 ms (8.2%)
Runs: 0.01859499979764223 0.01896199956536293 0.0194089999422431 0.019410000182688236 0.019491000100970268 0.0195720000192523 0.019979000091552734 0.020018999464809895 0.020020000636577606 0.020100999623537064 0.02071099914610386 0.021321999840438366 0.021525000222027302 0.021647000685334206 0.021769000217318535 0.021769000217318535 0.021810000762343407 0.022012999281287193 0.022257000207901 0.022420000284910202 0.0224609998986125 0.022704999893903732 0.022948999889194965 0.023031000047922134 0.023152999579906464 0.0235190000385046 0.023599999956786633 0.02380300033837557 0.0244140001013875 0.026082000695168972

Current
Mean: 0.015 ms
Stdev: 0.001 ms (4.3%)
Runs: 0.013590999878942966 0.013794000260531902 0.013916000723838806 0.013957000337541103 0.013996999710798264 0.014120000414550304 0.014200999401509762 0.014240999706089497 0.01432300079613924 0.014364000409841537 0.014405000023543835 0.014525999315083027 0.014526000246405602 0.014527000486850739 0.014566999860107899 0.014607999473810196 0.0147299999371171 0.014770000241696835 0.014851999469101429 0.014851999469101429 0.014973999932408333 0.015340999700129032 0.01537999976426363 0.015381000004708767 0.01550300046801567 0.015584999695420265 0.015584999695420265 0.015584999695420265 0.015868999995291233 0.01587000023573637
App start nativeLaunch Baseline
Mean: 20.613 ms
Stdev: 3.179 ms (15.4%)
Runs: 17 17 18 18 18 18 18 18 18 19 19 19 19 19 19 19 20 20 20 21 21 21 21 22 23 23 24 26 27 28 29

Current
Mean: 19.966 ms
Stdev: 1.629 ms (8.2%)
Runs: 17 17 18 18 18 18 18 19 19 20 20 20 20 20 20 20 20 20 20 21 21 21 21 21 21 22 23 23 23
Open Search Page TTI Baseline
Mean: 617.309 ms
Stdev: 13.328 ms (2.2%)
Runs: 590.2817389992997 592.4326179996133 594.6939289998263 599.8495289999992 601.7798669999465 604.2808840004727 607.5093179997057 607.5441890005022 609.8099370002747 613.4153239997104 614.0454910006374 614.4007970001549 614.6816819999367 616.24768100027 618.7373869996518 618.9274089997634 619.8212900003418 620.5514730000868 622.7430429998785 623.9759519994259 624.5595300002024 625.8267830004916 625.8492029998451 627.535237999633 628.1431479994208 628.8428960004821 631.9644379997626 637.0799160003662 640.7792560001835 642.9540200000629

Current
Mean: 606.896 ms
Stdev: 15.484 ms (2.6%)
Runs: 572.091878999956 577.225056999363 580.8564050002024 588.7947180001065 591.3022050000727 592.6271969992667 593.0076900003478 594.3708089999855 594.8361009992659 596.2789710005745 598.4494639998302 600.2316900007427 603.0046389997005 605.4881189996377 605.7855640007183 606.8323969999328 606.9272870002314 608.326132000424 608.6433920003474 609.3852940006182 612.0590009996668 614.5997320003808 616.7046309998259 619.2305100001395 620.2520750006661 620.8047690000385 622.4710289994255 623.8865560004488 625.5780440000817 627.5196130005643 628.9215910006315 630.2984620006755 630.7747799996287

@OSBotify
Copy link
Contributor

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

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

@OSBotify
Copy link
Contributor

🚀 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 ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants