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 for payment 2023-06-01] [New Feature] Animate Native SplashScreen #14151

Closed
6 tasks done
roryabraham opened this issue Jan 9, 2023 · 58 comments
Closed
6 tasks done
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering NewFeature Something to build that is a new item.

Comments

@roryabraham
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:

Open the app

Expected Result:

The splash screen should appear, and then there should be an animated transition between the splash screen and the homepage of the app.

Actual Result:

The splash screen disappears, and the homepage appears. No animation.

Workaround:

n/a

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

This should apply to all platforms, though it's worth noting that there are currently some problems with the splash screen on web, stemming from both a misconfiguration and the CSP. So for the scope of this issue we can start with the native splash screen and then loop back to web.

Issue reported by: @mrousavvy
Slack conversation: https://expensify.slack.com/archives/C035J5C9FAP/p1673054140778389

View all open jobs on GitHub

@roryabraham roryabraham added Weekly KSv2 NewFeature Something to build that is a new item. labels Jan 9, 2023
@roryabraham roryabraham self-assigned this Jan 9, 2023
@melvin-bot melvin-bot bot locked and limited conversation to collaborators Jan 9, 2023
@roryabraham
Copy link
Contributor Author

This is only assigned to me in the interim, but I plan to pass this off to @mrousavy ASAP (see the slack thread)

@roryabraham
Copy link
Contributor Author

DM'd Marc to ask if he still wants this one. It's not urgent at all.

@JmillsExpensify
Copy link

Someone that's passionate is welcome to pick it up, though adding the not a priority label for clarity.

@roryabraham roryabraham removed their assignment Jan 19, 2023
@roryabraham
Copy link
Contributor Author

Going to drop this since I'm not going to work on it any time soon

@mrousavy
Copy link
Member

Cool, I can take a look at this soon (next week-ish)

@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Feb 13, 2023
@MelvinBot
Copy link

This issue has not been updated in over 15 days. eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@roryabraham
Copy link
Contributor Author

@mrousavy assigning to you to keep it on your radar. It's a nice-to-have improvement but we'd welcome it whenever you've got some spare cycles.

@JmillsExpensify
Copy link

+1 to that. Also CCing @Szymon20000 since he's handling the weekly updates.

@Szymon20000
Copy link
Contributor

We have it on our list but haven't started working on it yet.

@mrousavy
Copy link
Member

mrousavy commented Mar 7, 2023

Here's an example on how it could look like: https://twitter.com/mrousavy/status/1323305326473551874

@shawnborton
Copy link
Contributor

Love it.

@janicduplessis
Copy link
Contributor

This might be out of scope, but I also noticed we are not using the new splash screen api on android, this causes 2 different splash screens to show on recent android versions. Looks like we currently vendor the splash screen library, I know latest version of https://github.com/zoontek/react-native-bootsplash supports it.

@JmillsExpensify
Copy link

Actually I think that's a great point, as we've struggled with the 2 splash screens in a string of bug reports. I think we should address that, either separately or in this same issue. It's probably best practice to mention something in our Slack room so we get more 👀 on this and align.

@zoontek
Copy link
Contributor

zoontek commented Mar 13, 2023

Hi folks! If needed, I can help you migrate to Android 12 Splash screen API first and animate the splash screen. We can use Animated, reanimated or even Lottie for that.
But it's probably better to do it in two times: first we migrate, then we add the animated splash screen.

@MelvinBot

This comment was marked as resolved.

@zoontek

This comment was marked as resolved.

@grgia
Copy link
Contributor

grgia commented Apr 18, 2023

@JmillsExpensify I think we were considering using the coin instead of our in-app loader, so it wouldn't replace the icon mark in the splash screen for this issue.

But instead replace this one:

Screenshot 2023-04-18 at 9 41 25 AM

@zoontek
Copy link
Contributor

zoontek commented Apr 18, 2023

@grgia This has been removed in my PR (#17477). The issue was here:

https://github.com/Expensify/App/pull/17477/files#diff-c3d3bf8bc4056d8f2e8ff15f6d72d7e81656cd36c30a202a084c50dda1dff582L55

And has their doc mention here:

If you have a native splash screen, please use onReady instead of fallback prop.

Now the splash screen fade with a small shrinking effect to show the form, no additional loader.

@MelvinBot
Copy link

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

@shawnborton
Copy link
Contributor

Out of curiosity @shawnborton, do we want to use this animation instead of keeping the user focused on the logo? I'm not particularly passionate, but I'd be curious to know your thought process versus for the coin at this stage versus later and in an app flow.

@JmillsExpensify I think I was mostly curious to see the coin animation as a placeholder, because I am trying to understand what this could even look like in terms of when the animation can start, how long it might last, etc.

But I agree, my preference for the splash screen would be to just match the E iconmark from the app launcher logo. And that being said, maybe I am overthinking this and we should just go with something super simple for now - we don't need to get crazy for the splash screen because we want it to feel super lightweight and quick. So yeah, maybe we just stick with something similar to this for now, and then separately Georgia can implement the coin loader for in-app loading.

@JmillsExpensify
Copy link

Nice, sounds great!

@kadiealexander
Copy link
Contributor

kadiealexander commented Apr 26, 2023

Making a note that we initially agreed to pay @zoontek for this issue here, however @roryabraham and I spoke to @zoontek in Newdot, and since the scope of this issue blew quite far out from the initial agreement, we agreed to the following:

$2000 total (one bounty for each of):

The first payment for the first project has been paid here in a milestone, and I'll retain the same contract to make the second payment as a milestone once both PRs have completed the regression period.

@zoontek
Copy link
Contributor

zoontek commented May 4, 2023

In this PR, I fixed this issue: #18172 (as @grgia mentioned it here)
This was an old bug since it has been here since you have a native splash screen. It might be more visible lately since the latest changes made the splash screen faster to hide.

Like I said in the description, this was caused by NavigationContainer fallback prop:

I removed the fallback prop on NavigationContainer, which was responsible for this ugly loading screen just after the splash screen hide. It appears their documentation say: "If you have a native splash screen, please use onReady instead of fallback prop.", so this is the way to go (Link)

@kadiealexander I'm not sure I could apply on Upwork for this one too?

@roryabraham
Copy link
Contributor Author

Yeah, @zoontek if your PR fixes #18172 then you are eligible for compensation for that one too

@melvin-bot
Copy link

melvin-bot bot commented May 19, 2023

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@0xmiros
Copy link
Contributor

0xmiros commented May 19, 2023

Not blocker anymore, Melvin! It was fixed earlier.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Monthly KSv2 labels May 25, 2023
@melvin-bot melvin-bot bot changed the title [New Feature] Animate Native SplashScreen [HOLD for payment 2023-06-01] [New Feature] Animate Native SplashScreen May 25, 2023
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label May 25, 2023
@melvin-bot
Copy link

melvin-bot bot commented May 25, 2023

Reviewing label has been removed, please complete the "BugZero Checklist".

@melvin-bot
Copy link

melvin-bot bot commented May 25, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.17-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-06-01. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels May 31, 2023
@kadiealexander
Copy link
Contributor

Both bounties issued for @zoontek.

@0xmiros
Copy link
Contributor

0xmiros commented Jun 1, 2023

@kadiealexander I am also eligible for C+ review

@kadiealexander
Copy link
Contributor

Your payment for the review of PR Invert bootsplash colors + fade splash screen on web #16932
should be covered here. Please let me know if you had any issues receiving this payment.

@0xmiros
Copy link
Contributor

0xmiros commented Jun 1, 2023

@kadiealexander They're different PRs from different requests.
The first PR was requested C+ review here by @neil-marcellini.
The 2nd PR was requested C+ review here by @marcochavezf.
Coincidentally, I raised interests on both PRs and became same C+ 🙂

And from this comment, it makes sense that bounty would be 2k total since this is not internal PR but external (maybe agency).

@kadiealexander
Copy link
Contributor

Thanks for flagging those! I've sent a contract to pay another $1000 to bring the total up to $2k like was paid to @zoontek. :)

@0xmiros
Copy link
Contributor

0xmiros commented Jun 1, 2023

Thanks @kadiealexander
Accepted offer

@kadiealexander
Copy link
Contributor

Paid! @roryabraham all payments have been issued, if this one is ready to close.

@kadiealexander
Copy link
Contributor

Closing, please reopen if anything was missed.

@melvin-bot
Copy link

melvin-bot bot commented Jun 2, 2023

@roryabraham @kadiealexander Be sure to fill out the Contact List!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering NewFeature Something to build that is a new item.
Projects
None yet
Development

No branches or pull requests