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

Add a few pronoun combinations #14899

Merged
merged 4 commits into from
Feb 9, 2023
Merged

Add a few pronoun combinations #14899

merged 4 commits into from
Feb 9, 2023

Conversation

Beamanator
Copy link
Contributor

@Beamanator Beamanator commented Feb 7, 2023

cc @johnmlee101 @twisterdotcom

Holding on slack convo: https://expensify.slack.com/archives/C01GTK53T8Q/p1675757915008479?thread_ts=1666881832.702589&cid=C01GTK53T8Q

Details

Adds a few pronoun combinations in case the user feels like the previous list doesn't completely cover what they want to use.

Related convos

$ Slack: https://expensify.slack.com/archives/C01GTK53T8Q/p1675705596761979?thread_ts=1666881832.702589&cid=C01GTK53T8Q

Tests

  1. Navigate to Settings -> Profile -> Pronouns
  2. Verify the new options He / They and She / They are in the list, can be selected, show up everywhere, etc

Screenshot 2023-02-08 at 5 56 00 PM

  • Verify that no errors appear in the JS console

Offline tests

  1. Do the same as ^ while offline

QA Steps

Same as above

  • 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 Screenshot 2023-02-08 at 5 56 00 PM
Mobile Web - Chrome

Slightly outdated, but same general concept:

Screenshot 2023-02-07 at 11 57 38 AM

Mobile Web - Safari

Slightly outdated, but same general concept:

Screenshot 2023-02-07 at 11 01 49 AM

Desktop

Slightly outdated, but same general concept:

Screenshot 2023-02-07 at 10 59 32 AM
iOS

Slightly outdated, but same general concept:

Screenshot 2023-02-07 at 11 01 20 AM

Android

Slightly outdated, but same general concept:

Screenshot 2023-02-07 at 11 56 34 AM

@Beamanator Beamanator self-assigned this Feb 7, 2023
@Beamanator Beamanator requested a review from a team as a code owner February 7, 2023 08:57
@melvin-bot melvin-bot bot requested review from MariaHCD and removed request for a team February 7, 2023 08:57
@MelvinBot
Copy link

@MariaHCD Please 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]

@Beamanator Beamanator changed the title Add a few pronoun combinations [HOLD Slack convo] Add a few pronoun combinations Feb 7, 2023
@Beamanator Beamanator changed the title [HOLD Slack convo] Add a few pronoun combinations Add a few pronoun combinations Feb 8, 2023
@Beamanator
Copy link
Contributor Author

Ready for review @MariaHCD ! 👍

@MariaHCD
Copy link
Contributor

MariaHCD commented Feb 9, 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.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Screenshot 2023-02-09 at 12 36 38 PM

Mobile Web - Chrome

Screenshot 2023-02-09 at 12 50 04 PM

Mobile Web - Safari

Screenshot 2023-02-09 at 12 42 46 PM

Desktop

Screenshot 2023-02-09 at 12 55 11 PM

iOS

Screenshot 2023-02-09 at 12 44 24 PM

Android

Screenshot 2023-02-09 at 12 52 53 PM

@MariaHCD MariaHCD merged commit d9a126d into main Feb 9, 2023
@MariaHCD MariaHCD deleted the beaman-addFewComboPronouns branch February 9, 2023 08:57
@OSBotify
Copy link
Contributor

OSBotify commented Feb 9, 2023

✋ 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

github-actions bot commented Feb 9, 2023

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries
Name Duration
Open Search Page TTI 636.467 ms → 648.274 ms (+11.806 ms, +1.9%)
App start TTI 700.323 ms → 706.797 ms (+6.474 ms, +0.9%)
App start runJsBundle 196.094 ms → 198.500 ms (+2.406 ms, +1.2%)
App start nativeLaunch 19.387 ms → 20.031 ms (+0.644 ms, +3.3%)
App start regularAppStart 0.014 ms → 0.016 ms (+0.003 ms, +18.6%) 🟡
Show details
Name Duration
Open Search Page TTI Baseline
Mean: 636.467 ms
Stdev: 39.111 ms (6.1%)
Runs: 583.8903000000864 603.2159420009702 603.2379969991744 603.3101810012013 603.5530189983547 604.3002530001104 605.1171879991889 605.4183759987354 605.5649020001292 609.7455239985138 610.684529999271 614.7145189996809 614.7698570005596 614.8051350004971 619.0488700009882 621.0561529994011 621.438069999218 622.0941160004586 622.2386480011046 626.8811440002173 630.4914560001343 637.953288000077 645.4352630004287 649.209839001298 655.922322999686 662.4829510003328 671.1956789996475 674.0686040017754 677.0195719990879 694.0692960005254 723.3322750013322 733.339883999899 733.8161629997194

Current
Mean: 648.274 ms
Stdev: 32.005 ms (4.9%)
Runs: 602.1352539993823 604.1796059999615 606.719034999609 611.2364099994302 611.6895759999752 616.0483800005168 617.6468100007623 618.7470700014383 620.4653730001301 626.8331300001591 629.0192869994789 631.3309739995748 632.1478269994259 632.5174160003662 634.3118500001729 637.6001390013844 637.8856610003859 645.9031989984214 648.1745199989527 649.7961030006409 653.1055510006845 663.4782719984651 663.5686439983547 671.3506670016795 674.8264569994062 677.7456470001489 682.2733970005065 685.769206000492 691.2050369996578 691.3078620005399 701.7065030001104 709.3779710009694 712.9270430002362
App start TTI Baseline
Mean: 700.323 ms
Stdev: 38.572 ms (5.5%)
Runs: 622.1390199996531 639.2870070002973 640.3522440008819 657.0275480002165 660.8733660001308 667.0335559993982 667.0474490001798 669.845236999914 669.9328439999372 675.403273999691 678.5971159990877 685.5507389996201 690.1998459994793 693.7670210003853 695.0034779999405 696.2007679995149 700.0376869998872 704.6148119997233 706.7366129998118 707.4760839994997 708.3866109997034 711.0033979993314 715.6062979996204 728.0457559991628 728.1756960004568 738.9173460006714 745.8309160005301 746.9210269991308 751.1117199994624 754.943554000929 774.4115100000054 779.8448760006577

Current
Mean: 706.797 ms
Stdev: 27.513 ms (3.9%)
Runs: 662.5786530002952 663.3484519999474 669.6210779994726 674.0830680001527 674.1572290007025 674.6611499991268 676.9942749999464 682.5955550000072 683.6588339991868 684.6914709992707 693.2857140004635 694.0647049993277 700.0018000006676 701.2622359991074 703.6210440006107 706.5528100002557 710.3112039994448 710.6829000003636 711.3229879997671 714.3975840006024 715.5077759996057 715.7609829995781 718.4183249995112 720.8970049992204 731.9279190003872 732.664211999625 735.6076999995857 741.7835039999336 744.181298000738 745.3726659994572 758.1095490008593 765.3719350006431
App start runJsBundle Baseline
Mean: 196.094 ms
Stdev: 21.932 ms (11.2%)
Runs: 167 169 169 172 172 173 173 175 177 178 181 183 185 187 192 196 198 198 199 201 204 204 208 211 212 214 215 215 224 230 235 258

Current
Mean: 198.500 ms
Stdev: 23.210 ms (11.7%)
Runs: 153 174 174 175 178 178 179 184 184 185 185 188 189 190 190 190 191 193 193 200 202 205 209 209 210 220 223 226 228 240 252 255
App start nativeLaunch Baseline
Mean: 19.387 ms
Stdev: 1.580 ms (8.1%)
Runs: 17 18 18 18 18 18 18 18 18 18 18 18 19 19 19 19 19 19 19 20 20 20 20 20 21 21 21 22 22 23 23

Current
Mean: 20.031 ms
Stdev: 2.271 ms (11.3%)
Runs: 17 18 18 18 18 18 18 18 18 18 19 19 19 19 19 19 19 20 20 20 20 21 21 21 21 22 22 22 23 24 26 26
App start regularAppStart Baseline
Mean: 0.014 ms
Stdev: 0.001 ms (4.2%)
Runs: 0.012736000120639801 0.012816999107599258 0.012939000502228737 0.012939000502228737 0.013061000034213066 0.013183001428842545 0.013224000111222267 0.013346001505851746 0.013387000188231468 0.013508999720215797 0.013549000024795532 0.013712000101804733 0.013793999329209328 0.013793999329209328 0.0138349998742342 0.0138349998742342 0.0138349998742342 0.013916000723838806 0.013996999710798264 0.014079000800848007 0.01411999948322773 0.01411999948322773 0.014159999787807465 0.014281999319791794 0.014322999864816666 0.014322999864816666 0.0143630001693964 0.014608001336455345 0.015258999541401863

Current
Mean: 0.016 ms
Stdev: 0.001 ms (8.0%)
Runs: 0.014689000323414803 0.01469000056385994 0.014810999855399132 0.014811001718044281 0.01493300125002861 0.014973999932408333 0.015137000009417534 0.015177000313997269 0.015300000086426735 0.01534000039100647 0.015543000772595406 0.015625 0.01574700139462948 0.015786999836564064 0.015828000381588936 0.01603200100362301 0.016154000535607338 0.01631700061261654 0.016439000144600868 0.016561001539230347 0.01660199835896492 0.01664300076663494 0.016764000058174133 0.017049001529812813 0.017211999744176865 0.017374999821186066 0.01741499826312065 0.017496000975370407 0.01839200034737587 0.01855500042438507 0.019409000873565674 0.019491000100970268

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by https://github.com/MariaHCD in version: 1.2.69-0 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/mountiny in version: 1.2.69-2 🚀

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

@situchan
Copy link
Contributor

This PR crashes for the users with deprecated pronouns. i.e. He / They
#15268

@Beamanator
Copy link
Contributor Author

@situchan Feel free to explain a bit more on that issue - I don't see how this PR would cause that problem since He / They was removed in the final commit in this PR

@situchan
Copy link
Contributor

He / They was removed in the final commit in this PR

@Beamanator this is the issue.
I think this is similar case to #12795

@Beamanator
Copy link
Contributor Author

@situchan I mean I added He / They as an option in the first commit in this PR, then I removed that option in the last commit. So I don't see how He / They could have been selected unless someone tested my PR before it was finished

@situchan
Copy link
Contributor

@situchan I mean I added He / They as an option in the first commit in this PR, then I removed that option in the last commit. So I don't see how He / They could have been selected unless someone tested my PR before it was finished

Ah, you're right. Sorry for confusion.

@Beamanator
Copy link
Contributor Author

No worries at all, thanks for discussing 🙏 👍

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.

6 participants