-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat(routeprogressbar): add progressbar when loading new chunks #330
base: master
Are you sure you want to change the base?
Conversation
61843d2
to
3e80c2a
Compare
80ddb1d
to
03e88e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code-wise, looks good to me 😄 , but maybe Joe would want to review the UI?
className={css.progressBar} | ||
style={ | ||
{ | ||
["--progress"]: `${(-1 + progress) * 100}%`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the -1? Does progress go from 1 to 100? (I tried looking this up in the docs for the package, but didn't find info there 🤷 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good question - I just followed the example code for the css stuff.
But it seems to be a "trick" because it uses width: 100%
. So it actually goes from -100%
to 0%
. So eg. 0%
margin is 100% and -10%
is visually 90% of progress.
I would prefer to stick with the loaders we have available in UI, rather than introducing this new Perhaps we can't get the name of the section/page while waiting for loading, but is there a way to use the |
We could probably render the bread crumb. But depending on the complexity of the header/name it might be hard to show it, since my initial plan was to have this as part of the page, and thus it's not available when loading it. Should be possible though (but then we cant render any data names. eg "Edit Data element: ANC Visit". We can use the I've been doing some research on react-router, and they have their own loading mechanism to prevent the "waterfall" of loading spinners. So an app would work more like a traditional (non SPA) application, and wait for data before navigating. However I wasn't planning to implement that since it has a lot of technical implications. But since these route-transitions should in most cases be quite quick, I do find adding an additional spinner and blocking the entire page is unfortunate. And since in most cases it's pretty quick, you will get this "flash" of the spinner. A lot of newer apps have moved to this style of route transitions. Eg. here on GitHub. Gmail also "waits" rendering the new page and just shows a snackbar - "Loading" at the top. Here you can see the "spinner flash" Im talking about (dont blink or you will miss it :)) Screen.Recording.2023-05-24.at.14.04.01.movWe could however show the spinner somewhere on the page, while not blocking the current page, eg something like this: Screen.Recording.2023-05-24.at.14.53.56.mov |
Yes, I see your point. I suppose this is more of a 'preload' state, rather than a fully-fledged 'loading' state. My concern with the progress bar is, though it's getting more popular, it doesn't feel like a particularly intuitive UI. I think it's easily missed or misunderstood and can lead to clicking something and feeling "nothing happened!". We could make the loading indicator more visible, as in your second example, but I think there's still a feeling of "I clicked a navigation link and nothing happened". I think users are more used to seeing immediate action after a click, even if that immediate action is a loading spinner. Not noticing that loading is actually happening can then lead to people clicking on other things (to see if anything works) and ends up making them wait even longer. On the other hand, I'm also not a fan of flashing spinners, so I'd like to avoid those too. Assuming we went for a "spinner on the page" for this preloader, I wonder if we could avoid flashing by:
Would the above be technically feasible? |
I think that timing animations is something that is very tricky as we can't really say how long a request will take. With a good internet connection and a decent bandwidth, these values might make sense. But in other countries, where infrastructure isn't as developed, we might produce flickering effect we tried to avoid. I agree with the fact that the progress bar can easily be overlooked. It took me quite a few times to realize what was happening on GH until I got used to it, and I still don't like them that much. I also don't like flickering loading spinners, I think this kind of feedback, while not pretty, makes things very obvious and therefore adds to UX while sacrificing on the UI / prettiness of the app. In that regard the amazon catalog is a very good example. It's really not that pretty, has multiple loading spinners, prices are not aligned, etc. But it's very easy to use, you get almost all the information you want, and so far no one I know has ever complained about the UI. I'd be in favor of some more ugly, but obvious loading spinners, even if that means that after the first loading spinner, we see a headline with another loading spinner, etc |
I'm not an expert at this, but I mean, that's how the web has worked forever? Every non-SPA app doesn't do anything until the page is loaded. The only difference here is that the "refresh" symbol in your browser won't change from "reload" to an "X", and sometimes the browser says "loading" in the bottom left corner . It's only with SPA apps that the user is "expects" something to happen immediately, and most "every day" webpages are not SPA (some have hybrid solutions like GitHub). Of course, the DHIS2 users will be used to SPAs, and could react as you say, but the same is true when clicking a link in the header-bar - nothing happens until navigated link is loaded Go to amazon, slow down your network and click a link -> "nothing" happens for several seconds until the new page is loaded. Of course the browser gives some feedback that something is happening, but it's pretty minor, and it might be more ingrained in the common user that "something is happening because the reload symbol is an "X" and the favicon is a loading-spinner" then I give it credit for.
Well I'm one to have complained about their UI numerous times. Amazon is notoriously famous for having ugly UI. I really don't think we should look to amazon as an example of what to do. It's amazing to me that one the biggest corps in the world has such an ugly UI. Note that I'm not saying it's necessarily bad - they've made it extremely easy to buy stuff with as few click as possible. It works, but as you say it's not pretty. But they have such a big infrastructure that they're probably constrained by legacy systems, and "If it aint broke, dont fix it" - but we're at a stage where we can do better. |
Here's 2 more examples. I made the bar 3 px instead of 2px. Bottom of HeaderBar 3px. Screen.Recording.2023-05-24.at.23.34.51.movBottom of Headbar 3px, inside main area. Screen.Recording.2023-05-24.at.23.56.38.mov |
Yeah, it's true that the page itself doesn't do anything. We used to have clear feedback in the status bar:
In my experience, browser loading indicators are still the way most users get feedback that their click did something. Of course, this doesn't apply to SPA, so we can't leverage it. "Avoiding flickering spinners" is certainly something we want to avoid. This PR is the canary in the coal mine telling us to solve this at the platform level, so it's good this has been highlighted clearly here. To move this particular PR forward, I think we should:
|
Alright I understand we need to take more time to introduce something like this. I can experiment with some animation timeouts for the blocking spinner in the meantime. Probably enough examples from me, but I want to mention that most mobile browsers use this kind of loader. Of course mobile is a bit different, but my point is that it shouldn't be foreign for users. screen-20230526-114002.2.mp4 |
This is based on https://github.com/tanem/react-nprogress , and the live example for react-router-v6. I've improved it abit, since that example used
CSSTransition
to manually update theloading
-state.Based on @tomzemp feedback on this PR: #327 (review)
Note that setting a
key
is the recommended way to use it, even though it might look at bit unusual?This introduces
react-nprogress
as a dependency.useNprogress
is very easy to useThis is what is looks like (throttled to slow 3g)
nprogress-bar.mov