woocommerce / woocommerce-android

WooCommerce Android app
https://www.woocommerce.com/mobile
GNU General Public License v2.0
277 stars 135 forks source link

Improve orders details intro animation #4363

Closed wzieba closed 2 years ago

wzieba commented 3 years ago

When I navigate to order details from order list, the animation seems to be "jumpy". If you'll go frame-by-frame of the animation you can see that after clicking on order:

All this combined give some "jumpy" effect and doesn't look the best:

https://user-images.githubusercontent.com/5845095/124561851-a2775400-de3e-11eb-98f1-3be9e6decd78.mov

We could improve animations there, one idea can be to go with such transition (and dialog fragment):

https://user-images.githubusercontent.com/5845095/124562181-039f2780-de3f-11eb-83cc-dc20ad3d1e68.mp4

Source

hichamboushaba commented 3 years ago

Just a remark @wzieba, I don't think that using a DialogFragment would be a good idea, as the order details screen leads to many other Fragments and when navigating from DialogFragment to other fragments, unless you dismiss it, it will stay displayed to the top of them, and if we dismiss it we will lose the state.

Also just to have a bit of history, the slide in from the right animation was requested specifically here 🙂

wzieba commented 3 years ago

Thanks for the tips @hichamboushaba 👍

I don't think that using a DialogFragment would be a good idea, as the order details screen leads to many other Fragments

Sure it does, but we could create a separate NavGraph attached to ChildFragmentManager for navigating between different fragments in scope of OrderDetailFragment.

the slide in from the right animation was requested specifically

Sure, I hope that it's not a "status quo" 🙂!

@Garance91540 could you please take a look and see if this type of animation would be a fit for us? And if not - do you have some ideas to improve the current one we have?

hichamboushaba commented 3 years ago

Sure it does, but we could create a separate NavGraph attached to ChildFragmentManager for navigating between different fragments in scope of OrderDetailFragment.

Unless I'm missing something, I don't think this will solve the issue, as DialogFragments are attached to a separate Window, so even if you navigate to fragment using their ChildFragmentManager, it won't be attached to the same window, but to the activity's window. I may be wrong though 🙂.

Also I don't like the idea of complicating the navigation architecture just for the sake of improving an animation, I think we should investigate why the animation is jumpy with the current solution first, as the main issue IMO is the delay on displaying the shimmer effect.

I just wanted to mention this because in a previous project, DialogFragment were overused for full screen UIs, and we had to do a lot of refactoring to migrate them back to regular Fragments to fix a lot of issues that they were causing, and it was a nightmare.

hichamboushaba commented 3 years ago

One other remark @wzieba 🙂 , I don't think the video shared is using a DialogFragment, as in the video a shared element is used in the transition, but DialogFragment doesn't support them AFAIK

Screen Shot 2021-07-06 at 14 27 53
wzieba commented 3 years ago

I'd have to do some tests but I believe that it's possible to navigate between fragments inside DialogFragment without issues.

Also I don't like the idea of complicating the navigation architecture just for the sake of improving an animation

I'd say it's not complicating anything but it's difficult to discuss it without any code samples. I think we could move this discussion further only in PR review with some code changes indeed.

I just wanted to mention this because in a previous project, DialogFragment were overused for full screen UIs

Thanks for your awareness! I don't have such experiences so it's good to have an opposite opinion on DialogFragment. For me it's totally ok if we decide to not use them if it'll be indeed less problematic 🙂.

I don't think the video shared is using a DialogFragment

TBH I don't think those videos are rendered on Android phone 😅 (look at status bar - is there a way to have such symbols instead of batter/clock etc.?). I believe those might be just preview animations rendered in animation software. But good point with not DialogFragment not supporting shared element transitions.

Garance91540 commented 3 years ago

Hi there,

I'm all for making this transition animation a bit smoother. @wzieba is it possible to show a side by side of the current animation and the one you suggest in the product list? It's a bit hard to compare right now

wzieba commented 3 years ago

👋

It's a bit hard to compare right now

You're right - I'll be back with some side-to-side samples next week as I'll be working on this animation then.

wzieba commented 3 years ago

Just a heads-up @Garance91540 - I might not fit this animation improvements in this sprint as some other priority occurred. But I'll be happy to return to this on some hack week time!

Garance91540 commented 3 years ago

Sounds good!