15211: search pronouns

[gedu]

Details

Now the PronounsPage is a class component and will show a search input to filter the pronouns.

Fixed Issues

$ #15211
PROPOSAL: #15211 (comment)

Tests

1. Open the Pronouns page

2. You should see an input and no options

3. Open the Pronouns page

4. Write some letters to start seeing some options

5. Click on one and it should be applied to your profile

• Verify that no errors appear in the JS console

Offline tests

1. Open the Pronouns page
2. Write some letters to start seeing some options
3. Click on one and it should be applied to your profile only locally, you won't see it applied to other devices
4. Connect to the internet, it will sync with the server and you will see the selected pronouns on your other devices

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.
• 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

pro_chrome.mp4

Mobile Web - Chrome

pro_androidChrome.mp4

Mobile Web - Safari

pro_iosSafari.mp4

Desktop

pro_desktop.mp4

iOS

pro_ios.mp4

Android

pro_android.mp4

[MelvinBot]

@puneetlath @mollfpr 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]

[puneetlath]

@gedu are you ready for us to review? If so, I'll let @mollfpr take the first pass.

[mollfpr]

Missing JSDoc on onChangeText function.

[gedu]

@gedu are you ready for us to review? If so, I'll let @mollfpr take the first pass.

Yes, should be ready

[gedu]

Missing JSDoc on onChangeText function.

What should I add? I follow Avoid descriptions that don't add any additional information
it doesn't receive a param and doesn't return a value. I looked at SearchPage, NewChatPage, WorkspaceInvitePage and didn't have JSDoc.

Could you help me what would be the best way to Document a function like onChangeText?

[mollfpr]

Yeah, you’re right! Since we have a default value for the parameter, we’re good without the JSDoc.

[mollfpr]

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

15317.Web.mov

Mobile Web - Chrome

15317.mWeb.Chrome.mp4

Mobile Web - Safari

15317.mWeb.Safari.mp4

Desktop

15317.Desktop.mov

iOS

15317.iOS.mp4

Android

15317.Android.mp4

[mollfpr]

@puneetlath Just for clarity, if the user has selected pronouns, what should we show on the PronounsPage? Is it still with no list or showing the selected pronouns selection as default?

[puneetlath]

Good question. Maybe we should do it similar to the Timezone page. Where the input is filled with your current choice, but the text is selected so that if you start typing it is overwritten. What do you think?

Screen.Recording.2023-02-22.at.10.57.34.AM.mov

[puneetlath]

Also, we should allow selection via arrow keys. Like we do on the New Chat page.

[gedu]

Good question. Maybe we should do it similar to the Timezone page. Where the input is filled with your current choice, but the text is selected so that if you start typing it is overwritten. What do you think?

I Think this is easy to add.

Also, we should allow selection via arrow keys. Like we do on the New Chat page.

For this one I will have to look a bit more

[gedu]

@puneetlath for the input filled seems that there is a bug on iOS, it isn't selecting the text (this error happens on Timezone Page too)

Screen.Recording.2023-02-23.at.10.07.11.mov

what should I do?

[mollfpr]

@gedu Is the issue you mentioned need to be fixed separately on TimezonePage and PronounsPage? Or it can be in one fix?

[gedu]

@gedu Is the issue you mentioned need to be fixed separately on TimezonePage and PronounsPage? Or it can be in one fix?

It can be fixed together, I rollback and won't submit it

[gedu]

All should be ok now, is behaving the same way as the videos in the description

[puneetlath]

ok cool. @mollfpr can you give it the first review?

[mollfpr]

The code changes look good.

@puneetlath Do you think the bottom margin of the title and the selection is too far?

Before	After

[puneetlath]

I have terrible eyes for this. What do you think @shawnborton?

[mollfpr]

We have extra 12px vertical padding on the text input; we can add new props for the text input wrapper to eliminate the top padding.

Here's how it looks like before/after adding 0 top padding on the text input wrapper.

Before	After

[shawnborton]

For that change, can we see what it looks like when the input focused? We probably want 20px of margin under the text and above the input.

[mollfpr]

@shawnborton Here you go. It has 12px space from vertical padding of the text input.

[shawnborton]

