search

vaadin / flow

Vaadin Flow is a Java framework binding Vaadin web components to Java. This is part of Vaadin 10+.
Apache License 2.0
618 stars 167 forks source link

Custom loading indicator for long server requests #3763

Open jouni opened 6 years ago

jouni commented 6 years ago

When a server request takes a long time I want to display a custom loading indicator so that the end user knows that the app is still processing the request.

I also want the designer in my team to be in full control of how that custom loading indicator behaves.

Solution proposal

Apply a state attribute to the <body> element when a server request is active, f.e. <body flow-request-active> (name needs bikeshedding), without any delays.

Extract the current v-loading-indicator implementation into a separate deprecated component that can be added back to the app as an opt-in feature, f.e. myapp.add(new LegacyLoadingIndicator());. This would allow existing custom theme CSS to still work. The LoadingIndicatorConfiguration could be moved inside this component.

Optionally, design and implement a new opt-in loading indicator component specifically for the Lumo theme.

Write a tutorial how to create a custom loading indicator.

Legioth commented 6 years ago

A CSS based solution can surely be triggered by adding and removing a body attribute, but what if I want to also trigger some JS logic for something related to the progress indicator?

Question: are there different kinds of request?

In Flow, I think there's currently only one type of request. Vaadin 7 and 8 also has the concept of background requests that are completely transparent to the user (e.g. prefetching items to populate the client-side cache in Grid), and would therefore not cause anything to show.

jouni commented 6 years ago

A CSS based solution can surely be triggered by adding and removing a body attribute, but what if I want to also trigger some JS logic for something related to the progress indicator?

Can’t you add a MutationObserver to the body element in your custom loading indicator element?

manolo commented 6 years ago

I see easier, an annotation for defining what should be the localName of the loading indicator. Hence the logic about when to add/remove the indicator from body is still responsability of the framework. Developer responsibility is to import the appropriate custom element, which might have any logic you need.

My idea is something like 

@LoadingIndicatorHTML("<div class='v-loading-indicator'>'")`  // default implementation
@LoadingIndicatorHTML("<vaadin-progress-bar theme='foo' ></vaadin-progress-bar>")`
@LoadingIndicatorHTML("<paper-progress class='bar'><paper-progress>")`
or
@LoadingIndicatorTagName("div")
@LoadingIndicatorTagName("vaadin-progress-bar")
@LoadingIndicatorTagName("paper-progress")

First seems simpler to implement, and probably more customisable.

Lumo might have some css for div.v-loading-indicator that works out-of-the-box if the user does not want to customise anything.

jouni commented 6 years ago

@manolo, I think an annotation is unnecessarily complex. In my opinion, Flow should only toggle the attribute on the body element, and that’s it. All other API would be encapsulated in a “web component” that you place directly under the body element (like I described in the issue description).

By ”web component” I mean a simple custom element (f.e. <my-loading-indicator>) that adds CSS to the global scope and reacts to the attribute on the body. For example:

my-loading-indicator {
  opacity: 0;
  pointer-events: none;
  ...
}

[flow-request-active] my-loading-indicator {
  opacity: 1;
  animation-name: my-loading-indicator;
  animation-duration: 1s;
  animation-delay: 0.5s;
  animation-iteration-count: infinite;
  ...
}

Global CSS is arguably a bad practice. A MutationObserver, which toggles an attribute on the custom element host, might be a better option.

jouni commented 6 years ago

Having the attribute on the body element allows the developer to use that information in more custom ways than just one element that Flow adds/removes to/from the DOM. So it feels like a more low level, generic API in that way, not tied to an element specifically that you can only configure with an annotation.

manolo commented 6 years ago

Another option might be that the attribute is set to the active UI component in the app instead to the body, in this way developer would have more control in their app.

manolo commented 6 years ago

I don't have a strong preference about the implementation, whether the attribute is set to body or UI, but I think loading should be shown out-of-the-box like happens in FW8. The user just need to set Lumo theme

jouni commented 6 years ago

I think loading should be shown out-of-the-box like happens in FW8

I don’t completely agree.

I can agree that it’s probably better than nothing.

But, in my view, the default loading indicator (the dummy progress bar at the top of the viewport) is an easy excuse for the designers and developer to not implement better feedback mechanisms to the end user (like instantly updating the UI with a skeleton screen and then progressively loading the content).

That said, it might be better to have the default loading indicator as opt-out instead of opt-in.

My suggestion to have it opt-in was because I wanted to get rid of the current default indicator ASAP without having to design and implement a replacement at the same time. But I suppose we can keep the current default loading indicator as long as we have a new one designed and implemented, and then deprecate the old one somehow.

jouni commented 6 years ago

Another option might be that the attribute is set to the active UI component in the app instead to the body

I thought the body element corresponds to the UI component.

jouni commented 6 years ago

Slightly related: when the view Flow is trying to render is complex enough, the UX degrades somewhat.

As an end user, I click a link to move to another view. The server round-trip takes enough time for the loading indicator to kick in (~300ms) but not enough time for the loading indicator to really animate and show any amount of progress. It either doesn’t show at all or just halts right in the beginning. Then Flow starts to process the response and render the UI update, which blocks rendering. Once the rendering finishes, the loading indicator also flashes from 0 -> 100 immediately.

Not sure how to mitigate that, but it doesn’t feel like great UX.

mstahv commented 6 years ago

I strongly agree with @manolo. The request may take longish for several reasons and in certain cases only occasionally. We certainly don't want to force developers to add custom css for each and every app. In certain cases developers definitely should show a progress indicator with meaningful text what is being done, but if the request is slow due to a temporary network outage, there is no way how Java developers could show a progress indicator.

jouni commented 6 years ago

We certainly don't want to force developers to add custom css for each and every app.

I can also agree with that, very much (that’s one reason I created this issue).

The implementation we have in Framework 8 and before is very rigid and unnecessarily configured from the server. And the progress bar implementation is ugly, hacky CSS. I would’ve have liked us to use this new major version as an opportunity to improve on this. But I suppose it’ll wait until the next major version then. If that’s the case, then I suppose we can just as well throw in the old messy CSS as well by default. But then we need a tutorial how to get rid of that.

In my view, it would be better to encapsulate the default loading indicator to a component that can be removed from the UI is wanted (opt-out) and have lower level hooks (like the <body> attribute) that allow developers to create their own loading indicator if needed. You kind of described the need, that in some cases there could be a need for a customized progress indicator.

mstahv commented 6 years ago

Better (both from looks and technical PoV) would of course be better, here any in many other places. An ugly one (by default) that can be overridden if needed is much better than nothing.

jouni commented 6 years ago

Yes, won’t argue against that either. I never intended to block us from having a default loading indicator. Just hoped we would’ve had time for ”better” before V10 😅

pleku commented 6 years ago

The same indicator as in Vaadin 8 has been brought back in #4174 and is by default on. It can be opt-out if necessary from the LoadingIndicatorConfiguration (which it also fixes to work).

Braus commented 5 years ago

Is still issue still active?, I see there hasn't been any activity for a long time, so I was just wondering.

pleku commented 5 years ago

There is plans on the roadmap for changing the current loading indicator behavior https://vaadin.com/docs/v13/flow/advanced/tutorial-loading-indicator.html