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 "LHN - Workspace badge is not fully visible/missing for some Room chats" #15463

Merged

Conversation

situchan
Copy link
Contributor

@situchan situchan commented Feb 23, 2023

Details

  • set flexShrink to display name
  • remove flexShrink from workspace name (badge)
  • set maxWidth (180px) to badge

Fixed Issues

$ #14491
PROPOSAL: #14491 (comment)

Tests

Same as QA step

  • Verify that no errors appear in the JS console

Offline tests

Same as QA step

QA Steps

  1. Login with any account
  2. Create new workspace if not exists
  3. Update workspace name to "PR testing"
  4. Go to Create new room page
  5. Put "00--00--00--00--00--00--00--00--00" on room name
  6. Select workspace - "PR testing"
  7. Save room and go back to LHN
  8. Verify that workspace (PR testing) badge is fully visible on "00--00--00--00--00--00--00--00--00" room
  • 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 web1 web2
Mobile Web - Chrome mchrome1 mchrome2
Mobile Web - Safari

msafari11
msafari12
msafari2

Desktop desktop1 desktop2
iOS

ios1
ios2

Android

android1
android2

@situchan situchan requested a review from a team as a code owner February 23, 2023 23:22
@melvin-bot melvin-bot bot requested review from aimane-chnaif and grgia and removed request for a team February 23, 2023 23:22
@MelvinBot
Copy link

@grgia @aimane-chnaif 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]

src/styles/styles.js Outdated Show resolved Hide resolved
Copy link
Contributor

@grgia grgia 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 to me, @aimane-chnaif let me know if you see anything I missed while testing

@aimane-chnaif
Copy link
Contributor

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 web-focus web-recent
Mobile Web - Chrome

mchrome-focus
mchrome-recent

Mobile Web - Safari

msafari-focus
msafari-recent

Desktop desktop-focus desktop-recent
iOS

ios-focus
ios-recent

Android

android-focus
android-recent

Copy link
Contributor

@aimane-chnaif aimane-chnaif left a comment

Choose a reason for hiding this comment

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

@grgia looks good to me too. Tested all possible edge cases.

Just these concerns as we discussed already:
ios-recent

@grgia
Copy link
Contributor

grgia commented Feb 24, 2023

@shawnborton given the pills might be removed later on, are we good to merge this as is, or would you prefer we fix that spacing?

@shawnborton
Copy link
Contributor

I think this is probably fine for now, especially given that the threads pre-designs can change how we display LHN rows and thus we might end up changing/removing the badges. So yeah, let's get this merged!

@situchan
Copy link
Contributor Author

@grgia @aimane-chnaif now that all are approved and confirmed, can we merge?

@grgia
Copy link
Contributor

grgia commented Feb 27, 2023

Yep, I'll merge once that Jest test that didn't run finishes up :)

@situchan
Copy link
Contributor Author

It seems job3 is not completing recently for most PRs
#15370

@grgia
Copy link
Contributor

grgia commented Feb 28, 2023

@situchan I'm not sure what's going on with that Jest test, would you mind merging main?

@grgia
Copy link
Contributor

grgia commented Feb 28, 2023

Oops I apologize, just saw the link you sent! Let me look into it

@situchan
Copy link
Contributor Author

I think no problem merging main since that jest issue is nothing related with this PR.

@situchan
Copy link
Contributor Author

Any action needed on our side to merge?

@grgia grgia merged commit ef46d29 into Expensify:main Feb 28, 2023
@melvin-bot melvin-bot bot added the Emergency label Feb 28, 2023
@MelvinBot
Copy link

@grgia looks like this was merged without a test passing. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

@grgia grgia removed the Emergency label Feb 28, 2023
@grgia
Copy link
Contributor

grgia commented Feb 28, 2023

Not an emergency, Jest 3 Test is unrelated to this PR and a known issue

@github-actions
Copy link
Contributor

Performance Comparison Report 📊

Significant Changes To Duration

Name Duration
Open Search Page TTI 616.474 ms → 672.241 ms (+55.767 ms, +9.0%) 🔴
Show details
Name Duration
Open Search Page TTI Baseline
Mean: 616.474 ms
Stdev: 25.190 ms (4.1%)
Runs: 576.9514570003375 580.3572599999607 581.6546219997108 585.4949550004676 588.137532999739 594.9985349997878 596.6779379993677 599.8297529993579 600.271077000536 600.5714120008051 602.2634689994156 603.8941239994019 606.8490810003132 607.6510010007769 611.1562099996954 612.0404460001737 613.3563240002841 613.5012210002169 621.2083329996094 621.4287519995123 622.859211999923 626.0635990006849 628.3472090000287 629.6324869999662 630.3424079995602 631.1713460003957 634.578735999763 638.3743080003187 655.863078000024 662.7288819998503 667.5753180002794 681.3506269995123

Current
Mean: 672.241 ms
Stdev: 16.040 ms (2.4%)
Runs: 631.099324000068 638.0493169995025 645.8165689995512 658.8176680002362 660.6338299997151 661.359742000699 661.3608809998259 663.1980800004676 664.2148040002212 664.803101000376 667.1315520005301 667.9606530005112 668.7403569994494 668.7701820004731 669.3059899993241 669.6738689998165 674.6418460002169 676.9848230006173 677.1301680002362 678.0238849995658 679.7187909996137 680.5869950000197 681.1411129999906 683.7960609998554 685.4265959998593 688.9275309992954 689.9805509997532 690.272135999985 690.3211270002648 693.5571290003136 708.0351560004056

