-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
UI Update: WIP #2013
UI Update: WIP #2013
Conversation
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.
Did a quick review.
If you are making changes in the actual control and not just the template, make sure to test your changes with the previous template as some developers might still use it.
Also, make sure your changes work on creators update and above
var tb = new TextBlock { Text = Maximum.ToString() }; | ||
tb.Measure(new Size(Double.PositiveInfinity, Double.PositiveInfinity)); | ||
|
||
_minValueText.MinWidth = tb.ActualWidth; |
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.
If someone is using the old template, this might crash - make sure these changes work with the old template
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.
This is the one David updated. We've had some difficulties updating the controls to meet the designs while also not breaking functionality. The redlines weren't very specific about what was part of the control, and was is part of the sample page, and many of the controls had bugs to boot.
I'll test it out though :)
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.
I added some null checks around the tooltip and min/max value textblocks code. This allows the old template to continue to work with the new logic. 👍
@@ -3,7 +3,56 @@ | |||
xmlns:controls="using:Microsoft.Toolkit.Uwp.UI.Controls" | |||
xmlns:local="using:Microsoft.Toolkit.Uwp.UI.Controls"> | |||
|
|||
<AcrylicBrush x:Key="SystemControlChromeWhiteAcrylicElementBrush" |
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.
AcrylicBrush is not available in RS2 (our min version).
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.
Fixed.
I put the fallback color in the xaml template and placed the AcrylicBrush code in the OnApplyTemplate override with an ApiInformation type check.
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.
Unfortunatly, as I mentioned above, adding it in OnApplyTempalate would overwrite any background the user sets.
…nsure old template still works with new logic
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.
The PROPERTIES header of every sample is cut of - I assume because of the grid splitter update.
@@ -12,20 +12,17 @@ | |||
<controls:Expander x:Name="Expander1" VerticalAlignment="Top" Margin="0,0,0,10" |
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.
-
The expander no longer fully collapses. For example, change the second expander direction to
Down
and collapse it. The expander should completely collapse into the header. -
The background property should also change the background of the header when collapsed.
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.
Issue 1 is fixed, but issue 2 conflicts with the design redlines a little bit. I did my best to compensate though. Take a look and let me know what you think.
<Style TargetType="Border"> | ||
<Setter Property="BorderThickness" Value="1,1,0,0" /> | ||
<Setter Property="Padding" Value="16" /> | ||
<Setter Property="BorderBrush" Value="{StaticResource Brush-Grey-04}" /> |
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.
Don't use Brush-Gray-04
. Needs to be something that would work when copied to a blank app.
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.
Fixed
</ResourceDictionary> | ||
|
||
<Style TargetType="MenuFlyoutItem"> |
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.
Should we add this as a Style in the toolkit? Seems to be something others would like to reuse. @michael-hawker what do you think?
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.
I literally ripped this from the MenuFlyoutItem template in the generic.xaml for 10.0.17110.0, as it appears to be what Ani based her design on. I didn't write it, but it made it pretty easy to implement the design.
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.
So this is a new style that shipped in RS4? @nmetulev do we need to get buy-in to ship that as part of the toolkit? If it works down-level to RS2 still, then it could be useful to add.
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.
Hmm, great question. Let's not include it for now and we can investigate this once we merge this PR.
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.
Sounds good
@@ -13,7 +13,119 @@ | |||
<commands:NewFileCommand x:Key="NewFile" /> |
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.
The two actions on this page are broken.
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.
Fixed
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.
The actions add a an item to the view menu instead of the file menu. Can you change them so they add items to the file menu?
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.
+1 have a 'Recent Files' type submenu maybe these can appear in. Otherwise it breaks the View interaction.
MaxItemSize="@[MaxItemSize:Slider:60:50-100]" | ||
MinItemSize="@[MinItemSize:Slider:28:10-50]" | ||
MaxItemSize="@[MaxItemSize:Slider:40:40-100]" | ||
Height="400" |
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 new height?
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.
At 400px, the math aligns to be more like the redlines. If I allow the OrbitView to scale with the container, the items won't be the correct ratio to the rest of control.
@@ -549,13 +556,27 @@ | |||
<ContentControl Grid.Row="1" Grid.RowSpan="1" Grid.Column="0" Grid.ColumnSpan="2" | |||
x:Name="PART_ContentOverlay" | |||
Content="{TemplateBinding ContentOverlay}" | |||
Background="{ThemeResource SystemControlPageBackgroundChromeLowBrush}" | |||
MinHeight="120" |
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.
I think this minheight is messing with the collapsed state
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.
It was. I fixed it ;)
@@ -122,6 +123,17 @@ protected override void OnApplyTemplate() | |||
Loaded += Menu_Loaded; | |||
Unloaded += Menu_Unloaded; | |||
|
|||
if (ApiInformation.IsTypePresent("Windows.UI.Xaml.Media.AcrylicBrush")) | |||
{ | |||
Background = new Windows.UI.Xaml.Media.AcrylicBrush() |
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.
This would overwrite any background the user sets. You might be able to use conditional XAML in the template to achieve this effect.
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.
Fixed
@@ -3,7 +3,56 @@ | |||
xmlns:controls="using:Microsoft.Toolkit.Uwp.UI.Controls" | |||
xmlns:local="using:Microsoft.Toolkit.Uwp.UI.Controls"> | |||
|
|||
<AcrylicBrush x:Key="SystemControlChromeWhiteAcrylicElementBrush" |
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.
Unfortunatly, as I mentioned above, adding it in OnApplyTempalate would overwrite any background the user sets.
|
||
<Border x:Name="OutOfRangeContentContainer" | ||
Background="Transparent"> | ||
<TextBlock Grid.Column="0" x:Name="MinValueText" Text="Min"/> |
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.
This is a big breaking change. Should we have a property to show the min/ax text as part of the control?
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.
Feels like the default style should match the Slider (just with the extra thumb). This could be an alternate style though. Feel like it could just use binding to get those values, no?
I should have all comments addressed, minus the RangeSelector. After much feedback and some convo with Nikola, I am going to restyle the RangeSelector to be more inline with the Slider control. This was missed in the initial design review, but seems like the best way to go. |
</Grid> | ||
|
||
<controls:Expander.ContentOverlay> | ||
<Grid Background="Red" MinHeight="250"> | ||
<Grid Height="120"> |
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.
We should stick with MinHeight
here as otherwise it looks a bit strange if the expand direction is left or right.
Header="^Build"> | ||
<MenuFlyoutItem Text="Build Solution" /> | ||
<MenuFlyoutItem Text="Rebuild Solution" /> | ||
<interactivity:Interaction.Behaviors> |
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.
I like having the single item without a dropdown in the sample to show how to do this behavior. However, I can't activate it with the keyboard. So I think we also need to hook into the KeyDown event for the Enter key?
@@ -13,7 +13,119 @@ | |||
<commands:NewFileCommand x:Key="NewFile" /> |
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.
+1 have a 'Recent Files' type submenu maybe these can appear in. Otherwise it breaks the View interaction.
@@ -133,7 +140,7 @@ protected override void OnNavigatedTo(NavigationEventArgs e) | |||
Shell.Current.RegisterNewCommand("Show notification with buttons (with DataTemplate)", (sender, args) => |
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.
Hmm, I still can't dismiss this one (with DataTemplate) with the Dismiss action and the 'X' doesn't close it either.
@@ -133,7 +140,7 @@ protected override void OnNavigatedTo(NavigationEventArgs e) | |||
Shell.Current.RegisterNewCommand("Show notification with buttons (with DataTemplate)", (sender, args) => |
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.
It also seems to ignore the Notification duration
@@ -8,6 +8,7 @@ | |||
</ResourceDictionary.MergedDictionaries> |
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.
Just a heads up that @IbraheemOsama's Infinite Canvas uses the TextToolbar as well I believe, so not sure how these changes effect that (if at all).
Issue: #2012
The following controls will be updated with a more fluent and consistent default templates:
PR Checklist
Please check if your PR fulfills the following requirements: