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

[Pay on Wed 24th Nov] Improve IOU flow with step navigation animations #3921

Closed
JmillsExpensify opened this issue Jul 8, 2021 · 51 comments
Closed
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor Monthly KSv2 Reviewing Has a PR in review

Comments

@JmillsExpensify
Copy link

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:

The IOUModal and step PR introduces a multi-step navigation within a single Modal (see video at bottom of this issue).

Expected Result:

We should improve the current navigation with the use of animations when navigating between steps.

Our plan was to take advantage of the imminent react-native-navigation implementation (using Tab.Screen, hiding the TabBar, and disabling swipe), which gives us page animations and allows us to refresh each step without losing the current state. This change was never made, so it might instead be simpler to use a custom animation.

Actual Result:

Multi-step navigation within a single modal

Workaround:

E.cash is still functional with or without this improvement.

Platform:

All platforms.

Web
iOS
Android
Desktop App
Mobile Web

Version Number: 1.0.75
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:
https://user-images.githubusercontent.com/1127863/124861455-7952e280-df68-11eb-9001-d17b74cf5d62.mov

Expensify/Expensify Issue URL: https://github.com/Expensify/Expensify/issues/156712

View all open jobs on Upwork

@JmillsExpensify JmillsExpensify added the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Jul 8, 2021
@MelvinBot
Copy link

Triggered auto assignment to @conorpendergrast (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Jul 8, 2021
@JmillsExpensify
Copy link
Author

If you're coming from the Upwork posting (https://www.upwork.com/jobs/~01e8cb25843621f42d), please make sure to share your proposal here and a member from our team will evaluate. Thank you!

@MelvinBot
Copy link

Triggered auto assignment to @chiragsalian (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@parasharrajat
Copy link
Member

What type of animation are you looking here?

@JmillsExpensify
Copy link
Author

cc @Julesssss since you're online soon and this is coming from your issue in E.com.

@JmillsExpensify JmillsExpensify added the External Added to denote the issue can be worked on by a contributor label Jul 8, 2021
@MelvinBot MelvinBot added the Daily KSv2 label Jul 8, 2021
@rdjuric
Copy link
Contributor

rdjuric commented Jul 8, 2021

Really liked this idea!

We already have react-native-animatable as a dependency (from react-native-modal), it's a pretty good library for built-in animations. Might make sense to use it here too? It's performant and works on the web.

I added a few animations to our Request money flow so we can see how it might look:

Simulator.Screen.Recording.-.iPhone.SE.14.5.-.2021-07-08.at.03.57.21.mp4

Would love to work on this one. I can detail the proposal more once we get a better understanding of what we want here.

@chiragsalian
Copy link
Contributor

I'm not sure where we stand on this. @JmillsExpensify is there something else we need more for this issue or does any sort of animation work fine for the IOU pages? Does the video that @rdjuric shows above suffices for us?

@Julesssss
Copy link
Contributor

CC @shawnborton, as we talked about multi-step navigation.

In my opinion, we should only use left-to-right and right-to-left animations, depending on which way the user is navigating through the tabs. Vertical animations might give the impression of hierarchy between the pages, which we should avoid.

@Julesssss
Copy link
Contributor

We already have react-native-animatable as a dependency (from react-native-modal), it's a pretty good library for built-in animations. Might make sense to use it here too? It's performant and works on the web.

@rdjuric that makes sense to me. What do you think about the above comment?

@Julesssss
Copy link
Contributor

@chiragsalian I think we're close to approving the proposal. Let's just see if any issues are raised with the side-to-side animations before giving the go-ahead.

@rdjuric
Copy link
Contributor

rdjuric commented Jul 13, 2021

@rdjuric that makes sense to me. What do you think about the above comment?

Agree! I think we should use slide right-to-left to indicate that we're going forward, and slide left-to-right to indicate that we're going back - in both cases without the fadeIn effect from the video.

Just in case you and @shawnborton want to take a look, react-native-animatable has some examples of predefined animations (the ones we use in our Modals) in the README.

@Julesssss
Copy link
Contributor

That sounds good. slideInLeft & slideInRight seem to be the best candidates.

@shawnborton
Copy link
Contributor

Works for me too!

@chiragsalian
Copy link
Contributor

chiragsalian commented Jul 13, 2021

Cool looks like we're mostly in agreement about using react-native-animatable and slide left/right animations.

@rdjuric mind posting a more detailed proposal of how to plan to solve this so that we can go ahead and hire you if you're still interested 🙂

@rdjuric
Copy link
Contributor

rdjuric commented Jul 13, 2021

@chiragsalian Sure!

  1. Wrap each of the the steps (IOUAmount, IOUParticipants, IOUConfirm) of our IOUModal in an Animatable.View, and pass to the Animatable.View the animation config that we agreed upon.
    (It might be better to wrap some components of the Step instead of the whole View, I'll test it during the PR)
  2. Pass slideInLeft if we got to that step by calling our navigateToNextStep method, and slideInRight if we got to that step by calling our navigateToPreviousStep method.

@chiragsalian
Copy link
Contributor

Cool, LGTM, @JmillsExpensify feel free to hire @rdjuric on upwork.
@rdjuric, feel free to create the PR.

@JmillsExpensify
Copy link
Author

@rdjuric Would you mind retracting and re-submitting your proposal in Upwork. We've been having issues where the proposal isn't available for hire, and the re-submission fixes the issue. Thanks!

@rdjuric
Copy link
Contributor

rdjuric commented Jul 15, 2021

@JmillsExpensify I'm receiving a "This job is private. Only freelancers invited by client can view this job." message when trying to open the job link you posted here. So I haven't sent a proposal/was invited yet.

@Julesssss
Copy link
Contributor

The PR was updated again today and is looking better. After testing I noticed a few remaining issues which are shown here in the PR.

@MelvinBot
Copy link

@JmillsExpensify, @chiragsalian, @rdjuric Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@Julesssss
Copy link
Contributor

Hi @rdjuric, have you had the chance to review the feedback yet?

@rdjuric
Copy link
Contributor

rdjuric commented Sep 7, 2021

@Julesssss Sorry, looking into it now! Thanks.

@MelvinBot
Copy link

@JmillsExpensify, @chiragsalian, @rdjuric Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@JmillsExpensify
Copy link
Author

JmillsExpensify commented Sep 16, 2021

Requested changes updated in the linked PR.

@JmillsExpensify
Copy link
Author

Holding per the N6 hold label.

@MelvinBot
Copy link

@JmillsExpensify, @chiragsalian, @rdjuric Whoops! This issue is 2 days overdue. Let's get this updated quick!

@Julesssss
Copy link
Contributor

Holding due to N6 still

@MelvinBot
Copy link

@JmillsExpensify, @chiragsalian, @rdjuric Eep! 4 days overdue now. Issues have feelings too...

@MelvinBot
Copy link

@JmillsExpensify, @chiragsalian, @rdjuric 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

@chiragsalian
Copy link
Contributor

chiragsalian commented Oct 5, 2021

Issue on n-6 HOLD. Demoting to weekly since it melvin is too noisy.

@chiragsalian chiragsalian added Weekly KSv2 and removed Daily KSv2 labels Oct 5, 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.

@MelvinBot MelvinBot removed the Weekly KSv2 label Oct 29, 2021
@MelvinBot
Copy link

This issue has not been updated in over 15 days. @JmillsExpensify, @chiragsalian, @rdjuric 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!

@Julesssss
Copy link
Contributor

@chiragsalian resolved the conflicts and we've retested and merged the PR. Holding for payment.

@Julesssss Julesssss changed the title Improve IOU flow with step navigation animations [Pay on Wed 24th Nov] Improve IOU flow with step navigation animations Nov 17, 2021
@Julesssss Julesssss added Weekly KSv2 and removed Monthly KSv2 labels Nov 17, 2021
@MelvinBot MelvinBot removed the Weekly KSv2 label Dec 13, 2021
@MelvinBot
Copy link

This issue has not been updated in over 15 days. @JmillsExpensify, @chiragsalian, @rdjuric 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!

@MelvinBot MelvinBot added the Monthly KSv2 label Dec 13, 2021
@chiragsalian
Copy link
Contributor

Well the code for this is done. The only thing that remained was the payment to @rdjuric.
@JmillsExpensify will you be handling that? You can close the issue right after.

@JmillsExpensify
Copy link
Author

Ah I must have missed this one right before heading on for the holidays. Closing out and providing a bonus for the delay on my part.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering External Added to denote the issue can be worked on by a contributor Monthly KSv2 Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

10 participants