Meaningless Changes To Duration

Show entries
Name Duration
App start TTI 736.889 ms → 752.715 ms (+15.826 ms, +2.1%)
App start runJsBundle 197.400 ms → 208.258 ms (+10.858 ms, +5.5%)
App start regularAppStart 0.015 ms → 0.015 ms (+0.001 ms, +4.4%)
App start nativeLaunch 20.156 ms → 19.867 ms (-0.290 ms, -1.4%)
Show details
Name Duration
App start TTI Baseline
Mean: 736.889 ms
Stdev: 36.961 ms (5.0%)
Runs: 657.7726950002834 694.5767810000107 695.0082109998912 697.8060440002009 699.6159870000556 702.5699829999357 710.0605499995872 713.3230299996212 714.9508239999413 716.1206989996135 717.4560479996726 718.6011960003525 719.3954090001062 722.8385640000924 723.1798930000514 723.7834050003439 728.902889999561 732.0108890002593 734.0253779999912 737.2913870001212 746.877152999863 755.5348709998652 757.9673119997606 760.3717510001734 767.8335260003805 768.8524930002168 777.1565859997645 779.2319830004126 783.6754080001265 789.1809029998258 791.8971370002255 842.5919749997556

Current
Mean: 752.715 ms
Stdev: 30.537 ms (4.1%)
Runs: 705.0951699996367 706.6866560000926 711.6720399996266 712.0282800002024 719.5993330003694 719.6255550002679 720.2831309996545 726.5324210003018 727.3346579996869 729.2468499997631 736.9818590003997 737.8172169998288 739.2510519996285 744.0641689999029 744.1579670002684 748.3026980003342 750.0189399998635 750.4572489997372 753.8713400000706 755.7939320001751 769.4465629998595 770.9121019998565 780.5211650002748 780.6665500001982 782.3456020001322 784.5442700004205 786.8299939995632 787.3065339997411 792.443295000121 796.5306789996102 802.6812460003421 813.8420850001276
App start runJsBundle Baseline
Mean: 197.400 ms
Stdev: 16.578 ms (8.4%)
Runs: 167 171 177 178 179 181 183 185 188 191 194 194 194 195 195 196 197 198 201 202 202 205 205 208 209 210 224 228 232 233

Current
Mean: 208.258 ms
Stdev: 17.423 ms (8.4%)
Runs: 174 178 179 189 190 190 194 194 197 199 200 203 207 208 208 208 209 210 211 213 216 218 220 221 221 222 224 236 236 239 242
App start regularAppStart Baseline
Mean: 0.015 ms
Stdev: 0.001 ms (4.7%)
Runs: 0.013469000346958637 0.013712999410927296 0.013875000178813934 0.013915999792516232 0.013956000097095966 0.013957000337541103 0.013957000337541103 0.013996999710798264 0.01403799932450056 0.014159999787807465 0.014240999706089497 0.01444500032812357 0.01448499970138073 0.014566999860107899 0.014607999473810196 0.01464799977838993 0.014689000323414803 0.014728999696671963 0.0147299999371171 0.015054999850690365 0.015137000009417534 0.015177999623119831 0.015381000004708767 0.015422000549733639 0.015422000549733639 0.015461999922990799 0.015502999536693096 0.015625 0.015706000849604607 0.016073000617325306

Current
Mean: 0.015 ms
Stdev: 0.001 ms (6.5%)
Runs: 0.013061999343335629 0.0138349998742342 0.013875000178813934 0.013996999710798264 0.01428299956023693 0.014322999864816666 0.014525999315083027 0.014689999632537365 0.0148930000141263 0.0148930000141263 0.014933000318706036 0.014973999932408333 0.015014000236988068 0.015095999464392662 0.015095999464392662 0.015095999464392662 0.015258999541401863 0.01534000039100647 0.015381000004708767 0.015420999377965927 0.015544000081717968 0.015584999695420265 0.015665000304579735 0.015786999836564064 0.016032000072300434 0.01623500045388937 0.016397999599575996 0.016886000521481037 0.0168869998306036 0.016927000135183334 0.01696700043976307 0.017171000130474567
App start nativeLaunch Baseline
Mean: 20.156 ms
Stdev: 2.451 ms (12.2%)
Runs: 17 18 18 18 18 18 18 18 18 18 19 19 19 19 19 19 19 19 20 20 20 21 21 21 22 22 23 23 25 25 25 26

Current
Mean: 19.867 ms
Stdev: 1.962 ms (9.9%)
Runs: 18 18 18 18 18 18 18 19 19 19 19 19 19 19 19 19 19 19 20 20 20 20 20 21 22 22 23 23 25 25

@github-actions github-actions bot added the DeployBlockerCash This issue or pull request should block deployment label Feb 28, 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

OSBotify commented Mar 2, 2023

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

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

@OSBotify
Copy link
Contributor

OSBotify commented Mar 6, 2023

🚀 Deployed to production by https://github.com/mountiny in version: 1.2.78-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
DeployBlockerCash This issue or pull request should block deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants