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

Revert 'fix: Timezone changes to some 1st option in the list while pressing the enter key' #14826

Merged
merged 1 commit into from
Feb 3, 2023

Conversation

thienlnam
Copy link
Contributor

@thienlnam thienlnam commented Feb 3, 2023

Details

Fixed Issues

$ GH_LINK
PROPOSAL: GH_LINK_ISSUE(COMMENT)

Straight revert of https://github.com/Expensify/App/pull/14466/files to fix a couple deploy blockers

#14781
#14784

Tests

  • Verify that no errors appear in the JS console

Offline tests

QA 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
      • 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
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android

@thienlnam thienlnam self-assigned this Feb 3, 2023
@thienlnam thienlnam requested a review from a team as a code owner February 3, 2023 22:10
@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2023

⚠️ ⚠️ Heads up! This pull request has the CP Staging label ⚠️ ⚠️
If you applied the CP Staging label before the PR was merged, the PR will be be immediately deployed to staging even if the open StagingDeployCash deploy checklist is locked.
However if you applied the CP Staging after the PR was merged it's possible it won't be CP'ed automatically. If you need it to be CP'ed to staging, tag a member of @Expensify/mobile-deployers to CP it manually, otherwise you can wait for it to go out with the next deploy.

@melvin-bot melvin-bot bot requested review from techievivek and removed request for a team February 3, 2023 22:11
@melvin-bot
Copy link

melvin-bot bot commented Feb 3, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Feb 3, 2023

❗ Please, do not use Github auto-linking keywords such as these: close, closes, closed, fix, fixes, fixed, resolve, resolves or resolved.

For more details, see the Contributing Guidelines, specifically Submit your pull request for a final review 📖.

@thienlnam thienlnam requested a review from deetergp February 3, 2023 22:15
@grgia grgia self-requested a review February 3, 2023 22:48
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.

I don't think this fully fixes the linked issue #14781 for New Group Page
image
But good for invite members page
image

Although both look correct on staging to me? Let me know if I'm reading this wrong!

And I tested and it looks like #14784 is solved by this revert

Screen.Recording.2023-02-03.at.3.00.55.PM.mov

@thienlnam
Copy link
Contributor Author

thienlnam commented Feb 3, 2023

Although both look correct on staging to me? Let me know if I'm reading this wrong!

Well crap, just tested staging and you're right I don't run into that anymore

But I do on this branch.... but reverting it fixes another deploy blocker...

@thienlnam
Copy link
Contributor Author

It seems to have to do with the mouse hover though, if I go over it with my mouse it clears up

@deetergp deetergp merged commit abd4d80 into main Feb 3, 2023
@deetergp deetergp deleted the jack-revertOPtionsListChanges branch February 3, 2023 23:27
OSBotify pushed a commit that referenced this pull request Feb 3, 2023
Revert 'fix: Timezone changes to some 1st option in the list while pressing the enter key'

(cherry picked from commit abd4d80)
OSBotify added a commit that referenced this pull request Feb 3, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 4, 2023

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries
Name Duration
App start TTI 687.454 ms → 710.636 ms (+23.182 ms, +3.4%)
App start runJsBundle 188.469 ms → 197.063 ms (+8.594 ms, +4.6%)
Open Search Page TTI 600.452 ms → 608.829 ms (+8.377 ms, +1.4%)
App start regularAppStart 0.014 ms → 0.015 ms (+0.001 ms, +5.5%)
App start nativeLaunch 20.625 ms → 19.419 ms (-1.206 ms, -5.8%)
Show details
Name Duration
App start TTI Baseline
Mean: 687.454 ms
Stdev: 28.536 ms (4.2%)
Runs: 643.8405550001189 645.2395629999228 647.7851700000465 653.7898800000548 658.734261999838 660.4257479999214 661.4706569998525 663.3664029999636 667.109672000166 670.9367039999925 672.1080950000323 672.7511419998482 673.8816550001502 675.324808999896 677.4042320000008 677.4683260000311 688.3768879999407 690.5956230000593 691.1458890000358 691.2994719999842 695.952056999784 698.4763460000977 700.6225640000775 705.9550350001082 706.3793790000491 710.4419909999706 711.8457619999535 719.4765590000898 733.527546999976 738.3089430001564 741.3229490001686 753.1720890002325

Current
Mean: 710.636 ms
Stdev: 32.365 ms (4.6%)
Runs: 651.5377520001493 654.139940999914 666.6057759998366 670.1945259999484 671.2542460002005 682.7885590000078 682.8505640001968 682.9364220001735 685.3589110001922 690.0536279999651 694.0825160001405 698.8654570002109 699.1044580000453 700.7992099998519 704.5841270000674 707.2913230000995 708.4153419998474 708.4496300001629 713.9884449997917 718.9374219998717 724.2033150000498 724.8173940000124 731.0027600000612 736.0524090002291 736.1374229998328 745.9706859998405 748.455351000186 750.6782889999449 753.2487739999779 754.2215339997783 769.4282320002094 773.897491000127
App start runJsBundle Baseline
Mean: 188.469 ms
Stdev: 18.865 ms (10.0%)
Runs: 155 167 169 169 171 171 176 176 177 177 177 178 178 179 181 182 183 184 185 185 188 191 193 202 207 208 214 215 218 219 224 232

Current
Mean: 197.063 ms
Stdev: 21.643 ms (11.0%)
Runs: 167 169 169 173 174 174 176 177 180 181 182 183 185 187 189 199 199 200 202 204 204 205 206 207 208 213 216 224 233 235 238 247
Open Search Page TTI Baseline
Mean: 600.452 ms
Stdev: 20.018 ms (3.3%)
Runs: 571.9127199999057 572.1188560002483 572.4148360001855 575.9197189998813 578.2145589999855 578.8572179996409 585.8561599999666 586.125489000231 587.5798349999823 588.2198079996742 588.6796059999615 590.0684409998357 591.8292229999788 592.0847579999827 592.5607100003399 594.7113040001132 597.6279299999587 598.974934999831 600.0718999998644 603.6550300000235 604.9393319999799 608.156534999609 608.3905850001611 610.4950769999996 614.8707690001465 615.6313069998287 616.9899909999222 618.8311360003427 620.0099280001596 621.4999589999206 623.3086760002188 648.2475189999677 656.0637620002963

Current
Mean: 608.829 ms
Stdev: 24.574 ms (4.0%)
Runs: 566.2821450000629 574.6461590002291 577.7384440000169 580.009521999862 581.3590090000071 587.6632079998963 589.8669440001249 593.9075120002963 594.5601399997249 596.2790529998019 596.605550000444 597.9783930000849 599.2517909999005 600.006510999985 601.140788000077 601.7078449996188 602.9761559995823 603.5386560000479 603.6988120004535 606.3568529998884 609.6921799997799 616.0770680001006 616.1490079998039 617.4169109999202 618.4641929999925 619.7804769999348 634.996907999739 637.5805260003544 637.7646889998578 645.6519369999878 650.0375159997493 663.7165529998019 668.4657800002024
App start regularAppStart Baseline
Mean: 0.014 ms
Stdev: 0.001 ms (5.0%)
Runs: 0.012655000202357769 0.012938999570906162 0.013143000192940235 0.0133050000295043 0.013549999799579382 0.013712999876588583 0.013753000181168318 0.013793999794870615 0.013956999871879816 0.013957000337541103 0.01407900033518672 0.014119999948889017 0.014159999787807465 0.014282000251114368 0.014322000090032816 0.014322999864816666 0.014405000023543835 0.014566999860107899 0.014566999860107899 0.014567000325769186 0.01464799977838993 0.014648000244051218 0.014649000018835068 0.01472900016233325 0.014851999934762716 0.014851999934762716 0.015056000091135502 0.015176999848335981 0.015461999922990799 0.01590899983420968

Current
Mean: 0.015 ms
Stdev: 0.001 ms (7.4%)
Runs: 0.013264999724924564 0.013549999799579382 0.013794000260531902 0.013916000258177519 0.014038999564945698 0.014078999869525433 0.014078999869525433 0.014159999787807465 0.014159999787807465 0.014282000251114368 0.01444500032812357 0.014485999941825867 0.014485999941825867 0.014526000246405602 0.014649000018835068 0.014649000018835068 0.0147299999371171 0.014770000241696835 0.014771000016480684 0.015217999927699566 0.015300000086426735 0.015503000002354383 0.01599099999293685 0.016032000072300434 0.01607199991121888 0.016234999988228083 0.01631700014695525 0.016398999840021133 0.016600999981164932 0.01660200022161007 0.01688600005581975 0.017985000275075436
App start nativeLaunch Baseline
Mean: 20.625 ms
Stdev: 2.870 ms (13.9%)
Runs: 17 18 18 18 18 18 18 18 18 19 19 19 19 19 19 20 20 20 20 21 21 21 21 23 23 23 23 23 25 25 27 29

Current
Mean: 19.419 ms
Stdev: 1.540 ms (7.9%)
Runs: 17 18 18 18 18 18 18 18 18 18 19 19 19 19 19 19 19 19 19 20 20 20 20 20 21 21 21 21 21 23 24

@OSBotify
Copy link
Contributor

OSBotify commented Feb 4, 2023

🚀 Deployed to production by https://github.com/thienlnam in version: 1.2.64-7 🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Feb 4, 2023

🚀 Cherry-picked to staging by https://github.com/deetergp in version: 1.2.64-7 🚀

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

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

@OSBotify
Copy link
Contributor

OSBotify commented Feb 4, 2023

🚀 Deployed to production by https://github.com/thienlnam in version: 1.2.64-7 🚀

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

1 similar comment
@OSBotify
Copy link
Contributor

OSBotify commented Feb 4, 2023

🚀 Deployed to production by https://github.com/thienlnam in version: 1.2.64-7 🚀

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

@OSBotify
Copy link
Contributor

🚀 Cherry-picked to staging by https://github.com/AndrewGable in version: 1.3.28-2 🚀

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

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/AndrewGable in version: 1.3.28-5 🚀

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