Cool, that works for me.

[mollfpr]

@gedu I found a bug after refreshing the pronouns page; then selecting a pronoun. The selected pronoun is not checked, and the input value is not cleared.

Screen.Recording.2023-02-27.at.23.45.53.mov

[gedu]

@gedu I found a bug after refreshing the pronouns page; then selecting a pronoun. The selected pronoun is not checked, and the input value is not cleared.

I will take a look

[gedu]

@gedu I found a bug after refreshing the pronouns page; then selecting a pronoun. The selected pronoun is not checked, and the input value is not cleared.

I'm still looking at this, this bug is happening on TimezonePage too. Seems that after refreshing the page, the lifecycle of the component breaks.

@mountiny should I keep looking into this? or we can create a follow-up PR given that happens on other pages?

[mollfpr]

@gedu On the timezone select page, it's fixed on this PR. Maybe you can find something there?

[mountiny]

@gedu Lets have a look at the other PR @mollfpr linked if we can see how it was fixed there. Would be ideal to do it here too.

[gedu]

@gedu Lets have a look at the other PR @mollfpr linked if we can see how it was fixed there. Would be ideal to do it here too.

Yes, I just found a similar solution and is working, thanks for the input, will upload the fix soon

[gedu]

Screen.Recording.2023-02-28.at.12.24.23.mp4

@mollfpr

[mountiny]

@gedu couple of comments

@mollfpr will you be able to run through the checklist today 💪 ?

[mollfpr]

Thanks, @gedu for the updates! @mountiny Yup, I’ll review it again within an hour.

[puneetlath]

Code looks good to me.

[mollfpr]

LGTM, good work @gedu 👍

[github-actions]

Performance Comparison Report 📊

Significant Changes To Duration

Name	Duration
Open Search Page TTI	610.254 ms → 675.861 ms (+65.607 ms, +10.8%) 🔴

Show details

Name

Duration

Open Search Page TTI

Baseline Mean: 610.254 ms Stdev: 17.401 ms (2.9%) Runs: 582.1308190003037 587.4152020001784 591.2745770001784 591.4338379995897 592.1486409995705 592.884887999855 595.0715330000967 595.5838219998404 595.9916179999709 597.2692059995607 598.3995780004188 599.5347499996424 600.4634199999273 602.8835040004924 605.9181310003623 606.0485839992762 609.8683679997921 611.4167479993775 611.9782720003277 612.8904220005497 616.4038909999654 617.4903560001403 620.5387369999662 621.2267250008881 624.0808920003474 626.1440030001104 627.2703860001639 628.6426999997348 630.0272220000625 636.9429119993001 639.1483159996569 659.6118580000475 Current Mean: 675.861 ms Stdev: 19.687 ms (2.9%) Runs: 625.1268720002845 649.7894700001925 654.8549399999902 655.5244959993288 657.8339439993724 658.0897220000625 658.5771490000188 661.5378819992766 663.5422369996086 664.7216390008107 666.4169920003042 666.4192299991846 669.0918379994109 670.7983410004526 671.6773680001497 672.0584720000625 674.1739509999752 674.5319830002263 674.7518319999799 675.0381680000573 682.1088059991598 683.8211669996381 688.1047369996086 690.9702559998259 691.6885579992086 691.9704999998212 695.2057699989527 701.5360509995371 702.8662930000573 710.4482419993728 710.612182000652 713.6558839995414

Meaningless Changes To Duration

Show entries

Name	Duration
App start TTI	711.839 ms → 716.272 ms (+4.434 ms, +0.6%)
App start regularAppStart	0.014 ms → 0.015 ms (+0.001 ms, +7.3%)
App start nativeLaunch	20.969 ms → 19.321 ms (-1.647 ms, -7.9%)
App start runJsBundle	194.645 ms → 189.875 ms (-4.770 ms, -2.5%)

Show details

