uno.toolkit.ui icon indicating copy to clipboard operation
uno.toolkit.ui copied to clipboard

LoadingView enhancements

Open Xiaoy312 opened this issue 3 years ago • 4 comments

What would you like to be added:

  • include a default LoadingContent as part of default style
  • make default style a keyed one, and then aliased it as an implicit (unkeyed) one

Why is this needed: QoL

For which Platform: any

Anything else we need to know?

n/a

Xiaoy312 avatar Sep 26 '22 14:09 Xiaoy312

Two additional suggestions:

  • Change visual state from Loaded to NotLoading. The LoadingView has two states to indicate whether something is loading or not, so the intent is clearer if the visual state name is NotLoading (cc @francoistanguay for your thoughts)

  • Change LoadingContent to be a DataTemplate, rather than object to make it possible to define the LoadingContent in a style. The issue with defining an object is that you end up with a single instance that's created when resources are initialized. This causes issue with dependent resources not being able to be found (try this by setting LoadingContent to include a ProgressRing in the style for the LoadingView). Changing to a DataTemplate will allow it to be lazy loaded and will create a new instance for each time it's used (ie each instance of the LoadingView)

nickrandolph avatar Sep 28 '22 05:09 nickrandolph

ProgressRingTestApp.zip I've included a test app that shows two options for setting a ProgressRing in the LoadingContent of a LoadingView using a Style: Option 1 - ProgressRing directly - this fails (see code for error) Option 2 - Uses a ProgressRing inside a DataTemplate, which I assume causes a delayed evaluation

nickrandolph avatar Sep 28 '22 14:09 nickrandolph

Two additional suggestions:

  • [...]
  • Change LoadingContent to be a DataTemplate, rather than object to make it possible to define the LoadingContent in a style. The issue with defining an object is that you end up with a single instance that's created when resources are initialized. This causes issue with dependent resources not being able to be found (try this by setting LoadingContent to include a ProgressRing in the style for the LoadingView). Changing to a DataTemplate will allow it to be lazy loaded and will create a new instance for each time it's used (ie each instance of the LoadingView)

Regarding point 2, I would preserve both LoadingContent + LoadingContentTemplate so there is option to choose which to employ or both. Since if you had simply replaced that with a DataTemplate, we lose the access to the data-context at the LoadingControl level. (Ofc, you could use the AncestorBinding if you are on WinUI.) However, this introduce a problem if we were to add the default loading content where:

  • if we provide a default LoadingContent with the ProgressRing, that is a crash as you demonstrated
  • if we provide a default LoadingContentTemplate with the ProgressRing, the LoadingContent is ignored until the developer replace the LoadingContentTemplate too

Xiaoy312 avatar Sep 28 '22 15:09 Xiaoy312

I think if we at least add LoadingContentTemplate then at least a developer can set the loadingcontent as part of the Style.

Including the progressring in the default template is a lower priority and I think we can ignore it for the moment

nickrandolph avatar Sep 29 '22 03:09 nickrandolph