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

[HOLD] [iOS] Figure out why setTimeout is needed when viewing SidebarScreen on load, replace with better solution #5296

Closed
Beamanator opened this issue Sep 16, 2021 · 27 comments
Assignees

Comments

@Beamanator
Copy link
Contributor

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Remove setTimeout from this code
  2. Set isActive={true} there so the global create menu should show immediately
  3. Rebuild app
  4. Close app completely on iOS simulator
  5. Reopen app

Expected Result:

Global create menu should show up. Since it doesn't, we were forced to introduce a 200ms timeout here, but we want to understand why & how to remove that and still make the global create menu display when a new expensify user opens the app for the first time (unless they already have a workspace)

Actual Result:

Global create menu doesn't show up

See @mateomorris 's tests in this comment

Workaround:

Keep the code as it is, it works but we don't know why it's necessary 👎

Possible solutions / ideas so far

  1. Idea from @marcaaron here
  2. Some notes from @mateomorris in his comment here

Platform:

Where is this issue occurring?

  • iOS

Version Number: latest
Notes/Photos/Videos: Any additional supporting documentation
Issue reported by: @mateomorris here
Slack conversation: N/A

View all open jobs on GitHub

@Beamanator Beamanator added External Added to denote the issue can be worked on by a contributor Daily KSv2 n6-hold labels Sep 16, 2021
@MelvinBot
Copy link

Triggered auto assignment to @kadiealexander (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@Beamanator Beamanator self-assigned this Sep 16, 2021
@kadiealexander
Copy link
Contributor

Upwork job: https://www.upwork.com/jobs/~01e9442bc5ff417a90

@kadiealexander kadiealexander added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 17, 2021
@Spider3dev
Copy link

Spider3dev commented Sep 17, 2021

@Beamanator
I think it will be fixed in this way.

if (!hasFreePolicy) { // For some reason, the menu doesn't open without the timeout setTimeout(() => { this.toggleCreateMenu(); }, 200); }

!hasFreePolicy && this.toggleCreateMenu();

Thanks.

@MelvinBot MelvinBot added Weekly KSv2 and removed Daily KSv2 labels Sep 17, 2021
@Beamanator
Copy link
Contributor Author

@nhanth1012 please add more detail and explanation to your proposal. Why do you think that will fix the issue?

Also, are you recommending replacing the entire if-statement with !hasFreePolicy && this.toggleCreateMenu();? This doesn't follow our style guidelines, and I don't believe it will help us because the setTimeout is the only way we got this to work without changing code elsewhere

@Spider3dev
Copy link

@Beamanator
I think you want me to remove the setTimeout from the code. right?
Of course, I will consider your suggestion carefully.

@Beamanator
Copy link
Contributor Author

@nhanth1012 Ideally yes, or if setTimeout is absolutely necessary here, we would like to know why and document it. Thanks 🙏

@roryabraham
Copy link
Contributor

I think that a valuable next-step for this issue would be to take some lessons/evidence from this issue and submit a detailed bug report to react-navigation with a minimal reproduction of the issue. From there, we can see what activity the issue generates/whether the maintainers pick it up or whether we should investigate the possibility of submitting a fix ourselves.

@kadiealexander
Copy link
Contributor

N6 hold is still applied this week.

@Beamanator
Copy link
Contributor Author

This could be another candidate for getting assistance after sponsoring react-navigation (https://github.com/Expensify/Expensify/issues/179779)

@MelvinBot MelvinBot removed the Overdue label Oct 6, 2021
@kadiealexander
Copy link
Contributor

Please refer to this post for updated information on the n6 hold, we've raised the bonus to $250 for all issues where a PR is created before the N6 hold is lifted.

@Beamanator Beamanator changed the title [iOS] Figure out why setTimeout is needed when viewing SidebarScreen on load, replace with better solution [HOLD] [iOS] Figure out why setTimeout is needed when viewing SidebarScreen on load, replace with better solution Oct 11, 2021
@Beamanator
Copy link
Contributor Author

Back at it after a busy last week, will try to get this done the upcoming week

@kadiealexander
Copy link
Contributor

Not overdue Melvin, please go away!

@kadiealexander
Copy link
Contributor

@Beamanator do you have any update on this one?

@MelvinBot MelvinBot removed the Overdue label Dec 7, 2021
@Beamanator Beamanator added Monthly KSv2 and removed External Added to denote the issue can be worked on by a contributor Weekly KSv2 Exported labels Dec 7, 2021
@Beamanator
Copy link
Contributor Author

I'm dropping this to Monthly since I've got a bunch of other priorities at the moment, I'll try to take a look this month

@Beamanator
Copy link
Contributor Author

Haven't gotten to this yet, other priorities keep popping up

@Beamanator
Copy link
Contributor Author

Not top priority still

@Beamanator
Copy link
Contributor Author

Still in the middle of some high priory tasks (currently Offline First doc)

@MelvinBot MelvinBot removed the Overdue label Mar 25, 2022
@tgolen
Copy link
Contributor

tgolen commented Apr 19, 2022

// This is a short-term workaround

This is precisely why we shouldn't ever put these comments in the code 😬

@Beamanator
Copy link
Contributor Author

So true 😅 🤦

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants