Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.32k stars 2.76k forks source link

[New feature] Add loading indicator when ReconnectApp is running #46611

Open slafortune opened 1 month ago

slafortune commented 1 month ago

Original post -

Version Number: v1.4.77-11 Reproducible in staging?: Yes Reproducible in production?: Yes If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): NA Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: https://github.com/Expensify/Expensify/issues/350373 Issue reported by: @flodnv Slack conversation: Original proposal here : https://expensify.slack.com/archives/C01GTK53T8Q/p1699972596342559

Sometimes, the app is loading something for a little while; as an end user, you have no clue what's happening. Then, when loading finishes, sometimes several seconds later, all of a sudden, new data pops up and takes you by surprise, resulting in a very unpleasant UX that almost feels broken.

Coming from this we've landed on showing the thin bar loader at the top of the screen when ReconnectApp is running.

image

@dubielzyk-expensify created a CodePen and it's the animation called Rubber Band or #loading1 in code.

My code isn't the best so I'd urge us to optimize it rather than copy paste 😅

Much discussion took place here

Issue OwnerCurrent Issue Owner: @
melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @jliexpensify (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

melvin-bot[bot] commented 1 month ago

:warning: It looks like this issue is labelled as a New Feature but not tied to any GitHub Project. Keep in mind that all new features should be tied to GitHub Projects in order to properly track external CAP software time :warning:

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to Design team member for new feature review - @dubielzyk-expensify (NewFeature)

getusha commented 1 month ago

@slafortune could you assign me here too? thanks :)

jliexpensify commented 1 month ago

Assigned you @getusha

dubielzyk-expensify commented 1 month ago

Excited about this one. Let me know when there's something to look and test 😄

melvin-bot[bot] commented 1 month ago

Unable to auto-create job on Upwork. The BZ team member should create it manually for this issue.

melvin-bot[bot] commented 1 month ago

Current assignee @getusha is eligible for the External assigner, not assigning anyone new.

nkdengineer commented 1 month ago

Edited by proposal-police: This proposal was edited at 2024-08-06 15:13:28 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Add loading indicator when ReconnectApp is running

What is the root cause of that problem?

This is a new feature

What changes do you think we should make in order to solve the problem?

The test branch is here: https://github.com/nkdengineer/App/tree/fix/46611

About the animation: animation: 1.5s loading-bar .3s cubic-bezier(0.65, 0, 0.35, 1) infinite;

@keyframes loading-bar {
  0% {
    left: 0;
    width: 0;
  }
  50% {
    left: 0;
    width: 100%;
  }
  100% {
    left: 100%;
    width: 0;
  }
}
  1. We can create a ProgressBar component showing a loading indicator as the design above. Here is an example of this. We can refactor the logic and variables in the PR phrase. The implement detail can see in the test branch above

  2. When ReconnectApp is called, we have isLoadingReportData in Onyx to know when it's running and it's complete. So we can use this variable to show the ProgressBar while ReconnectApp API is running.

const [isLoadingReportData] = useOnyx(ONYXKEYS.IS_LOADING_REPORT_DATA);
{<ProgressBar shouldShow={isLoadingReportData}/>}

https://github.com/Expensify/App/blob/3666caa186f9c4530a1c57176eca601768c4953c/src/pages/home/sidebar/SidebarScreen/BaseSidebarScreen.tsx#L56-L59

What alternative solutions did you explore? (Optional)

NA

Result

https://github.com/user-attachments/assets/c45fda81-5b3b-4e7f-9176-64547e7fdf63

https://github.com/user-attachments/assets/e7986b41-9f54-47ce-b9ad-43ee97502705

ShridharGoel commented 1 month ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Add loading indicator when ReconnectApp is running.

What is the root cause of that problem?

New feature.

What changes do you think we should make in order to solve the problem?

Add isLoadingApp in TopBar:

const [isLoadingApp] = useOnyx(ONYXKEYS.IS_LOADING_APP);

Then, add the animation component at the bottom:

{isLoadingApp && <LoadingAnimation />}

Example code for loading animation:

import React, { useEffect, useRef } from 'react';
import { Animated, View, StyleSheet, Dimensions } from 'react-native';

const { width } = Dimensions.get('window');

const LoadingAnimation= () => {
    const greenAnimation = useRef(new Animated.Value(0)).current;

    useEffect(() => {
        const animate = () => {
            greenAnimation.setValue(0);

            Animated.sequence([
                // Animation from 0 to width
                Animated.timing(greenAnimation, {
                    toValue: width,
                    duration: 2000,
                    useNativeDriver: false,
                }),
                // Animation from width to 0
                Animated.timing(greenAnimation, {
                    toValue: 0,
                    duration: 2000,
                    useNativeDriver: false,
                }),
            ]).start(() => animate());
        };

        animate();
    }, [greenAnimation]);

    const greenWidth = greenAnimation.interpolate({
        inputRange: [0, width],
        outputRange: [width, 0],
    });

    return (
        <View style={styles.container}>
            <Animated.View style={[styles.loadingBar, { width: greenWidth }]} />
        </View>
    );
};

const styles = StyleSheet.create({
    container: {
        left: 0,
        width: '100%',
        height: 4,
        backgroundColor: 'transparent',
    },
    loadingBar: {
        position: 'absolute',
        height: '100%',
        backgroundColor: 'green',
    },
});

export default LoadingAnimation;

We can improve the animation.

https://github.com/user-attachments/assets/6205d25f-0764-4957-84ed-43c457ce4de7

https://github.com/user-attachments/assets/39e0c9f7-b2c1-4f7a-9702-7765cb361f32

Note that the exact animation can be created during the PR time.

ShridharGoel commented 1 month ago

Proposal

Updated to include videos.

dubielzyk-expensify commented 1 month ago

Not overdue

getusha commented 1 month ago

The demos attached by both @ShridharGoel & @nkdengineer look way off to me... Could you guys refer to the codepen provided by @dubielzyk-expensify https://codepen.io/dubielzyk/full/ExBVxVG?

nkdengineer commented 1 month ago

@getusha I updated my proposal with the result as the expected design from codepen and also updated the test branch.

nkdengineer commented 1 month ago

I modified my proposal with the expected.

getusha commented 1 month ago

Reviewing @nkdengineer's proposal

melvin-bot[bot] commented 1 month ago

@jliexpensify, @getusha, @dubielzyk-expensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

JmillsExpensify commented 1 month ago

How's the proposal review going?

getusha commented 4 weeks ago

I will provide an update today

getusha commented 4 weeks ago

I think it's better than the previous one, @nkdengineer. As you probably noticed, the timing isn't constant on which the width of the bar fills and goes away. https://www.w3schools.com/cssref/tryit.php?filename=trycss_func_cubic-bezier

It's in the demo too:

animation: 1.5s loading-bar .3s cubic-bezier(0.65, 0, 0.35, 1) infinite;

Could you check this and include in your proposal how we will implement the animation effect here?

nkdengineer commented 4 weeks ago

@getusha Updated proposal with the detail about the animation.

The new result here

https://github.com/user-attachments/assets/19ba2f04-4861-42e0-80ff-6135e7fe4ec1

dubielzyk-expensify commented 3 weeks ago

This is looking great. Minor detail, but can we have it start from a place of empty, then fill? That is, it shouldn't start as fully green.

cc @Expensify/design for visibility

nkdengineer commented 3 weeks ago

This is looking great. Minor detail, but can we have it start from a place of empty, then fill? That is, it shouldn't start as fully green.

@dubielzyk-expensify Sure, it's expected from my solution. Here is the full result.

https://github.com/user-attachments/assets/0f34d0ba-ed49-4325-a11b-c21fc306ef1f

flodnv commented 3 weeks ago

Coming from https://github.com/Expensify/App/issues/43453 we've landed on showing the thin bar loader at the top of the screen when ReconnectApp is running.

@dubielzyk-expensify sorry for realizing this so late -- why are we not showing the loader on the LHN in large screen mode?

dannymcclain commented 3 weeks ago

Dang this is feeling pretty darn slick!

sorry for realizing this so late -- why are we not showing the loader on the LHN in large screen mode?

@shawnborton @flodnv @dubielzyk-expensify Good call. You're right that we had decided to initially go with the LHN on large screens. Relevant links:

I'm honestly not sure how much I care re: LHN vs. Content pane, I think both look good. Just noting what we decided on that other issue 🙃

shawnborton commented 3 weeks ago

Yeah, I don't feel super strongly but I did initially like just using the LHN to start since it would be consistent everywhere. Does ReconnectApp really only modify what's in the LHN? Or does it also impact what is seen in the content pane?

flodnv commented 3 weeks ago

Does ReconnectApp really only modify what's in the LHN? Or does it also impact what is seen in the content pane?

It could be both. If it modifies the content pane, it probably also modifies something in the LHN.

getusha commented 3 weeks ago

I'm honestly not sure how much I care re: LHN vs. Content pane, I think both look good. Just noting what we decided on that other issue 🙃

So this means, on larger screens, display it only on the LHN. and on smaller screens, display it in both the LHN and the content pane. right?