Name	Duration
App start TTI	Baseline Mean: 711.839 ms Stdev: 29.310 ms (4.1%) Runs: 645.4602089999244 679.8221819996834 681.7685510003939 685.273683000356 687.802493000403 688.7650739997625 689.5833470001817 690.6180450003594 692.0804780004546 694.4498039996251 700.216539000161 700.2450270000845 702.805486000143 703.3765979995951 705.5942329997197 707.9916080003604 708.9295720001683 710.3889039997011 712.6947560003027 712.8932790001854 719.0880330000073 720.4029240002856 723.1654700003564 726.617886999622 730.8121020002291 731.6055319998413 732.4264000002295 754.8599619995803 755.7917480003089 784.9160609999672 786.5508749997243 Current Mean: 716.272 ms Stdev: 22.523 ms (3.1%) Runs: 669.9036640003324 680.6030449997634 684.4412179999053 687.882784999907 688.1856620004401 699.6838999995962 700.5947939995676 702.5999450003728 706.8997969999909 708.2643020004034 708.4569730004296 709.4951290003955 709.5470390003175 709.9748109998181 710.6454729996622 710.6502630002797 712.5782559998333 718.0231170002371 718.1789429998025 719.5646879998967 726.7621969999745 730.3610579995438 730.807915000245 732.140296000056 733.7306159995496 736.8391190003604 740.2253529997543 745.3759430004284 746.1177570000291 749.1753500001505 776.7345810001716
App start regularAppStart	Baseline Mean: 0.014 ms Stdev: 0.001 ms (5.0%) Runs: 0.012165999971330166 0.012246999889612198 0.0125730000436306 0.0125730000436306 0.012695999816060066 0.012777000665664673 0.013061000034213066 0.013062000274658203 0.0131029998883605 0.01314299926161766 0.01314299926161766 0.013182999566197395 0.0134680001065135 0.013671999797224998 0.013712999410927296 0.01375299971550703 0.0138349998742342 0.013875000178813934 0.013875999487936497 0.013915999792516232 0.013956000097095966 0.013956000097095966 0.013957000337541103 0.014038000255823135 0.014038000255823135 0.014077999629080296 0.014161000028252602 0.014240999706089497 0.014322999864816666 0.0143630001693964 0.014364000409841537 0.015096000395715237 Current Mean: 0.015 ms Stdev: 0.001 ms (7.7%) Runs: 0.012898999266326427 0.013020999729633331 0.013020999729633331 0.013224000111222267 0.013387000188231468 0.01338799949735403 0.0134680001065135 0.01355000026524067 0.013590999878942966 0.013631000183522701 0.013915999792516232 0.013997000642120838 0.0139979999512434 0.014200999401509762 0.014364000409841537 0.014607999473810196 0.01464799977838993 0.0147299999371171 0.0147299999371171 0.014770000241696835 0.014810999855399132 0.014973999932408333 0.014973999932408333 0.015014000236988068 0.015461999922990799 0.015543999150395393 0.015544000081717968 0.015991000458598137 0.01607299968600273 0.016397999599575996 0.01696700043976307 0.0170889999717474
App start nativeLaunch	Baseline Mean: 20.969 ms Stdev: 2.710 ms (12.9%) Runs: 17 17 17 18 18 18 18 19 19 19 20 20 20 20 20 21 21 21 21 21 22 22 22 23 23 23 23 24 24 26 27 27 Current Mean: 19.321 ms Stdev: 1.283 ms (6.6%) Runs: 18 18 18 18 18 18 18 19 19 19 19 19 19 19 19 19 19 19 19 20 20 20 20 20 21 21 21 24
App start runJsBundle	Baseline Mean: 194.645 ms Stdev: 19.044 ms (9.8%) Runs: 163 172 173 175 179 179 180 180 182 183 183 185 187 188 190 191 191 194 194 196 199 200 201 209 211 213 213 219 221 228 255 Current Mean: 189.875 ms Stdev: 12.567 ms (6.6%) Runs: 165 171 173 174 175 181 181 181 182 183 183 184 186 186 187 188 188 189 191 192 193 193 195 199 200 200 201 203 207 213 213 219

[github-actions]

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

[mountiny]

This PR did not touch the Seaech page so this performance evaluation seems off

[OSBotify]

🚀 Deployed to staging by https://github.com/puneetlath in version: 1.2.78-0 🚀

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

[OSBotify]

🚀 Deployed to production by https://github.com/mountiny in version: 1.2.78-0 🚀

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