-
Notifications
You must be signed in to change notification settings - Fork 2
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
GridSplitter changes, rebased on top of OG GridSplitter PR #3
base: user/mhawker/gridsplitter
Are you sure you want to change the base?
GridSplitter changes, rebased on top of OG GridSplitter PR #3
Conversation
Content Sizer Issues: - [ ] TargetControl isn't loaded yet causes problems (Expander Issue) - [ ] Size Direction needs to be more specific (only works Left/Top, not Right/Bottom) - [ ] Do we support 'Auto'? - [ ] Need to set Automation Property Name in Code-Behind - [ ] Content Initial value as binding converter to ResizeDirection?
Bonus also fixes issue with sample loading the first time...
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.
@Arlodotexe not sure if you want to take a look at any of this yet or not either, FYI.
@XAML-Knight coming together! I'm also wondering at minimum with the keyboard events, but maybe somehow for the mouse events too, if we can push those into the subclass and virtualize the Horizontal/VerticalMovement methods for keyboard, if they can't share them. Seems like there's a bit more overlap here we may be able to capture between the two implementations until it splits into needing to understand a Grid
vs. resizing a general control. Thoughts?
Thanks!
_gripperDisplay.SetValue( | ||
Windows.UI.Xaml.Automation.AutomationProperties.AccessibilityViewProperty, | ||
Windows.UI.Xaml.Automation.Peers.AccessibilityView.Raw); | ||
GripperCursor = _resizeDirection == GridResizeDirection.Columns ? CoreCursorType.SizeWestEast : CoreCursorType.SizeNorthSouth; |
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.
Wouldn't we still want the gripper cursor even if the content is set by the dev?
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.
Indeed - change made.
Microsoft.Toolkit.Uwp.UI.Controls.Layout/GridSplitter/GridSplitter.xaml
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI.Controls.Layout/GridSplitter/SplitBase.cs
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI.Controls.Layout/GridSplitter/SplitBase.cs
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI.Controls.Layout/GridSplitter/SplitBase.cs
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI.Controls.Layout/GridSplitter/SplitBase.cs
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI.Controls.Layout/GridSplitter/GridSplitter.Options.cs
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI.Controls.Layout/GridSplitter/GridSplitter.Events.cs
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI.Controls.Layout/GridSplitter/GridSplitter.cs
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI.Controls.Layout/ContentSizer/ContentSizer.Events.cs
Outdated
Show resolved
Hide resolved
Just from a pure talking point, yes, it would make sense to try to genericize any kind of keyboard resizing operations, in a base/subclass. |
Microsoft.Toolkit.Uwp.UI.Controls.Layout/GridSplitter/GridSplitter.Data.cs
Show resolved
Hide resolved
@@ -85,14 +84,6 @@ | |||
<controls:GridSplitter.RenderTransform> | |||
<TranslateTransform Y="-8" /> | |||
</controls:GridSplitter.RenderTransform> | |||
<controls:GridSplitter.Element> |
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.
BREAKING CHANGE: When removing the reliance upon the now-defunct GripperHoverWrapper
class, this then allowed further refactoring, to where this Element
property behaved more or less just like the framework Content
property, so I refactored the Element
property out.
- The change would be invisible to anyone just using the default
GridSplitter
settings, and would not introduce any breaking changes. - However, for anyone that has a custom implementation of the
GridSplitter
, this could break the customization after we remove thisElement
property.
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.
Yeah, if we can just use the default Content
pattern, then that's going to be fine to remove the custom Element
thing. :)
Microsoft.Toolkit.Uwp.UI.Controls.Layout/GridSplitter/SplitBase.cs
Outdated
Show resolved
Hide resolved
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 may be good to break-down the API sets between GridSplitter and ContentSizer and then what we want each of them to look like and the base class. (For example CommunityToolkit/dotnet#2 and CommunityToolkit/dotnet#1)
...kit.Uwp.SampleApp/SamplePages/FrameworkElementExtensions/FrameworkElementExtensionsCode.bind
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI.Controls.Layout/ContentSizer/ContentSizer.xaml
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI.Controls.Layout/GridSplitter/GridSplitter.Events.cs
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI.Controls.Layout/GridSplitter/GridSplitter.Options.cs
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI.Controls.Layout/GridSplitter/GridSplitter.xaml
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI.Controls.Layout/ContentSizer/ContentSizer.cs
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI.Controls.Layout/ContentSizer/ContentSizer.cs
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI.Controls.Layout/GridSplitter/SplitBase.cs
Outdated
Show resolved
Hide resolved
Trying to do a small test in a file new project:
Getting a crash in layout, not sure why yet. But wanted to document here. Changing to Binding doesn't make a difference, and the native stack trace isn't helpful... Edit: Oh, I bet this is from the Now just trying to figure out this scenario. |
95006fb
to
0ac40ef
Compare
What is this?
A PR to compare the difference to the original GridSplitter PR (CommunityToolkit#4083) and this new refactoring of both GridSplitter and ContentSizer.
Details
The pair of controls
GridSpliter
andContentSizer
share a lot of the same functionality.What is the new behavior?
The plan is to refactor out all the common base functionality of
GridSplitter
&ContentSizer
, and add them to a common base class (SplitBase
).Also includes, but is not limited to, the following updates:
- Addressed by adding a new bool variable
InvertDragDirection
What does the future hold for this specific PR?
The end goal of this PR is just to compare the new refactoring, to the original GridSplitter PR. It is not expected that this PR will be merged into the user branch; instead, if all goes accordingly, the code refactoring changes in this PR will eventually be merged with the master branch of the CommunityToolkit.