flodnv commented 3 weeks ago

I think that makes the most sense to me.

nkdengineer commented 3 weeks ago

Updated proposal

shawnborton commented 3 weeks ago

Hmm I think it could look weird if we have two different loading bars on desktop for the LHN and the content pane. I would rather we just pick one (and I would probably pick LHN?) or make the loading bar one continuous bar that stretches across the LHN and content pane at the same latitude.

dubielzyk-expensify commented 3 weeks ago

Yeah. Now that the proof of concept is out, I'd agree with those two solutions. I think across the whole thing would be the best though, but LHN works too. I guess it's weird to see pulsing of skeleton and the bar in the same area

dubielzyk-expensify commented 3 weeks ago

Not overdue

getusha commented 2 weeks ago

@nkdengineer Instead of the bar suddenly disappearing, can we make it slide away? I think it would be better to render it all the time with a transparent background as well to avoid layout shift. WDYT @flodnv?

nkdengineer commented 2 weeks ago

I think it would be better to render it all the time with a transparent background as well to avoid layout shift

@getusha If we display a transparent background after the API is complete, doesn't it suddenly disappearing?

We also want it to start from an empty place. So I think it's good to mount this component when the ReconnectApp API is in progress.

This is looking great. Minor detail, but can we have it start from a place of empty, then fill? That is, it shouldn't start as fully green.

getusha commented 2 weeks ago

@getusha If we display a transparent background after the API is complete, doesn't it suddenly disappearing?

I don't understand your question @nkdengineer, it will stay with a transparent background and we will just animate the green bar as needed.

nkdengineer commented 2 weeks ago

@getusha As my proposal we have a wrap view and the animated view (green line), do you mean we will always display the wrap view and only display the animated view when we need, right?

getusha commented 2 weeks ago

@getusha As my proposal we have a wrap view and the animated view (green line), do you mean we will always display the wrap view and only display the animated view when we need, right?

@nkdengineer Yes, so when the ReconnectApp call ends, we will simply slide the green line away so it won't appear to be sudden.

nkdengineer commented 2 weeks ago

@getusha Got it, I updated my test branch and proposal. The new result:

https://github.com/user-attachments/assets/cf26c464-ce0c-4e3d-bf9c-3dc701ed70dc

getusha commented 2 weeks ago

when the ReconnectApp call ends, we will simply slide the green line away so it won't appear to be sudden.

@nkdengineer can we also make sure the green bar slides away (set the width/left value with animation) instead of suddenly disappearing?

nkdengineer commented 2 weeks ago

@getusha Is this the expected behavior that we want?

https://github.com/user-attachments/assets/f0f364f8-1e55-43da-97dd-c641d7353943

shawnborton commented 2 weeks ago

Hmm it looks like there is a permanent 2px border now under the header area, which I don't think we want.

getusha commented 2 weeks ago

We could match the background? Otherwise it will appear jumpy when it gets mounted and removed. Not sure but could also shift the layout for a moment, which wouldn’t look ideal.

On Mon, 26 Aug 2024 at 2:23 PM Shawn Borton @.***> wrote:

Hmm it looks like there is a permanent 2px border now under the header area, which I don't think we want.

— Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/46611#issuecomment-2309971094, or unsubscribe https://github.com/notifications/unsubscribe-auth/AR4OEVYENBSUNFOECICCJATZTMF3FAVCNFSM6AAAAABLZH7ZSGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMBZHE3TCMBZGQ . You are receiving this because you were mentioned.Message ID: @.***>

shawnborton commented 2 weeks ago

I think it should be absolutely positioned so it doesn't bump the content at all.

dubielzyk-expensify commented 2 weeks ago

I think it should be absolutely positioned so it doesn't bump the content at all.

100% agree.

getusha commented 2 weeks ago

I think it should be absolutely positioned so it doesn't bump the content at all.

That makes perfect sense!

getusha commented 2 weeks ago

@shawnborton @dubielzyk-expensify do you think the way we hide the bar matters? (i.e. adding a transition out/slide out instead of just disappearing suddenly?)

shawnborton commented 2 weeks ago

I think adding some kind of transition would be nice, whether it slides down from the top, or fades in, or something.

dannymcclain commented 2 weeks ago

Agree with you all about making it so the bar doesn't make the UI bump. Whether that's absolutely positioning it or using a transparent border as a placeholder, I don't have strong feelings. I just don't want a jumpy UI haha.

As for the way we hide it, I think a decently quick fade would be nice, because we don't really know at what point in its "bounce" it's going to go away, right?

dubielzyk-expensify commented 2 weeks ago

Agree with a pretty quick fade would look good here.