remove workaround introduced in #2180 after the proper fix from #2727 #9143

lbaldy · 2022-05-24T07:39:25Z

Details

This change reverts the workaround implemented through #2180
In favour of a fix coming from #2727.

Fixed Issues

Tests

iPad Mini tests:

Reproduction + Fix test video (Test 1): https://youtu.be/jyzoNHLYHPo
Reproduction + Fix test video (Test 2): https://youtu.be/CLimE-Fba-g

Test 1:

Launch app in portrait, open chat - no sidebar visible.
Switch to landscape - sidebar shows.
Put app to background.
Put app back to foreground - make sure the side menu doesn't flicker.

Test 2:

Launch app in portrait, open chat - no sidebar visible.
Switch to landscape - sidebar shows.
Put app to background. Switch orientation back to portrait.
Put app back to foreground - make sure the side menu hides again as it should be in portrait.

iPad Pro tests:

Reproduction + Fix test video (Test 3, Test 4): https://youtu.be/EJkUUQCiLRg

iPad mini test 1 applies.

Scenario 2 does not as the screen is too wide in both orientations and iPad pro shows sidebar always.

Test 3:

launch the app.
Make sure you're in landscape mode.
See split screen with some other app. Make sure the side bar is visible.
Play with the size of the view, resize it a bit. When the view shrinks it should hide the sidebar, when it grows it should show it.
Move the app to background and back to foreground, please observe there are no flickers.

Test 4:

Launch the app.
Make sure you're in landscape mode.
Make the multitasking view and make Expensify app a slide over app.
Move back to fullscreen/split screen. Make sure the menu is shown accordingly
Move the app to background and back to foreground, please observe there are no flickers.

iPhone:

Non reg with and without the fix video: https://youtu.be/kuv9in8vtbk

Please perform standard smoke tests on transformation changes.

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed

QA Steps

Verify that no errors appear in the JS console

Screenshots

Web

desktop-chrome-working.mov

Mobile Web - Chrome

android-chrome.mov

Mobile Web - Safari

safari-ios-working-480.mov

Desktop

desktop-working.mov

iOS

fix.mov

ios-works.mov

Android

android-working.mov

Screenshots

github-actions · 2022-05-24T07:39:45Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

lbaldy · 2022-05-24T07:42:48Z

I have read the CLA Document and I hereby sign the CLA

parasharrajat · 2022-09-26T13:09:53Z

Will review shortly.

First of all, thank you @lbaldy for being patient about this issue. We agree that this took relatively longer time.

Please attach videos for all platforms for the same bug. This is must. With respect of this add general browsing of the app in those videos as this PR affect the whole app indirectly.
Follow the author checklist and mark all the items.

I will test it afterwards.

parasharrajat · 2022-09-26T18:17:34Z

You might have to update the PR template to the latest which is present here

lbaldy · 2022-09-26T21:48:47Z

@parasharrajat updated using the latest template, added screenshots related to all.

I had to push another commit, when running the web version of the app I noticed there was 'unused import' lint warning, so I had to fix it. Unfortunately it automatically dismissed @iwiznia's review.

I noticed a bug on iPad web, reproducible on production. Obviously this issue does not seem to be related to the PR as it is reproducible in production. I logged a new issue here: #11302

parasharrajat · 2022-09-30T15:42:18Z

@lbaldy Please merged main so that I can review it.

parasharrajat · 2022-09-30T15:56:39Z

I noticed a bug on iPad web, reproducible on production. Obviously this issue does not seem to be related to the PR as it is reproducible in production. I logged a new issue here: #11302

is this happening on main or this PR?

lbaldy · 2022-10-01T13:09:43Z

@lbaldy Please merged main so that I can review it.

@parasharrajat done

I noticed a bug on iPad web, reproducible on production. Obviously this issue does not seem to be related to the PR as it is reproducible in production. I logged a new issue here: #11302

is this happening on main or this PR?

This is not specific to this branch. I was able to reproduce it on a real production instance of the app at new.expensify.com.

I will report it on slack once I get my invitation resolved.

parasharrajat · 2022-10-03T15:28:23Z

Good. Thanks. Running my tests now.

lbaldy · 2022-10-04T20:01:50Z

@parasharrajat have you been running npm test when you were testing this? I see the pipeline fails here, however it passes on my machine every time I run it. I am running using the same nodejs version as pipeline.

parasharrajat · 2022-10-04T20:20:00Z

These tests are failing for some other reason, not on this PR only. I will keep an eye on this and let you know if fix is merged on main.

parasharrajat · 2022-10-05T14:32:34Z

So, I have run like 70% tests. Still running more. I don't see any issues but a few of the things are just happening randomly.

Sometimes, the sidebar(LHN) is getting stuck in the middle of the page on the web while resizing. (Need to check this on main).

@lbaldy Did you see any delays in animations or layout adjustments while resizing on this PR? It is hard to say that any of those are affected by removing debouncing. But it is good to know a few cases for testing.

I would say that even though I tried to test everything that could be affected by this change but we can't be 100% sure that everything is working as expected. Things, like animation delay or small jank on layout adjustment, could go unnoticed.

My personal recommendation is to keep the debouncing as it is not causing any issues and might be useful in reducing the computation cost in the js thread. But based on the slack thread https://expensify.slack.com/archives/C01GTK53T8Q/p1664819505882419, it is suggested to remove the debouncing.

So I think the best path forward would be to merge this PR and revert it if we found any issues.

Note: Reverting this PR does not revert the original fix that was done to fix the real issue on RN. This PR is only removing the debouncing on Dimension change event.

What do you say @iwiznia?

parasharrajat · 2022-10-05T14:34:35Z

@lbaldy I think Tests on the OP can't be run on our app at its current state as we disabled landscape mode.

Please modify these to include steps that the user needs to take to enable landscape mode and run those.

I am not sure what should be the QA steps here. Do you have suggestions? Please add those.

lbaldy · 2022-10-05T14:49:17Z

@lbaldy Did you see any delays in animations or layout adjustments while resizing on this PR? It is hard to say that any of those are affected by removing debouncing. But it is good to know a few cases for testing.

I would say that even though I tried to test everything that could be affected by this change but we can't be 100% sure that everything is working as expected. Things, like animation delay or small jank on layout adjustment, could go unnoticed.

@parasharrajat I haven't noticed anything like that. If you think it makes more sense, we can make this change ios only.

@lbaldy I think Tests on the OP can't be run on our app at its current state as we disabled landscape mode.

Please modify these to include steps that the user needs to take to enable landscape mode and run those.

I am not sure what should be the QA steps here. Do you have suggestions? Please add those.

@parasharrajat Orientation is only locked on iPhones, select iPad as a target to see the rotation happening.

parasharrajat · 2022-10-05T15:10:45Z

If you think it makes more sense, we can make this change ios only.

I don't see any benefit in that. Sooner or later we will have to remove it for all. It is better to take the step now and get it tested.

parasharrajat

LGTM. Tests complete.

cc: @iwiznia

PR 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 verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- 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 was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team 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 */
- Any functional components have the displayName property
- 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 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.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

🎀 👀 🎀 C+ reviewed

parasharrajat · 2022-10-06T13:07:41Z

Earlier discussed issue

Sometimes, the sidebar(LHN) is getting stuck in the middle of the page on the web while resizing. (Need to check this on the main).

Steps:

Open the app on a web browser (I am on Firefox).
resize the browser window to mobile view.
Now slightly enlarge the window horizontally but keep it in the mobile view(<800px width).
Observe sometimes, the drawer does not take the full width and the background content is visible.

I can reproduce this on both Staging and this PR. Though I noticed that the effect of this bug is a little severe on this PR, I do not blame this PR for the cause. It seems reasonable that the effect increased as we are removing the debouncing and the layout is now trying to update at a higher intensity.

screen-2022-10-06_18.31.20.mp4

parasharrajat · 2022-10-06T13:09:17Z

@lbaldy Please merge main into this and I hope that jest tests will pass.

lbaldy · 2022-10-06T13:18:11Z

@lbaldy Please merge main into this and I hope that jest tests will pass.

done

trjExpensify · 2022-10-07T18:00:05Z

👋 @iwiznia @parasharrajat - who's doing the honours on that merge button? This one has been a long time coming! 🎉

iwiznia · 2022-10-07T18:11:11Z

I was waiting for @AndrewGable's review.

OSBotify · 2022-10-07T18:31:04Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2022-10-12T00:05:56Z

🚀 Deployed to staging by @AndrewGable in version: 1.2.13-0 🚀

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

yuwenmemon · 2022-10-12T21:42:41Z

@kbecciv Are we able to check this one off?

OSBotify · 2022-10-13T05:55:55Z

🚀 Deployed to production by @yuwenmemon in version: 1.2.13-5 🚀

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

remove workaround introduced in Expensify#2180 after the proper fix from Expensify#2727

lbaldy mentioned this pull request May 24, 2022

[$16,000] Let's fix a React Native issue -- useWindowDimensions() returns swapped height and width in iOS #2727

Closed

5 tasks

lbaldy force-pushed the bugfix/2727 branch from a770b30 to b0e61f5 Compare September 8, 2022 20:50

lbaldy changed the title ~~[WIP] remove workaround introduced in #2180 after the proper fix from #2727~~ remove workaround introduced in #2180 after the proper fix from #2727 Sep 8, 2022

lbaldy closed this Sep 24, 2022

lbaldy force-pushed the bugfix/2727 branch from 13ee16b to b75faf0 Compare September 24, 2022 08:36

lbaldy reopened this Sep 24, 2022

lbaldy marked this pull request as ready for review September 24, 2022 20:00

lbaldy requested a review from a team as a code owner September 24, 2022 20:00

melvin-bot bot requested review from iwiznia, parasharrajat and AndrewGable and removed request for a team September 24, 2022 20:00

iwiznia previously approved these changes Sep 26, 2022

View reviewed changes

lbaldy dismissed iwiznia’s stale review via fa77873 September 26, 2022 21:23

lbaldy closed this Oct 1, 2022

lbaldy force-pushed the bugfix/2727 branch from fa77873 to 9ff60c3 Compare October 1, 2022 12:59

second part of Expensify#2727 fix, reverting Expensify#29290

eea4128

lbaldy reopened this Oct 1, 2022

parasharrajat reviewed Oct 6, 2022

View reviewed changes

Merge branch 'Expensify:main' into bugfix/2727

82ab2aa

iwiznia approved these changes Oct 6, 2022

View reviewed changes

AndrewGable approved these changes Oct 7, 2022

View reviewed changes

AndrewGable merged commit bf00dca into Expensify:main Oct 7, 2022

neil-marcellini mentioned this pull request Oct 10, 2022

[HOLD] [$500] Height of dropdown menu increases - reported by @thesahindia #8719

Closed

OSBotify mentioned this pull request Oct 11, 2022

Deploy Checklist: New Expensify 2022-10-11 #11741

Closed

48 tasks

kbecciv mentioned this pull request Oct 12, 2022

mWeb - Chat - Display mode switches from portrait to landscape when turning phone horizontally #11767

Closed

smrutiparida pushed a commit to autosave-app/App that referenced this pull request Oct 13, 2022

Merge pull request Expensify#9143 from lbaldy/bugfix/2727

b8292af

remove workaround introduced in Expensify#2180 after the proper fix from Expensify#2727

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove workaround introduced in #2180 after the proper fix from #2727 #9143

remove workaround introduced in #2180 after the proper fix from #2727 #9143

lbaldy commented May 24, 2022 •

edited

Loading

github-actions bot commented May 24, 2022 •

edited

Loading

lbaldy commented May 24, 2022

parasharrajat commented Sep 26, 2022 •

edited

Loading

parasharrajat commented Sep 26, 2022 •

edited

Loading

lbaldy commented Sep 26, 2022

parasharrajat commented Sep 30, 2022

parasharrajat commented Sep 30, 2022

lbaldy commented Oct 1, 2022

parasharrajat commented Oct 3, 2022

lbaldy commented Oct 4, 2022

parasharrajat commented Oct 4, 2022

parasharrajat commented Oct 5, 2022 •

edited

Loading

parasharrajat commented Oct 5, 2022

lbaldy commented Oct 5, 2022 •

edited

Loading

parasharrajat commented Oct 5, 2022

parasharrajat left a comment

parasharrajat commented Oct 6, 2022 •

edited

Loading

parasharrajat commented Oct 6, 2022

lbaldy commented Oct 6, 2022

trjExpensify commented Oct 7, 2022

iwiznia commented Oct 7, 2022

OSBotify commented Oct 7, 2022

OSBotify commented Oct 12, 2022

yuwenmemon commented Oct 12, 2022

OSBotify commented Oct 13, 2022

remove workaround introduced in #2180 after the proper fix from #2727 #9143

remove workaround introduced in #2180 after the proper fix from #2727 #9143

Conversation

lbaldy commented May 24, 2022 • edited Loading

Details

Fixed Issues

Tests

iPad Mini tests:

iPad Pro tests:

iPhone:

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

Screenshots

github-actions bot commented May 24, 2022 • edited Loading

lbaldy commented May 24, 2022

parasharrajat commented Sep 26, 2022 • edited Loading

parasharrajat commented Sep 26, 2022 • edited Loading

lbaldy commented Sep 26, 2022

parasharrajat commented Sep 30, 2022

parasharrajat commented Sep 30, 2022

lbaldy commented Oct 1, 2022

parasharrajat commented Oct 3, 2022

lbaldy commented Oct 4, 2022

parasharrajat commented Oct 4, 2022

parasharrajat commented Oct 5, 2022 • edited Loading

parasharrajat commented Oct 5, 2022

lbaldy commented Oct 5, 2022 • edited Loading

parasharrajat commented Oct 5, 2022

parasharrajat left a comment

Choose a reason for hiding this comment

PR Reviewer Checklist

parasharrajat commented Oct 6, 2022 • edited Loading

parasharrajat commented Oct 6, 2022

lbaldy commented Oct 6, 2022

trjExpensify commented Oct 7, 2022

iwiznia commented Oct 7, 2022

OSBotify commented Oct 7, 2022

OSBotify commented Oct 12, 2022

yuwenmemon commented Oct 12, 2022

OSBotify commented Oct 13, 2022

lbaldy commented May 24, 2022 •

edited

Loading

github-actions bot commented May 24, 2022 •

edited

Loading

parasharrajat commented Sep 26, 2022 •

edited

Loading

parasharrajat commented Sep 26, 2022 •

edited

Loading

parasharrajat commented Oct 5, 2022 •

edited

Loading

lbaldy commented Oct 5, 2022 •

edited

Loading

parasharrajat commented Oct 6, 2022 •

edited

Loading