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

correct the emoji list for Windows OS and categories updation #15416

Merged
merged 1 commit into from
Feb 27, 2023

Conversation

Pujan92
Copy link
Contributor

@Pujan92 Pujan92 commented Feb 23, 2023

Details

Initialize the emoji list based on the OS and for windows remove the last category of FLAGS

Fixed Issues

$ #15106
PROPOSAL: #15106 (comment)

Tests

  1. Open emoji picker
  2. Reach the end and try to navigate more with the arrow keys
  3. Verify there isn't any console error (needs to test in Windows OS browser for larger screen)
  • Verify that no errors appear in the JS console

Offline tests

  1. Open emoji picker
  2. Reach the end and try to navigate more with the arrow keys
  3. Verify there isn't any console error (needs to test in Windows OS browser for larger screen)

QA Steps

  1. Open emoji picker
  2. Reach the end and try to navigate more with the arrow keys
  3. Verify there isn't any console error (needs to test in Windows OS browser for larger screen)
  • 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.
  • 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 author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web
Screen.Recording.2023-02-23.at.10.30.56.PM.mov
Mobile Web - Chrome

mobile (10)

Mobile Web - Safari

mobile (9)

Desktop
Screen.Recording.2023-02-23.at.11.02.10.PM.mov
iOS

Simulator Screen Shot - iPhone 14 - 2023-02-23 at 22 55 59

Android

Screenshot_1677173602

@Pujan92 Pujan92 requested a review from a team as a code owner February 23, 2023 17:33
@melvin-bot melvin-bot bot requested review from Gonals and sobitneupane and removed request for a team February 23, 2023 17:33
@MelvinBot
Copy link

@Gonals @sobitneupane 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]

Copy link
Contributor

@sobitneupane sobitneupane left a comment

Choose a reason for hiding this comment

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

Screenshots/Videos

Windows/Web
Windows.mp4
MacOS/Web
Screen.Recording.2023-02-24.at.09.27.59.mov
Mobile Web - Chrome
Screen.Recording.2023-02-24.at.09.33.48.mov
Mobile Web - Safari
Screen.Recording.2023-02-24.at.09.38.32.mov
Desktop
Screen.Recording.2023-02-24.at.09.25.19.mov
iOS
Screen.Recording.2023-02-24.at.09.31.41.mov
Android
Screen.Recording.2023-02-24.at.09.37.17.mov

Copy link
Contributor

@sobitneupane sobitneupane left a comment

Choose a reason for hiding this comment

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

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.

@Gonals Gonals merged commit 6a3ec3a into Expensify:main Feb 27, 2023
@github-actions
Copy link
Contributor

Performance Comparison Report 📊

Significant Changes To Duration

Name Duration
Open Search Page TTI 620.764 ms → 687.980 ms (+67.216 ms, +10.8%) 🔴
Show details
Name Duration
Open Search Page TTI Baseline
Mean: 620.764 ms
Stdev: 17.371 ms (2.8%)
Runs: 590.8297129999846 594.4033200005069 595.6840009996668 596.3435060000047 602.4115809998475 604.8977870000526 606.65625 609.1995449997485 611.875163000077 612.2928470000625 612.5911059998907 612.9984130002558 614.1601970000193 617.6059570000507 623.5329999998212 623.7434899993241 624.7547200005502 624.7834880002774 626.0202639997005 626.0686450004578 626.1314700003713 626.4626060002483 627.9702559998259 633.3363450001925 636.4680180000141 638.277383999899 638.548340999987 646.6560880001634 651.0894779991359 667.1147060003132

Current
Mean: 687.980 ms
Stdev: 20.844 ms (3.0%)
Runs: 635.7136640008539 646.6824139999226 657.9456790005788 665.800293000415 668.194010999985 670.3805339997634 673.6175939999521 675.776408999227 679.5397550007328 679.8800460007042 679.8885900005698 680.9828290008008 681.0393890002742 685.1933599999174 686.647299000062 687.5748290000483 689.4892179993913 691.6548669999465 693.8749190000817 694.134887999855 694.7576500000432 697.2339690001681 699.6169440001249 700.4240730004385 707.0275879995897 711.0405690008774 712.8980310000479 713.6429449999705 717.800862999633 720.21643100027 728.7113849995658

Meaningless Changes To Duration

Show entries
Name Duration
App start TTI 738.603 ms → 767.495 ms (+28.892 ms, +3.9%)
App start runJsBundle 204.813 ms → 210.125 ms (+5.313 ms, +2.6%)
App start nativeLaunch 19.552 ms → 20.067 ms (+0.515 ms, +2.6%)
App start regularAppStart 0.015 ms → 0.018 ms (+0.003 ms, +19.6%) 🟡
Show details
Name Duration
App start TTI Baseline
Mean: 738.603 ms
Stdev: 24.610 ms (3.3%)
Runs: 689.4819789999165 691.9884520000778 703.0821589999832 706.2837089998648 709.323185000103 709.898738999851 710.6362109999172 717.1911880001426 718.1248400001787 724.9857680001296 725.9777790000662 729.5615530000068 733.7421749997884 735.1021030000411 738.0903480001725 739.9601019998081 740.9300660002045 742.3695450001396 742.6357160001062 751.7037670002319 755.5762049998157 755.7649070001207 756.8085219999775 761.4124070000835 761.5470460001379 764.8927810001187 767.9118189997971 768.0801610001363 768.8855900000781 769.0359339998104 771.5964009999298 772.7273989999667

Current
Mean: 767.495 ms
Stdev: 22.042 ms (2.9%)
Runs: 712.5183880003169 724.7206690004095 735.1631300002337 737.4217259995639 748.1164250001311 749.2271140003577 749.9463969999924 753.2678479999304 756.7740070000291 757.7257200004533 758.129968999885 759.5571600003168 759.9702939996496 760.4189130002633 760.7818040000275 767.4377769995481 773.9189680004492 775.0510459998623 778.5802969997749 779.2932320004329 779.9078670004383 784.868386999704 784.9949489999563 787.252429000102 788.4570150002837 789.5725520001724 790.1854870002717 793.5674000000581 796.2278110003099 799.0936829997227 800.1937020001933
App start runJsBundle Baseline
Mean: 204.813 ms
Stdev: 17.684 ms (8.6%)
Runs: 181 182 185 187 188 188 191 191 192 193 193 196 198 198 199 199 200 200 202 204 206 209 211 215 215 221 222 223 236 239 244 246

Current
Mean: 210.125 ms
Stdev: 17.182 ms (8.2%)
Runs: 181 186 187 189 195 196 196 196 196 197 200 201 205 205 206 208 209 209 210 212 214 216 217 218 223 223 227 233 234 239 248 248
App start nativeLaunch Baseline
Mean: 19.552 ms
Stdev: 1.673 ms (8.6%)
Runs: 18 18 18 18 18 18 18 18 18 18 18 19 19 19 19 19 20 20 20 20 20 21 21 21 21 21 21 24 24

Current
Mean: 20.067 ms
Stdev: 1.788 ms (8.9%)
Runs: 17 18 18 18 18 18 19 19 19 19 19 19 19 20 20 20 20 20 20 20 21 21 21 22 22 22 22 23 24 24
App start regularAppStart Baseline
Mean: 0.015 ms
Stdev: 0.001 ms (7.0%)
Runs: 0.013142999727278948 0.013264999724924564 0.013509000185877085 0.013753000181168318 0.013915999792516232 0.013916000258177519 0.014078999869525433 0.014485999941825867 0.014566999860107899 0.01460800040513277 0.014649000018835068 0.014812000095844269 0.014973999932408333 0.014973999932408333 0.015055999625474215 0.015299000311642885 0.015381000004708767 0.015583999920636415 0.015584999695420265 0.015746999997645617 0.01582799991592765 0.015868999995291233 0.016032000072300434 0.016112999990582466 0.016113999765366316 0.016479999758303165 0.016480000223964453 0.0165200000628829 0.016559999901801348 0.016600999981164932 0.016642999835312366 0.016764999832957983

Current
Mean: 0.018 ms
Stdev: 0.001 ms (6.0%)
Runs: 0.015949999913573265 0.0166830001398921 0.016846000216901302 0.01708999928086996 0.017090000212192535 0.017292999662458897 0.017374999821186066 0.0174150001257658 0.017619000747799873 0.01782300043851137 0.017863000743091106 0.018025999888777733 0.018106999807059765 0.018147999420762062 0.01826899964362383 0.01826899964362383 0.018350999802350998 0.01843199972063303 0.0185139998793602 0.018798000179231167 0.0188809996470809 0.0188809996470809 0.018920999951660633 0.01912400033324957 0.019205999560654163 0.019979000091552734 0.020508000627160072 0.020711000077426434

@github-actions github-actions bot added the DeployBlockerCash This issue or pull request should block deployment label Feb 27, 2023
@github-actions
Copy link
Contributor

@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by https://github.com/Gonals in version: 1.2.77-0 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by https://github.com/Gonals in version: 1.2.77-0 🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Mar 2, 2023

🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.2.77-4 🚀

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
DeployBlockerCash This issue or pull request should block deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants