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

fix room header avatar overlay border radius is missing #14318

Merged
merged 10 commits into from
Jan 16, 2023

Conversation

bernhardoj
Copy link
Contributor

Details

There is a regression happens after this PR #14181 is merged. The room header avatar overlay border radius is missing. This PR solves the regression.

Fixed Issues

$ #13585
PROPOSAL:

Tests

  1. Create a group of 5 or more users
  2. Verify the 4th avatar overlay is completely rounded
  • Verify that no errors appear in the JS console

Offline tests

  1. Create a group of 5 or more users
  2. Verify the 4th avatar overlay is completely rounded

QA Steps

  1. Create a group of 5 or more users
  2. Verify the 4th avatar overlay is completely rounded
  • 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

image

Mobile Web - Chrome

328887

Mobile Web - Safari

image

Desktop

desktop

iOS

image

Android

328888

@bernhardoj bernhardoj requested a review from a team as a code owner January 15, 2023 11:47
@melvin-bot melvin-bot bot requested review from danieldoglas and removed request for a team January 15, 2023 11:47
@melvin-bot
Copy link

melvin-bot bot commented Jan 15, 2023

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

@bernhardoj
Copy link
Contributor Author

Waiting for #14317 to be merged

@mountiny
Copy link
Contributor

@bernhardoj The PR has been merged! #14317

Can you please have a look and merge main

@bernhardoj
Copy link
Contributor Author

Updating in a few minutes.

@bernhardoj
Copy link
Contributor Author

Merged with main.

@eVoloshchak
Copy link
Contributor

Testing in a couple minutes

@eVoloshchak
Copy link
Contributor

eVoloshchak commented Jan 16, 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 image
Mobile Web - Chrome

Screenshot_20230116-132549_Chrome

Mobile Web - Safari

Simulator Screen Shot - iPhone 12 - 2023-01-16 at 13 24 03

Desktop image
iOS

Simulator Screen Shot - iPhone 12 - 2023-01-16 at 13 24 54

Android

Screenshot_20230116-132527_New Expensify

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.

LGTM
@bernhardoj, thank you for the fast fix

Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

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

Thanks @bernhardoj and @eVoloshchak Going ahead and merging this since I chatted with you in the Slack thread

@mountiny mountiny merged commit a59825c into Expensify:main Jan 16, 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 699.852 ms → 721.117 ms (+21.264 ms, +3.0%)
App start runJsBundle 191.938 ms → 200.375 ms (+8.438 ms, +4.4%)
App start regularAppStart 0.021 ms → 0.018 ms (-0.003 ms, -16.1%) 🟢
App start nativeLaunch 21.219 ms → 19.933 ms (-1.285 ms, -6.1%)
Open Search Page TTI 592.616 ms → 585.000 ms (-7.616 ms, -1.3%)
Show details
Name Duration
App start TTI Baseline
Mean: 699.852 ms
Stdev: 25.105 ms (3.6%)
Runs: 642.8478190000169 655.7650620001368 658.1041210000403 663.3618299998343 664.4648989997804 671.5136879999191 679.1071739997715 680.9011280001141 685.7541359998286 689.9612980000675 691.7575289998204 694.5926979999058 696.9787829997949 698.5940849999897 701.167156000156 705.6926060002297 709.8631480000913 710.8074630000629 710.8592079998925 714.2284050001763 714.4663479998708 714.964062999934 716.2066020001657 716.3300760001875 721.6958659999073 725.432639000006 725.6138419997878 727.498091999907 729.7420270000584 736.4454379999079 740.7037630002014

Current
Mean: 721.117 ms
Stdev: 25.463 ms (3.5%)
Runs: 675.5255340002477 675.5445839995518 678.9091840004548 689.4538629995659 692.0358999995515 696.7681390000507 698.4377859998494 700.6668699998409 710.0658949995413 711.2240850003436 711.5080979997292 712.5360639998689 712.6528350003064 713.2835889998823 716.552628999576 717.0478999996558 719.2086429996416 720.4612050000578 727.6681679999456 730.221571999602 731.6968040000647 733.7115010004491 736.1209399998188 736.4536210000515 738.9535790001974 740.0650199996307 742.9455460002646 745.2177849998698 752.8689299998805 755.0959160001948 774.5771620003507 778.2559839999303
App start runJsBundle Baseline
Mean: 191.938 ms
Stdev: 14.912 ms (7.8%)
Runs: 155 168 175 176 176 178 181 181 182 182 184 188 188 188 191 191 192 193 194 195 197 199 199 199 202 205 207 208 210 211 221 226

