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

Allow individual options to have bold style #14176

Merged
merged 1 commit into from
Jan 11, 2023

Conversation

Beamanator
Copy link
Contributor

@Beamanator Beamanator commented Jan 10, 2023

cc @tgolen since it looks like this was missed in your cleanup PR (that I helped review 🙃 )

Details

Regression from #13319

Fixed Issues

$ #14154

Tests

  1. Navigate to Settings -> Profile -> Pronouns
  2. Verify your current selected pronoun is in bold

Screenshot 2023-01-10 at 3 28 30 PM

  1. Navigate to Settings -> Profile -> Timezone
  2. Make sure you're not automatically updating timezone, then click "Timezone"
  3. Verify your current selected timezone is in bold

Screenshot 2023-01-10 at 3 30 37 PM

Update each and make sure the newly selected items are now bold and the previously selected items are no longer bold

  • Verify that no errors appear in the JS console

Offline tests

Same steps above

QA Steps

Same as all above steps

  • 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
    • 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-01-10 at 3 28 30 PM
Mobile Web - Chrome Screenshot 2023-01-10 at 3 49 47 PM
Mobile Web - Safari Screenshot 2023-01-10 at 3 57 01 PM
Desktop Screenshot 2023-01-10 at 3 57 53 PM
iOS Screenshot 2023-01-10 at 3 55 56 PM
Android Screenshot 2023-01-10 at 3 43 53 PM

@Beamanator Beamanator self-assigned this Jan 10, 2023
@Beamanator Beamanator marked this pull request as ready for review January 10, 2023 13:58
@Beamanator Beamanator requested a review from a team as a code owner January 10, 2023 13:58
@melvin-bot melvin-bot bot requested review from sobitneupane and techievivek and removed request for a team January 10, 2023 13:58
@melvin-bot
Copy link

melvin-bot bot commented Jan 10, 2023

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

@sobitneupane
Copy link
Contributor

Reviewing.

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.
  • 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-01-10 at 20 37 26 Screenshot 2023-01-10 at 20 37 10
Mobile Web - Chrome Screenshot 2023-01-11 at 3 04 23 PM Screenshot 2023-01-11 at 3 03 20 PM
Mobile Web - Safari
Desktop Screenshot 2023-01-10 at 20 50 15 Screenshot 2023-01-10 at 20 50 04
iOS
Android

Copy link
Contributor

@techievivek techievivek left a comment

Choose a reason for hiding this comment

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

Looks good and tests well.
@sobitneupane you had Mobile Web - Chrome screenshots missing from your checklist, I have added them for now.

@techievivek techievivek merged commit 0f744b3 into main Jan 11, 2023
@techievivek techievivek deleted the beaman-allowIndividualOptionsToBeBold branch January 11, 2023 09:36
@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.

@sobitneupane
Copy link
Contributor

Thanks @techievivek.

@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 701.190 ms → 718.291 ms (+17.101 ms, +2.4%)
App start nativeLaunch 10.935 ms → 21.031 ms (+10.096 ms, +92.3%) 🟡
App start runJsBundle 197.258 ms → 198.188 ms (+0.929 ms, ±0.0%)
App start regularAppStart 0.014 ms → 0.022 ms (+0.008 ms, +58.9%) 🟡
Open Search Page TTI 600.192 ms → 593.964 ms (-6.228 ms, -1.0%)
Show details
Name Duration
App start TTI Baseline
Mean: 701.190 ms
Stdev: 26.507 ms (3.8%)
Runs: 655.4143209997565 657.9889839999378 666.2005249997601 666.9846930000931 668.3405609996989 674.21228899993 674.4636199995875 681.1680969996378 685.3493290003389 686.2674139998853 687.8444609995931 688.7577470000833 693.6405389998108 694.0588959995657 698.2936220001429 699.740114999935 700.0573399998248 700.6665669996291 701.4111339999363 703.1285269996151 703.2956550000235 714.5200129998848 716.2866449998692 719.3735809996724 722.2349169999361 728.4400930004194 728.8556930003688 736.4530539996922 736.9424949996173 742.1769279995933 747.1102790003642 758.401936000213

Current
Mean: 718.291 ms
Stdev: 24.221 ms (3.4%)
Runs: 667.647754999809 667.8544619996101 677.8891249997541 694.7653989996761 696.2720459997654 699.348384000361 699.7103739995509 700.7865930004045 703.2923469999805 705.1025890000165 710.460904000327 711.5343469996005 711.7921780003235 713.071614000015 717.7079100003466 718.3994709998369 723.1534190000966 723.7562469998375 724.5283460002393 726.7649490004405 728.2065540002659 730.1144589995965 730.7196389995515 732.1572289997712 735.6302629997954 737.9175079995766 744.909129999578 745.4663690002635 758.6201719995588 764.1750090001151 765.2775999996811
App start nativeLaunch Baseline
Mean: 10.935 ms
Stdev: 2.449 ms (22.4%)
Runs: 8 8 8 9 9 9 9 9 9 9 9 10 10 10 10 10 10 11 11 11 11 12 12 12 13 13 13 15 15 16 18

Current
Mean: 21.031 ms
Stdev: 2.531 ms (12.0%)
Runs: 18 18 18 18 19 19 19 19 19 19 19 19 20 20 20 20 20 21 21 21 22 22 22 22 22 24 24 25 25 25 26 27
App start runJsBundle Baseline
Mean: 197.258 ms
Stdev: 17.861 ms (9.1%)
Runs: 162 168 173 174 174 178 181 183 189 190 191 192 193 193 199 199 199 201 202 203 203 205 207 209 209 213 216 217 220 234 238

Current
Mean: 198.188 ms
Stdev: 14.384 ms (7.3%)
Runs: 166 170 173 177 186 186 187 187 188 188 192 195 196 196 199 199 200 203 203 204 204 205 205 206 210 212 213 214 218 219 220 221
App start regularAppStart Baseline
Mean: 0.014 ms
Stdev: 0.001 ms (7.1%)
Runs: 0.012246999889612198 0.012532000429928303 0.01253300067037344 0.012613999657332897 0.012736000120639801 0.012736000120639801 0.012776000425219536 0.0134680001065135 0.013549000024795532 0.013590999878942966 0.013632000423967838 0.0138349998742342 0.013915999792516232 0.013915999792516232 0.013957000337541103 0.01403799932450056 0.014079000800848007 0.014159999787807465 0.014200999401509762 0.014241000637412071 0.014241999946534634 0.014648000709712505 0.014648999087512493 0.014770000241696835 0.014852000400424004 0.0148930000141263 0.015056000091135502 0.015421000309288502 0.015502999536693096 0.015868999995291233 0.016032000072300434

Current
Mean: 0.022 ms
Stdev: 0.002 ms (10.6%)
Runs: 0.01802500057965517 0.019449999555945396 0.019613000564277172 0.020223000086843967 0.02026400063186884 0.020874000154435635 0.020915000699460506 0.020955000072717667 0.021200000308454037 0.021443000063300133 0.021564999595284462 0.02164699975401163 0.021688000299036503 0.021688000299036503 0.021769000217318535 0.021850000135600567 0.021932000294327736 0.022094999440014362 0.02254199981689453 0.022664999589323997 0.022704999893903732 0.02270600013434887 0.022827000357210636 0.0235190000385046 0.024577000178396702 0.024658000096678734 0.02469899971038103 0.028768000192940235 0.029337999410927296
Open Search Page TTI Baseline
Mean: 600.192 ms
Stdev: 34.835 ms (5.8%)
Runs: 554.3330889996141 555.7748210001737 559.6122229993343 560.7362470002845 563.5427649999037 564.1909590000287 566.3609220003709 567.3000900000334 567.5510249994695 568.260214000009 570.2797039998695 577.6051839999855 585.1995040001348 586.7133379997686 592.7281499998644 593.1515709999949 601.6280110003427 602.400512999855 603.2152109993622 608.1385909998789 610.3151460001245 612.4827479999512 613.1230879994109 614.0667319996282 615.4806319996715 619.9922690000385 636.3602289995179 637.2578529994935 637.5544030005112 647.1512449998409 650.4698900002986 666.0309660006315 697.3200280005112

Current
Mean: 593.964 ms
Stdev: 23.580 ms (4.0%)
Runs: 552.2121590003371 554.3401700006798 555.5255950000137 556.1588550005108 559.7211509998888 564.6499429997057 569.3832189999521 570.6313070002943 572.3582359999418 577.981241999194 579.5483400002122 580.0658369995654 587.684529999271 590.7824710002169 595.2532139997929 598.7165530007333 600.8021239992231 604.6888020001352 605.3319100001827 606.4045010004193 608.009766000323 609.3380939997733 610.3170169992372 610.6088060000911 612.2684740005061 612.2741299998015 617.2013349998742 618.6660159993917 623.119425999932 623.2308359993622 624.2771409992129 624.4234219994396 624.8444830002263

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @techievivek in version: 1.2.53-0 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @Julesssss in version: 1.2.53-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.

4 participants