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

GridSplitter changes, rebased on top of OG GridSplitter PR #3

Open
wants to merge 22 commits into
base: user/mhawker/gridsplitter
Choose a base branch
from

Conversation

XAML-Knight
Copy link

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 and ContentSizer 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:

  • Resizing is now functional when starting from the right or bottom edge (previously, resizing only worked from left and top)
     - Addressed by adding a new bool variable InvertDragDirection
  • AutomationProperty for unit tests added
  • Set proper gripper character based on resizing direction

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.

michael-hawker and others added 15 commits January 19, 2022 10:40
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...
Copy link
Owner

@michael-hawker michael-hawker left a 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;
Copy link
Owner

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?

Copy link
Author

Choose a reason for hiding this comment

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

Indeed - change made.

@XAML-Knight
Copy link
Author

... 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.

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.

@@ -85,14 +84,6 @@
<controls:GridSplitter.RenderTransform>
<TranslateTransform Y="-8" />
</controls:GridSplitter.RenderTransform>
<controls:GridSplitter.Element>
Copy link
Author

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 this Element property.

Copy link
Owner

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. :)

Copy link
Owner

@michael-hawker michael-hawker left a 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)

@michael-hawker
Copy link
Owner

michael-hawker commented Feb 4, 2022

Trying to do a small test in a file new project:

    <Grid>
        <Grid.ColumnDefinitions>
            <ColumnDefinition Width="Auto" />
            <ColumnDefinition Width="*" />
        </Grid.ColumnDefinitions>

        <muxc:NavigationView
            x:Name="ViewPanel"
            HorizontalAlignment="Left"
            PaneDisplayMode="Left">
            <muxc:NavigationView.MenuItems>
                <muxc:NavigationViewItem
                    Content="Menu Item1"
                    Icon="Play"
                    Tag="SamplePage1" />
                <muxc:NavigationViewItem
                    Content="Menu Item2"
                    Icon="Save"
                    Tag="SamplePage2" />
                <muxc:NavigationViewItem
                    Content="Menu Item3"
                    Icon="Refresh"
                    Tag="SamplePage3" />
                <muxc:NavigationViewItem
                    Content="Menu Item4"
                    Icon="Download"
                    Tag="SamplePage4" />
            </muxc:NavigationView.MenuItems>
        </muxc:NavigationView>

         <!-- crashes unless this is commented out, measureoverride issue -->
        <controls:ContentSizer TargetControl="{x:Bind ViewPanel}" />

        <Border Grid.Column="1">
            <TextBlock
                HorizontalAlignment="Center"
                VerticalAlignment="Center"
                Text="Content Area" />
        </Border>
    </Grid>

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 SymbolFonts not being defined... let me try adding that key to the app..,
Edit2: Yup, adding that to the app resources prevented the crash, so that needs to be fixed.

Now just trying to figure out this scenario.

@michael-hawker michael-hawker force-pushed the user/mhawker/gridsplitter branch 2 times, most recently from 95006fb to 0ac40ef Compare March 7, 2022 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants