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.

Sign up for GitHub

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

GridSplitter: Make IsTabStop work and simplify control's visual tree #1145

Merged
merged 3 commits into from
May 30, 2017
Merged

GridSplitter: Make IsTabStop work and simplify control's visual tree #1145

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9226023
Remove GridSplitterGripper to allow control of tab behavior and simpl…
lukasf May 2, 2017
3e6070d
GridSplitter: Rework according to review commments.
lukasf May 7, 2017
e63ba1e
GridSplitter: Add missing comment
lukasf May 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 27 additions & 22 deletions Microsoft.Toolkit.Uwp.UI.Controls/GridSplitter/GridSplitter.Events.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@
using Windows.System;
using Windows.UI.Core;
using Windows.UI.Xaml;
using Windows.UI.Xaml.Controls;
using Windows.UI.Xaml.Input;
using Windows.UI.Xaml.Media;

namespace Microsoft.Toolkit.Uwp.UI.Controls
{
Expand All @@ -22,31 +24,23 @@ namespace Microsoft.Toolkit.Uwp.UI.Controls
/// </summary>
public partial class GridSplitter
{
// Symbols for GripperBar in Segoe MDL2 Assets
private const string GripperBarVertical = "\xE784";
Copy link
Contributor

Choose a reason for hiding this comment

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

These styles should be applied within the control style and visual states instead of here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be possible to move the gripper content into the control template and use visual states for selecting the right content. But then it would always be shown and we would lose the functionality of easily overriding the gripper content by using the "Element" property. This would be a breaking change. We would have to remove the Element property, and our users would instead have to override the control template for custom content. I do not think that this is a good idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the control allows for content then it should be a ContentControl. I would suggest changing to a ContentControl, mark Element as deprecated and when it changes set the content to be the value. Default the content to be the style proposed

Copy link
Member

Choose a reason for hiding this comment

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

I tend to agree with Shawn it should have been a content control from the beginning (but of course not my bad :P )
We can do it right with a small change, just moving the textbox to the grid inside the control template and set the textbox value based on orientation on splitter loaded.
I see no need to move GripperBarVertical to visual state as it should be used only twice once on splitter loaded and when the splitter orientation change to update the gripper with the new value (which is not currently implemented)

Copy link
Member

Choose a reason for hiding this comment

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

Let's do the right thing :)

Copy link
Member

Choose a reason for hiding this comment

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

The nearest case for the splitter is Button control wish is a ContentControl that allows custom content to change the way it looks :)
Anyway, I see we can consider moving the splitter from using Element to ContentControl in V2 so we don't have to use DEPRECATED keyword with a change that doesn't add extra functionality.
Will review the PR this weekend.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see it like that. A button's main purpose is to show content and allow you to click it. Each button in your UI will usually show different content. So unlike the GridSplitter, showing custom content is inherent part of the button's functionality. Much like a checkbox, which is there to show content and allow you to check it. The gripper is more like the checkmark of a checkbox. It is part of the style which is usually the same for all instances of the control. It is not like the content of a checkbox.

But well, just because I don't think it should be a ContentControl, that surely does not mean you cannot make it one :) It's just my opinion on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@skendrot It would be great if you could approve the PR. Please see the discussion above. If someone wants to do a redesign of the GridSplitter control, please create a separate issue/PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@skendrot I juse see that @IbraheemOsama has already created an issue for changing the control to ContentControl. Then my PR should be ready to go?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with waiting to change the control. However:
If we plan to keep the concept that it will allow custom content (via the Element property or by making it a ContentControl), then let's embrace that now. Instead of having an internal textblock, set that to be the Element property and change the text based on a visual state.

Or do we want to remove the capability to allow custom content (eg: Remove Element property and do not change to be a ContentControl), which I think is also a valid solution and is how the WPF GridSplitter behaved. This option would probably need to be done in the 2.0 release.

private const string GripperBarHorizontal = "\xE76F";
private const string GripperDisplayFont = "Segoe MDL2 Assets";

private void GridSplitter_Loaded(object sender, RoutedEventArgs e)
{
_resizeDirection = GetResizeDirection();
_resizeBehavior = GetResizeBehavior();

GridSplitterGripper gripper;

// Adding Grip to Grid Splitter
if (Element == default(UIElement))
{
gripper = new GridSplitterGripper(
_resizeDirection,
GripperForeground);
}
else
{
var content = Element;
Element = null;
gripper = new GridSplitterGripper(content, _resizeDirection);
CreateGripperDisplay();
Element = _gripperDisplay;
}

Element = gripper;

gripper.KeyDown += Gripper_KeyDown;

var hoverWrapper = new GripperHoverWrapper(
CursorBehavior == SplitterCursorBehavior.ChangeOnSplitterHover
? this
Expand All @@ -60,23 +54,32 @@ private void GridSplitter_Loaded(object sender, RoutedEventArgs e)
_hoverWrapper = hoverWrapper;
}

private void Gripper_KeyDown(object sender, KeyRoutedEventArgs e)
private void CreateGripperDisplay()
{
var gripper = sender as GridSplitterGripper;

if (gripper == null)
if (_gripperDisplay == null)
Copy link
Member

Choose a reason for hiding this comment

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

No need for this check as _gripperDisplay will always be null if this function is reached

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Loaded event can fire multiple times. It is called each time a control is (re-)added to the visual tree. For example if you use a Frame with page caching and navigate to a different page, the old page content will get Unloaded event and then is put into cache. When moving back to the old page, it is re-added to the visual tree and so it will get Loaded event again, for the second time. In that case, our _gripperDisplay already exists, no need for us to re-create it.

{
return;
_gripperDisplay = new TextBlock
{
FontFamily = new FontFamily(GripperDisplayFont),
HorizontalAlignment = HorizontalAlignment.Center,
VerticalAlignment = VerticalAlignment.Center,
Foreground = GripperForeground,
Text = _resizeDirection == GridResizeDirection.Columns ? GripperBarVertical : GripperBarHorizontal
};
}
}

/// <inheritdoc />
protected override void OnKeyDown(KeyRoutedEventArgs e)
{
var step = 1;
var ctrl = Window.Current.CoreWindow.GetKeyState(VirtualKey.Control);
if (ctrl.HasFlag(CoreVirtualKeyStates.Down))
{
step = 5;
}

if (gripper.ResizeDirection == GridResizeDirection.Columns)
if (_resizeDirection == GridResizeDirection.Columns)
{
if (e.Key == VirtualKey.Left)
{
Expand All @@ -95,7 +98,7 @@ private void Gripper_KeyDown(object sender, KeyRoutedEventArgs e)
return;
}

if (gripper.ResizeDirection == GridResizeDirection.Rows)
if (_resizeDirection == GridResizeDirection.Rows)
{
if (e.Key == VirtualKey.Up)
{
Expand All @@ -112,6 +115,8 @@ private void Gripper_KeyDown(object sender, KeyRoutedEventArgs e)

e.Handled = true;
}

base.OnKeyDown(e);
}

/// <inheritdoc />
Expand Down
9 changes: 6 additions & 3 deletions Microsoft.Toolkit.Uwp.UI.Controls/GridSplitter/GridSplitter.Options.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

using Windows.UI.Core;
using Windows.UI.Xaml;
using Windows.UI.Xaml.Controls;
using Windows.UI.Xaml.Media;

namespace Microsoft.Toolkit.Uwp.UI.Controls
Expand Down Expand Up @@ -180,11 +181,13 @@ public SplitterCursorBehavior CursorBehavior
private static void OnGripperForegroundPropertyChanged(DependencyObject d, DependencyPropertyChangedEventArgs e)
{
var gridSplitter = (GridSplitter)d;
var grip = gridSplitter.Element as GridSplitterGripper;
if (grip != null)

if (gridSplitter._gripperDisplay == null)
{
grip.GripperForeground = gridSplitter.GripperForeground;
return;
}

gridSplitter._gripperDisplay.Foreground = gridSplitter.GripperForeground;
}

private static void OnGripperCursorPropertyChanged(DependencyObject d, DependencyPropertyChangedEventArgs e)
Expand Down
1 change: 1 addition & 0 deletions Microsoft.Toolkit.Uwp.UI.Controls/GridSplitter/GridSplitter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ public partial class GridSplitter : Control
private GridResizeDirection _resizeDirection;
private GridResizeBehavior _resizeBehavior;
private GripperHoverWrapper _hoverWrapper;
private TextBlock _gripperDisplay;

/// <summary>
/// Gets the target parent grid from level
Expand Down
6 changes: 4 additions & 2 deletions Microsoft.Toolkit.Uwp.UI.Controls/GridSplitter/GridSplitter.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,14 @@
xmlns:local="using:Microsoft.Toolkit.Uwp.UI.Controls">

<Style TargetType="local:GridSplitter">
<Setter Property="IsTabStop" Value="False"></Setter>
<Setter Property="IsTabStop" Value="True"></Setter>
<Setter Property="UseSystemFocusVisuals" Value="True"></Setter>
<Setter Property="IsFocusEngagementEnabled" Value="True"></Setter>
<Setter Property="Template">
<Setter.Value>
<ControlTemplate TargetType="local:GridSplitter">
<Grid Background="{TemplateBinding Background}">
<ContentPresenter Content="{TemplateBinding Element}" />
<ContentPresenter Content="{TemplateBinding Element}" HorizontalContentAlignment="Stretch" VerticalContentAlignment="Stretch" />
</Grid>
</ControlTemplate>
</Setter.Value>
Expand Down
85 changes: 0 additions & 85 deletions Microsoft.Toolkit.Uwp.UI.Controls/GridSplitter/GridSplitterGripper.cs
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion Microsoft.Toolkit.Uwp.UI.Controls/Microsoft.Toolkit.Uwp.UI.Controls.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@
<Compile Include="GridSplitter\GridSplitter.Events.cs" />
<Compile Include="GridSplitter\GridSplitter.Helper.cs" />
<Compile Include="GridSplitter\GridSplitter.Options.cs" />
<Compile Include="GridSplitter\GridSplitterGripper.cs" />
<Compile Include="GridSplitter\GripperHoverWrapper.cs" />
<Compile Include="HamburgerMenu\MenuItems\HamburgerMenuItem.cs" />
<Compile Include="HamburgerMenu\MenuItems\HamburgerMenuGlyphItem.cs" />
Expand Down