Added radial progress bar control #828
Conversation
| Hi @albertofustinoni, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! TTYL, DNFBOT; |
| @albertofustinoni, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
| { | ||
| this.DefaultStyleKey = typeof(RadialProgressBar); | ||
| | ||
| Foreground = Application.Current.Resources[DefaultForegroundColorBrushName] as SolidColorBrush; |
There was a problem hiding this comment.
These should not be set in code - should be set in your generic XAML definition so that it can be properly styled. For example:
<Setter Property="Foreground" Value="{StaticResourse SystemControlHighlightAccentBrush}" />
| <ControlTemplate TargetType="local:RadialProgressBar"> | ||
| <Grid> | ||
| <!-- Gray outline of progress bar --> | ||
| <Path Fill="Transparent" Stroke="{TemplateBinding Background}" StrokeThickness="{TemplateBinding Thickness}" StrokeDashCap="Flat"> |
There was a problem hiding this comment.
Typically you wouldn't use "Background" as the track background here, you'd create a separate DependencyProperty for it. As odd as it may be, Background should be template bound to the grid above this, and set as Transparent by default imo.
| }, | ||
| { | ||
| "Name": "RadialProgressBar", | ||
| "Type": "RadialProgressBarPage", |
| private const string DefaultForegroundColorBrushName = "SystemControlHighlightAccentBrush"; | ||
| private const string DefaultBackgroundColorBrushName = "SystemControlBackgroundBaseLowBrush"; | ||
| | ||
| private PathFigure OutlineFigure { get; set; } |
There was a problem hiding this comment.
These should be private fields, not properties
| /// <summary> | ||
| /// Identifies the Thickness dependency property | ||
| /// </summary> | ||
| public static readonly DependencyProperty ThicknessProperty = DependencyProperty.Register("Thickness", typeof(double), typeof(RadialProgressBar), new PropertyMetadata(4.0, ThicknessChangedHandler)); |
There was a problem hiding this comment.
Use nameof() instead a string for "Thickness".
| /// </summary> | ||
| public RadialProgressBar() | ||
| { | ||
| this.DefaultStyleKey = typeof(RadialProgressBar); |
There was a problem hiding this comment.
Remove usages of "this."
| <ControlTemplate TargetType="local:RadialProgressBar"> | ||
| <Grid> | ||
| <!-- Gray outline of progress bar --> | ||
| <Path Fill="Transparent" Stroke="{TemplateBinding Background}" StrokeThickness="{TemplateBinding Thickness}" StrokeDashCap="Flat"> |
| Addressed points raised so far |
| Question: what is supposed to happen with this PR? After fixing the issues that were raised I heard nothing... |
| Hey we will merge it but as we are closing 1.3 now I delayed this one for 1.4 |
| Got it, thank you. |
| I don't think it's a design requirement form Microsoft, this is my opinion as a user and developer. Visual consistency has always been an issue on Windows: I think a semi-official toolkit like this should try to be part of the solution instead of adding to the problem. |
| hello what is the status of this PR? Do we want to merge it with loading control? Close it? |
| I would like to know as well |
| @deltakosh we should leave the merging with Loading control as a next step. We can have just the progress control and include this as an option in loading control for 2.0 ? |
| @albertofustinoni can you please fix the .md file to use xml rather than XAML ? |
| @pedrolamas can you update your review? |
| Everything looks good to me! 😄 |
| @hermitdave If you mean in docs\controls\RadialProgressBar.md, it should be already changed to xml |
| @albertofustinoni my bad! |
| I like this control but what about an attached property to a progessBar? it is almost the same behavior. Only visual change. I assume a restyling is not enough but perhaps with an attached property? @skendrot just told me that you can inherit from ProgressBar. which could be even better |
| Deriving it from ProgressBar isn't a bad idea.. |
| PR creator here, posting from my alt. RadialProgressBar now inherits from ProgressBar. I have also synced with changes in upstream. |
| /// <summary> | ||
| /// Identifies the Outline dependency property | ||
| /// </summary> | ||
| public static readonly DependencyProperty OutlineProperty = DependencyProperty.Register(nameof(Outline), typeof(Brush), typeof(RadialProgressBar), new PropertyMetadata(new SolidColorBrush(Windows.UI.Colors.Transparent))); |
There was a problem hiding this comment.
Would it make sense to use the same property names as ProgressBar for the Background color? In other words, wondering if this property is needed?
| Maximum="100" | ||
| Width="@[Width:Slider:100:100-200]" | ||
| Height="@[Height:Slider:100:100-200]"/> | ||
| </Grid> |
There was a problem hiding this comment.
Would it make sense to also add the Color enum for the Outline(Background) and Foreground so the show up as properties?
| @albertofustinoni , what do you think about my comments? |
| I considered using the Background property to color the circular outline instead of creating a new dependency property, but doing so would mean that the actual background (rectangular pane containing the control) color could not be changed. To me, this seemed inconsistent with other controls, so I didn't go for it. Do you think doing so would be better? |
| @nmetulev as for adding colors to properties in the samples app, other controls didn't do it, so I didn't either. |
| Makes sense having the outline property, but in that case, we should have it as a property on the sample page so it can be discovered more easily. |
| Look at the SlidableListItem for an example of using the color enum in the sample app |
| It'll be nice to have also the possibility to change the direction of the progress. I can't help and make a pull request because I'm learning C#/XAML for UWP development. |
| @igorissen , that's a great suggestion, could you open a user voice entry to see if there is a larger need for this from the rest of the community? |
11 participants
Added a radial progress bar control, as described in issue #827