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

Refactor: Adaptive Column Width for StaggeredLayout #371

Merged

Conversation

Poker-sang
Copy link
Contributor

@Poker-sang Poker-sang commented Mar 17, 2024

Fixes

Adaptive Column Width for StaggeredLayout

PR Type

What kind of change does this PR introduce?

  • Refactoring (no functional changes, no api changes)

What is the current behavior?

old

What is the new behavior?

new

PR Checklist

Please check if your PR fulfills the following requirements:

  • Created a feature/dev branch in your fork (vs. submitting directly from a commit on main)
  • Based off latest main branch of toolkit
  • Tested code with current supported SDKs
  • New component
    • Documentation has been added
    • Sample in sample app has been added
    • Analyzers are passing for documentation and samples
    • Icon has been created (if new sample) following the Thumbnail Style Guide and templates
  • Tests for the changes have been added (if applicable)
  • Header has been added to all new source files
  • Contains NO breaking changes

Other information

@niels9001 niels9001 requested a review from Arlodotexe March 17, 2024 14:59
@Arlodotexe
Copy link
Member

Seems like we should maybe put this functionality behind a flag? The current non-adaptive behavior of the StaggeredLayout has likely been accounted for anyone already using it, this would change things under them. @michael-hawker What do you think?

@Poker-sang
Copy link
Contributor Author

maybe like LineFlowLayout, use ItemsStretch property?

@michael-hawker
Copy link
Member

@Poker-sang there's a comment for DesiredColumnWidth here on line 26:

/// The width of columns can exceed the DesiredColumnWidth if the HorizontalAlignment is set to Stretch.

Does the existing setup not allow for this scenario in this case? Otherwise wondering if that'd be the hook to look for?

@Poker-sang
Copy link
Contributor Author

Which object should I put HorizontalAlignment on? I tried ItemsRepeater and the items, but not work.

image

@michael-hawker
Copy link
Member

Ah, @Poker-sang I see now, it's a comment left over from the StaggeredPanel class here:

/// The width of columns can exceed the DesiredColumnWidth if the HorizontalAlignment is set to Stretch.

As it was the original template for the conversion to the Layout. Panels have their own alignment property, but Layouts don't.

So, indeed we need some configuration here for this. ItemsStretch seems like a good name, as that aligns with the UnifomGridLayout. Not sure if we want to re-use that enum though, as I think we only have two behaviors, right? Probably just need our own with None (default) and Fill as options (for this new behavior). Akin to https://learn.microsoft.com/en-us/windows/windows-app-sdk/api/winrt/microsoft.ui.xaml.controls.linedflowlayoutitemsstretch

@Poker-sang
Copy link
Contributor Author

Poker-sang commented Mar 20, 2024

Probably just need our own with None (default) and Fill as options (for this new behavior). Akin to https://learn.microsoft.com/en-us/windows/windows-app-sdk/api/winrt/microsoft.ui.xaml.controls.linedflowlayoutitemsstretch

Sounds good. But since it's also unlikely that we'll be able to directly use LinedFlowLayoutItemsStretch, maybe the bool type is enough?

@michael-hawker
Copy link
Member

Oh, just make our own enum for use, better future proofing that way, and works better for fitting in with expectations for values for the property since it's similar to other ones.

@michael-hawker michael-hawker added this to the 8.1 milestone Mar 20, 2024
@Poker-sang
Copy link
Contributor Author

ItemsStretch property is added

@michael-hawker
Copy link
Member

Thanks @Poker-sang, mind just adding a property to the sample page, so that we can show the difference there as well? You can see a similar example for the UnifomGrid here:

[ToolkitSampleMultiChoiceOption("OrientationProperty", "Horizontal", "Vertical", Title = "Orientation")]

Just need to add an x:Bind function (see below in that file) and the binding in the XAML.

(@Arlodotexe looks like no default is selected for these for the ComboBox?)

@Poker-sang
Copy link
Contributor Author

I've added the example. But I don't know what's wrong with xaml style.

@michael-hawker
Copy link
Member

I couldn't see on the CI which files failed, it's only showing the passing ones for some reason. But if you run the PowerShell script in the repo root, it should be able to fix them:

ApplyXamlStyling.ps1 -Main

@Arlodotexe we should ensure we have the notes in the wiki about this script as well as a screenshot about the XAML Styler extension setting to search up the directory path:

image

@Poker-sang
Copy link
Contributor Author

ok i will try it

Copy link
Member

@Arlodotexe Arlodotexe left a comment

Choose a reason for hiding this comment

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

Code changes look good. Pulled and tested under Uwp, Wasm and Wasdk and found no issues. Let's merge!

@Arlodotexe Arlodotexe merged commit 02a9449 into CommunityToolkit:main Mar 25, 2024
9 checks passed
@Poker-sang Poker-sang deleted the poker/feat/staggered-layout branch March 26, 2024 01:48
if (ItemsStretch is StaggeredLayoutItemsStretch.None)
{
columnWidth = double.IsNaN(DesiredColumnWidth) ? availableWidth : Math.Min(DesiredColumnWidth, availableWidth);
numColumns = Math.Max(1, (int)Math.Floor(availableWidth / state.ColumnWidth));
Copy link
Member

Choose a reason for hiding this comment

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

🦙 Leaving a marker here for historical record/the future that we had missed the state.ColumnWidth here not being the desired value of columnWidth calculated above, as @Poker-sang called out here: #542 (comment)

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