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

Add MultiColumnsStackPanel #3381

Draft
wants to merge 34 commits into
base: main
Choose a base branch
from

Conversation

vgromfeld
Copy link
Contributor

In one of my app, I have a very long form containing a lot of fields. I usually limit the width of the fields to have something not to big. To fill the rest of the screen, we can use adaptive visual state triggers but it is not that convenient.

In this PR, I'm proposing a multi columns stack panel that will automatically display its children in columns of equal height.
This control works best when used inside a vertical ScrollViewer but can also be used without.

image

PR Type

What kind of change does this PR introduce?

  • Feature

What is the new behavior?

Add a new MultiColumnsStackPanel. It exposes the following properties:

  • ItemsSpacing: the vertical space between the child elements
  • ColumnsSpacing: the horizontal width between the columns
  • MaxColumnWidth: the maximum width for the columns
    _ HorizontalContentAlignement: the horizontal alignment for the children. If the child is a Control we will use its Control.HorizontalAlignment value instead.

It should still be possibe to improve the algorithm to reduce the number of computation. That's something that can be done later :)

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Pull Request has been submitted to the documentation repository instructions. Link:
  • Sample in sample app has been added / updated (for bug fixes / features)
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

@ghost
Copy link

ghost commented Jul 7, 2020

Thanks vgromfeld for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@ghost ghost assigned azchohfi Jul 7, 2020
@michael-hawker
Copy link
Member

Thanks @vgromfeld, this is an interesting scenario.

I'm trying to think of other panels we have already for this scenario like WrapPanel w/ Orientation Vertical which can do some similar things (outside a ScrollViewer). I guess the difference here is it still tries to put things in columns explicitly?

In which case, how does this differ from something like the StaggeredPanel? Is it just the order of the layout, could we add an Orientation property to it instead?

If we move forward, I think we should align to other common spacing names like RowSpacing and ColumnSpacing or maybe VerticalSpacing?

FYI @azchohfi @skendrot @Sergio0694

@skendrot
Copy link
Contributor

skendrot commented Jul 10, 2020

This looks very much like the WrapPanel. However, when the Orientation is vertical and you have a ScrollViewer that scrolls vertically I don't think it will flow to another column.
From what I can see, the advantage of this control is that the developer does not have to do the math to split the items into columns of a Grid.

@michael-hawker
Copy link
Member

I don't think there's enough overlap in code to share with WrapPanel, but I wonder if we want a common interface for the properties that are shared like HorizontalSpacing, VerticalSpacing, and Padding?

@vgromfeld
Copy link
Contributor Author

In which case, how does this differ from something like the StaggeredPanel? Is it just the order of the layout, could we add an Orientation property to it instead?

The main difference is that StaggeredPanel will always try to have columns of equal height by adding items to the smallest columns. This implies that it can change the order of the items. When used in horizontal mode, it requires an horizontal scrollbar.
In this panel, I want the first column(s) to always be fully filled and have the last one partially filled. It fully fills the columns one after the other. This panel always keep the children order.

If we move forward, I think we should align to other common spacing names like RowSpacing and ColumnSpacing or maybe VerticalSpacing?

I agree, I will update the naming to stay aligned.

@skendrot
Copy link
Contributor

I wonder if we should add support for this in the WrapPanel instead of a new control. We could add a property for evenly distributing items

@vgromfeld
Copy link
Contributor Author

@skendrot, there is some overlap in how the two controls are displaying things but the approach is not the same. MultiColumnsStackPanel enforce the columns' width whereas WrapPanel does not and MultiColumnsStackPanel tries to find the best partition to limit scrolling. WrapPanel is more lightweight and only arrange the items one after the other.
It feels that merging the code of the two will end with a big if/else statement in the measure/arrange phases to run either one or the other layout. We should keep WrapPanel light (and faster).

@vgromfeld
Copy link
Contributor Author

@RosarioPulella, you are rigth about the constraint the control tries to follow:

  • Fill the columns so their heights are as even as possible following a constraint in width.
  • Order the items from top to bottom of a column and then from the left to the right column.
  • Fill up the first column(s) and put the rest in the last column (this depends on the number of children we have and how we can lay out them).

I agree that there is some overlap between this new control and the existing WrapPanel and StaggeredPanel controls regarding how they are displaying things but the approach is not the same.

  • WrapPanel does not enforce the columns' width and should not. WrapPanel is mostly a StackPanel which can arrange items on a new line when it reaches the end of a line. It uses lightweight computations to get this layout and should keep is performances.
  • StaggeredPanel may be a better fit but the actual implementation is closed to the WrapPanel's one. It arranges items on a new line once it reaches the end of a line but instead of restarting at the begining, it will put the items in the smallest column. This breaks the children order since the first element on the second row may not be the one after the last one of the first row. Adding the MultiColumnsStackPanel implementation in it feels like adding a big if/else statement in the measure/arrange phases to run either one or the other layout.

I choose to write a new panel to keep the existing ones light and simple.

I do agree that we can find a better name. I didn't spend a lot of time on the one I choose but didn't find anything better that MultiColumnStackPanel 😊.

@michael-hawker, I updated the sample code to add some HorizontalAlignment properties on the first children to validate the behavior but I can remove them since they can be confusing. I will update the heights of the text blocks to have a better alignment.

@michael-hawker
Copy link
Member

Thanks @vgromfeld, I think that's been the sticking point is it just feels so close even though the implementations are different. 🙂 I agree just adding a big if/else isn't going to be helpful. Let's keep it separate as you have it in this PR for now.

Good to know the buttons were just a test, I just wasn't sure what the desired behavior was as it seemed like a case where a dev might expect them to show up in the same positioning with the different alignments.

For naming, wondering if we want to call this FormPanel? It could actually be a good base if we want to tackle adding a prototype for the WinUI Form Control microsoft/microsoft-ui-xaml#82 proposal in the future. Not sure if that would require more functionality for this panel or not (for the 'groups'), but we could investigate that later.

image

It's something I could use in XAML Studio for my settings page too, so maybe it's something I can play with and prototype on one of my streams later... 🤔

@Rosuavio
Copy link
Contributor

Rosuavio commented Nov 12, 2020

@vgromfeld I Hope I was not too harsh on the name you chose. But I also think it makes a lot of sense as its own control.

One thing I don't understand is why to try to fill the fist columns and leave the rest in the last? If we are trying to make all the other columns as close to each other in height as possible, then why to have one overflow-ish one that we just dump items into. It sounds like it would be more consistent if we just try to make all the columns around the same height.

I might not fully understand the driving use case, but is the point of this control to dynamically layout items into columns (near in height to each other) according to a constraint in width? I think that behavior alone is clear to users, but adding the bit about the last column seems inconsistent to me.

On a similar note, if the StaggeredPanel did something like that I would be confused about the reasoning behind it. It seems like it would be less general, more opinionated, and maybe useful to fewer users.

@vgromfeld
Copy link
Contributor Author

@michael-hawker , I like the FormPanel name. I've updated the code to use this name 😊. I've also removed the text controls and updated the height of all the text fields:

image

@RosarioPulella the last/overflow column is mostly an artefact of the implementation. I probably need to improve the logic to have better columns' sizes. For now, the first column is driving everything and sometimes, based on the children heights, I might not get a fully filled last column. I will check if there is something easily doable.

@Rosuavio
Copy link
Contributor

Thanks, @vgromfeld. That clears up the intended behavior and I am excited to see this control merged 😃!
Also not sure if this behavior is intended, but it seems to adapt to the width of its container until 4 columns and stops distributing items past that.
Form_fill

Is it supposed to keep making columns like the StaggeredPanel?

Staggered_fill

@vgromfeld
Copy link
Contributor Author

@RosarioPulella, this is intended. The control tries to fill the full height of the container. If the container becomes so wide that we have more space than needed, only the "first" columns will be filled.

Microsoft.Toolkit.Uwp.UI.Controls/FormPanel/FormPanel.cs Outdated Show resolved Hide resolved
var columnsCount = 1;
if (columnsWidth < availableWidth)
{
var additionalColumns = (int)((availableWidth - columnsWidth) / (columnsWidth + HorizontalSpacing));
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of doing additional columns wouldn't it be better to set the column count to be the result of the available width divided by the column width?
columnsCount = (int)(availableWidth / (columnsWidth + HorizontalSpacing));

VerticalSpacing="@[VerticalSpacing:Slider:8:0-64]"
MaxColumnWidth="@[MaxColumnWidth:Slider:240:0-500]"
Padding="@[Padding:Thickness:0,0,0,0]@">
<TextBox Header="Field 1" />
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be best to generate these in code with random heights and allow the user to add new textboxes. Would be better for testing.

@skendrot
Copy link
Contributor

skendrot commented Nov 17, 2020

Doing some testing with adding random new fields with random heights and getting some behavior that seems off. Shouldn't these columns be closer in height? With the small heights of some of the boxes they could be shifted over.
image
image

@michael-hawker
Copy link
Member

Thanks for taking a look @skendrot, always appreciate your insights in this space!

@vgromfeld let us know when you have a chance to get back to looking at feedback, I know you're a bit busy. If you're in no rush too, I could see this spilling over into a 7.1 release so we can coordinate with the WTS folks in the comment I left in #3569 and test this out more with some more form helpers to complete the WinUI story vision... Thoughts?

@michael-hawker
Copy link
Member

Welcome to the Windows Community Toolkit @Loser09sly! We always love to see community folks reviewing features that are being worked on in the Toolkit.

Since this is your first interaction with our community, would you mind providing a few more details on what you tried for your review and your first impressions?

Thanks a bunch! Look forward to working with you more in the future.

@michael-hawker
Copy link
Member

@vgromfeld thoughts on where we are and what's left to do based on the feedback in the PR? Should we try and get this into 7.0 or hold back and evaluate adding more helpers for a larger story to support microsoft/microsoft-ui-xaml#82 for 7.1 more fully and use that as a test-case for this panel?

@vgromfeld
Copy link
Contributor Author

@michael-hawker, we should wait for 7.1 and see how we can integrate some of the form control elements in it.

@Kyaa-dost
Copy link
Contributor

@vgromfeld Thanks for the input.

@michael-hawker will add this in 7.1 for now and then will proceed from there.

@Kyaa-dost Kyaa-dost added this to the 7.1 milestone Jan 13, 2021
@michael-hawker michael-hawker modified the milestones: 7.1, 7.2/8.0? Aug 31, 2021
@michael-hawker michael-hawker added the labs 🧪 Marks an issue/PR involved with Toolkit Labs label Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants