Skip to content
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

Mamatt/progress ring determinate #93

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
2 changes: 1 addition & 1 deletion active/progress/ProgressRing.md
Original file line number Diff line number Diff line change
Expand Up @@ -167,4 +167,4 @@ that isn't necessary to understand the purpose and usage of the API.
For example, implementation details. -->

# Open Questions
* How should the edge cases of 0%, 1%, 98%, 99% be visualized? We are currently discussing with other teams at Microsoft how to visualize this with the rounded interior corners.
* How should the edge cases of 0%, 1%, 98%, 99% be visualized? We are currently discussing with other teams at Microsoft how to visualize this with the rounded interior corners.
146 changes: 146 additions & 0 deletions active/progress/ProgressRing2.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
# Background
>See proposal documents for:
>
> [determinate mode for ProgressRing](https://github.com/microsoft/microsoft-ui-xaml/issues/688)
>
> [ProgressRing style](https://github.com/microsoft/microsoft-ui-xaml/issues/837)
>
> [Guidance for Progress controls](https://github.com/microsoft/microsoft-ui-xaml/issues/880)

Progress controls indicate to a user that an operation is occuring.
Xaml has two controls for this:
* the [ProgressBar](https://docs.microsoft.com/uwp/api/Windows.UI.Xaml.Controls.ProgressBar)
control, which shows progress on a rectangle
* and the
[ProgressRing](https://docs.microsoft.com/uwp/api/Windows.UI.Xaml.Controls.ProgressRing)
control, which shows progress on a circle.

Progress _Bar_ has both a determinate and indeterminate mode.
In determine mode the rectangle goes from empty to full, in indeterminate mode it shows an animation.
Progress _Ring_ currently only has an indeterminate mode, but this spec is adding an indeterminate mode.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Progress _Ring_ currently only has an indeterminate mode, but this spec is adding an indeterminate mode.
Progress _Ring_ currently only has an indeterminate mode, but this spec is adding a determinate mode.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommend: please


ProgressBar is also getting a feature to replaces its built-in visualization (a spinning ring)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ProgressBar is also getting a feature to replaces its built-in visualization (a spinning ring)
ProgressRing is also getting a feature to replaces its built-in visualization (a spinning ring)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommend: please

with a custom animation defined as an
[IAnimatedVisualSource](https://docs.microsoft.com/uwp/api/Microsoft.UI.Xaml.Controls.IAnimatedVisualSource),
which is known as a "Lottie Animation", and already supported (playable by) an
[AnimatedVisualPlayer](https://docs.microsoft.com/uwp/api/Microsoft.UI.Xaml.Controls.AnimatedVisualPlayer).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably, the animation's Progress is 0 when the ProgressRing value is equal to the minimum value, is 1 when the ProgressRing value is equal to the maximum value, and linearly interpolates between the two extremes?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommend: explain this more clearly, that progress from Minimum to Maximum updates Progress from 0 to 1.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the control first appears on the screen, does it start at its initial Progress? Or does it animate up from zero?

When the progress ring's value changes, does the animation play until the new Progress is reached, and then pause?

What happens if the Progress goes backward? Do we play the animation backward?

Animating the Progress backward may not be appropriate for certain animations. E.g., a cup being filled with water from a faucet sends water against gravity back into the faucet.

If the Progress resets from 0.99 to 0.0 (e.g., because we just finished Phase 1 and are about to start Phase 2), the developer most likely wants the animation to jump-cut to 0 rather than animating backward to it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • When the control first appears on the screen, say at 50%, does it show that or animate that?
    Recommend: show (not animate)

  • When Value changes it animates over some default duration. The animation has a natural duration and plays at its natural speed
    Recommend: explain the details. Should we make it configurable (like AVP's PlaybackRate)?

  • Reducing Value makes it loop forward and around back to the new value.
    ProgressBar jumps backward but animates forward, ProgressRing animates forward
    Recommend: update to animate backwards?

  • How do you reset (0.99 to 0)? Should there be a hard cut feature?
    Consider

Recommend: ProgressRing should work like ProgressBar.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When Value changes it animates over some default duration. The animation has a natural duration and plays at its natural speed

Recommend: explain the details. Should we make it configurable (like AVP's PlaybackRate)?

There should probably be a set short duration, with an easing as it reaches the value. If it was a constant playback rate it could be frustrating waiting for the animation/transition to conclude. The wider the gap between current value and new value, the faster the animation would be.

Reducing Value makes it loop forward and around back to the new value.
ProgressBar jumps backward but animates forward, ProgressRing animates forward

Recommend: update to animate backwards?

It would be useful to expose the duration for the animated transitions as a StaticResource, so the developer could choose to set it to 0 and have instant changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once you replace the animation, there's really no difference between ProgressRing and ProgressBar is there?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the custom animation case be a different control?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussion: we already have ProgressBar and ProgressRing. We've discussed having ProgressUI to unify all three, but it got to be a complicated API.

Consider: move the custom source to a new class, rather than adding it to an existing class.


# ProgressRing Description
Represents a control that indicates that an operation is ongoing.
The typical visual appearance is a ring-shaped "spinner" that animates a filled area as progress continues.

By default, ProgressRing will display an indeterminate spinner that continuously
animates until the control is no longer visible ?? or
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfinished wordsmithing. Suggest

... that continuously animates as long as the IsActive property is true.

No need to say that it animates only when visible. Collapsed items cannot animate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommend: it will animate until it's IsActive is false or Visibility is collapsed.

[ProgressRing.IsActive](https://docs.microsoft.com/uwp/api/Microsoft.UI.Xaml.Controls.ProgressRing.IsActive)
is set to false. You can change the behavior to display
a determinate ring that fills in based on the value you provide, using the `IsIndeterminate` property.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
a determinate ring that fills in based on the value you provide, using the `IsIndeterminate` property.
a determinate ring that fills in based on the value you provide, by setting the `IsIndeterminate` property to `false` (default `true`).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommend: please


You can also replace the built-in animation shown by the ProgressRing with a custom one,
using the `DeterminateSource` and `IndeterminateSource` properties.

# Examples

The following examples show how to use the `IsIndeterminate` property to change the mode of
the ProgressRing and the Value property to change the proportionate amount indicated.

## Indeterminate ProgressRing

By default ProgressRing is indeterminate (a spinning circle).

> Issue: not the same without an AniGif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Internal editing comment needs to be addressed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommend: add a picture


```xml
<ProgressRing IsActive="True" />
```
![](images/ProgressRing-indeterminate.jpg)

## Determinate ProgressRing

When `IsIndeterminate` is set, the circle will show progress around the ring,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
When `IsIndeterminate` is set, the circle will show progress around the ring,
When `IsIndeterminate` is `false`, the circle will show progress around the ring,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommend: please

filling it when Value gets to Maximum.

```xml
<ProgressRing IsActive="True" IsIndeterminate="False"
Minimum="32" Maximum="212" Value="{x:Bind LiquidTemp}" />
```
![](images/ProgressRing-determinate.PNG)


## Custom animation

Show progress ring that visually looks like a thermometer boiling over.
See [AnimatedVisualPlayer](https://docs.microsoft.com/uwp/api/Microsoft.UI.Xaml.Controls.AnimatedVisualPlayer)
for more information on animated visuals.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that everybody who uses a ProgressRing, even if they just use the standard animation, still has to pull in all the code for a Lottie player?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussion: Yes, the internal implementation of the ring uses a Lottie animation.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussion: Yes, the internal implementation of the ring uses a Lottie animation.

Wasn't there a way to use standard Xaml animations via re-templating?


> Picture would be nice
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please clean up internal comments.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommend: add a picture


```xml
<ProgressRing IsActive="True" >
<ProgressRing.IndeterminateSource>
<animatedvisuals:LottieProgress />
</ProgressRing.IndeterminateSource>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A progress ring with an indeterminate source is just an AnimatedVisualPlayer, isn't it? What's the value of wrapping it inside a ProgressRing? Accessibility? Uniformity?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussion: Accessibility

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If an indeterminate ProgressRing becomes inactive, what happens to the animation? Is it paused at its current position? Does it rewind to 0? Does it become invisible (but occupy space)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussion: Will keep the current ProgressRing behavior; it will still take up layout space but will become transparent

</ProgressRing>
```

# API Notes

ProgressRing can now be used in scenarios where progress is determinate or indeterminate.
With this additional capability,
the determinate ProgressRing does not represent a "hung" state where the end user cannot interact
with the app.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change in guidance doesn't appear to be related to the new determinate capability.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussion: it's not motivated by this feature, the feature was motivated by the new guidance.

Recommend: move this to the guidance doc.


Previously, guidance recommended that ProgressRing only be used when the user cannot continue to interact with the app,
but this is no longer the only use case and ProgressRing can be used in scenarios where user interaction can continue
while the ring is spinning. An example of this would be when a video is loading on a page and a progress ring is displayed
while the media is loading. The end user can still interact with other UI elements on the page while they wait
for the media to load.

In addition to the updated guidance, the sources for the indeterminate and determinate mode have been exposed so you can
choose the animation file that can be played. This will give you the opportunity to customize the look and feel of your
progress ring and potentially have different animations for determinate vs. indeterminate.

|Name | Type | Description | Default |
|:--|:-:|:-:|:-:|
| IsIndeterminate | boolean | Gets or sets a value that indicates whether the progress ring reports generic progress with a repeating pattern (indeterminate progress) or reports progress based on the Value property (determinate progress) | True |
| IsActive | boolean | Gets or sets a value that indicates whether the progress ring is being displayed to the end user. If IsActive is false, the ProgressRing is not shown, but space is reserved for it in the UI layout. To not reserve space for it, set its Visibility property to Collapsed. | True |
| Value | double | Gets or sets the current setting of the range control, which may be coerced. The value ranges from 0 to 100??. Has no effect when `IsIndeterminte` is true. | 0-100 |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incomplete editing (question marks still in document).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussion: see below comment

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "Default = 0-100" mean? Is it 0? 100?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommend: remove this and add rows that Minimum defaults to 0, Maximum defaults to 100. (Value defaults to zero)

| Maximum | double | Gets or sets the highest possible `Value` of the range element. Has no effect when `IsIndeterminate` is true.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the defaults for Minimum and Maximum?

| Minimum | double | Gets or sets the lowest possible `Value` of the range element. Has no effect when `IsIndeterminate` is true.
| DeterminateSource | IAnimatedVisualSource | Gets or sets the animation file used for the determinate state of the ring. | N/A |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does N/A mean? If I read the DeterminateSource property on a freshly-constructed ProgressRing, what do I get? I'm guessing null.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommend: null

| IndeterminateSource | IAnimatedVisualSource | Gets or sets the animation file used for the indeterminate state of the ring. | N/A |


# API Details

```xaml
[webhosthidden]
unsealed runtimeclass ProgressRing : Windows.UI.Xaml.Controls.Control
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How horrible would it be if we just made ProgressRing derive from RangeBase?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would bring in LargeChange and SmallChange APIs, which make no sense on ProgressRing. And there's no polymorphism scenario to motivate a common case class.

{
// ... (existing APIs)

Boolean IsIndeterminate{ get; set; };

IAnimatedVisualSource DeterminateSource{ get; set; };
IAnimatedVisualSource IndeterminateSource{ get; set; };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: Sometimes read/write properties are expressed as Blah { get; set;} and sometimes as just Blah. Please be consistent with the rest of WinUI on this stylistic choice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommend: update


Double Value;
Double Minimum;
Double Maximum;

static Windows.UI.Xaml.DependencyProperty IsIndeterminateProperty{ get; };
static Windows.UI.Xaml.DependencyProperty DeterminateSourceProperty{ get; };
static Windows.UI.Xaml.DependencyProperty IndeterminateSourceProperty{ get; };
static Windows.UI.Xaml.DependencyProperty ValueProperty{ get; };
static Windows.UI.Xaml.DependencyProperty MinimumProperty{ get; };
static Windows.UI.Xaml.DependencyProperty MaximumProperty{ get; };
}
```


# Appendix

## Design Behavior Details
The determinate progress ring will be an empty ring when the progress is set to a value of 0.
When value is set to 1, a blue dot will appear and then continue to fill as the value increases.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value is a Double, so is this really saying "any nonzero value", like ½?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be when the Value is set to Minimum?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussion: Point is that should only be empty at 0 and full at 100.

Recommend: update

When the value is set to 98 or 99, the ring will not be completely filled
to indicate to the user that progress is not complete.
Binary file removed active/progress/images/ProgressRing-indeterminate.PNG
Binary file not shown.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.