Current
Mean: 200.375 ms
Stdev: 14.777 ms (7.4%)
Runs: 173 178 178 179 179 186 186 189 193 193 195 195 196 197 197 201 202 202 204 205 205 206 209 210 211 212 214 216 220 222 223 236
App start regularAppStart Baseline
Mean: 0.021 ms
Stdev: 0.002 ms (8.6%)
Runs: 0.018188000191003084 0.018962000031024218 0.01912400033324957 0.0195720000192523 0.019816000014543533 0.01993800001218915 0.020060999784618616 0.020142000168561935 0.020182000007480383 0.020304000005126 0.02038600016385317 0.020426999777555466 0.020792999770492315 0.021159000229090452 0.021239999681711197 0.0215659998357296 0.02172800013795495 0.02193199982866645 0.021972999908030033 0.02209499990567565 0.022135000210255384 0.022583000361919403 0.022704999893903732 0.022785999812185764 0.022908000275492668 0.023804000113159418 0.025837999768555164 0.02599999960511923

Current
Mean: 0.018 ms
Stdev: 0.001 ms (8.3%)
Runs: 0.015421000309288502 0.016195000149309635 0.0163569999858737 0.016397999599575996 0.016600999981164932 0.0166830001398921 0.016846000216901302 0.01692800037562847 0.017171000130474567 0.017211999744176865 0.017292999662458897 0.017374999821186066 0.017415999434888363 0.017455999739468098 0.017577999271452427 0.017578000202775 0.017578000202775 0.018066000193357468 0.018067000433802605 0.018106999807059765 0.0185139998793602 0.0185139998793602 0.01887999940663576 0.019042999483644962 0.019652999937534332 0.0197350000962615 0.019977999851107597 0.021077999845147133 0.02233900036662817
App start nativeLaunch Baseline
Mean: 21.219 ms
Stdev: 3.100 ms (14.6%)
Runs: 18 18 18 18 18 18 18 19 19 19 20 20 20 20 20 20 20 20 21 21 21 21 21 23 23 24 26 26 26 27 28 28

Current
Mean: 19.933 ms
Stdev: 1.236 ms (6.2%)
Runs: 17 18 18 19 19 19 19 19 19 19 20 20 20 20 20 20 20 20 20 20 20 20 21 21 21 21 21 22 22 23
Open Search Page TTI Baseline
Mean: 592.616 ms
Stdev: 22.414 ms (3.8%)
Runs: 552.0799150001258 558.3206380000338 561.4366859998554 564.9956870004535 565.0079759997316 568.1479499991983 569.8613280002028 570.93802900007 576.8462729998864 578.3573409998789 578.4527190001681 581.8646650006995 585.6309810001403 587.2896330002695 589.6278079999611 590.0794270001352 592.9434410003014 596.6869300003164 599.2087409999222 599.7791349999607 601.1305749998428 603.7190759996884 604.2444249996915 606.6173100001179 610.7923179999925 616.4182940004393 617.3872079998255 618.3814699999057 622.9421799997799 626.4718840001151 632.3398439995944 635.7022299999371

Current
Mean: 585.000 ms
Stdev: 29.857 ms (5.1%)
Runs: 541.1378990001976 546.178670999594 547.1259759999812 547.6328130001202 551.052896999754 555.5448000002652 556.6003830004483 559.4766040006652 563.3994549997151 567.6232909997925 567.6929930001497 568.4844570001587 569.1452230000868 569.4874680005014 570.2717289999127 572.687989000231 573.1044109994546 579.6137290000916 584.2180180000141 585.1519780000672 585.8240149999037 601.4925540005788 602.0649420004338 606.2721760002896 611.3719889996573 612.2759199999273 613.0953379999846 617.535279000178 629.440470000729 636.785442000255 637.2067870004103 637.2128909993917 638.7872319994494

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @mountiny in version: 1.2.55-0 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @AndrewGable in version: 1.2.55-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