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

[WrapPanel] Add Spacing Properties #18079

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Poker-sang
Copy link
Contributor

What does the pull request do?

Add ItemSpacing and LineSpacing Properties for WrapPanel

What is the current behavior?

WrapPanel has no spacing properties

What is the updated/expected behavior with this PR?

image

How was the solution implemented (if it's not obvious)?

Checklist

Breaking changes

Obsoletions / Deprecations

Fixed issues

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0054640-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@robloo
Copy link
Contributor

robloo commented Jan 30, 2025

In the UWP/WinUI community toolkit naming if these properties was discussed and decided as HorizontalSpacing and VerticalSpacing. I think we should follow that terminology here.

https://learn.microsoft.com/en-us/windows/communitytoolkit/controls/wrappanel#properties

Horizontal/Vertical terminology is more general-purpose (can apply to other controls) and fits well with existing properties for alignment.

@Poker-sang
Copy link
Contributor Author

Poker-sang commented Jan 30, 2025

@robloo I was going to use that name, but after careful consideration I realized:

  1. for unaligned controls, people care more about ItemSpacing and LineSpacing than H/V Spacing. If you set a different H/V Spacing and change the orientation, the new layout can be confusing.

  2. In WindowsAppSDK 1.5 and later, has LinedFlowLayout which is a very similar layout to WrapLayout, and its properties' names, MinItemSpacing and LineSpacing. I think these names make more sense.

So after weighing them, I choose to use ItemSpacing and LineSpacing instead of H/V Spacing in Toolkit.

@robloo
Copy link
Contributor

robloo commented Jan 30, 2025

@Poker-sang You've thought this through more than I thought and more than what was decided in the toolkit. I think the logic for your point 1 above is the strongest argument.

With controls that have an orientation property -- itself with horizontal/vertical -- that is a complication. It would be nice to be able to set the Spacing and just have it work and make sense for all orientations.

Let me think about this a bit more. I suspect you're right though.

@MrJul MrJul added needs-api-review The PR adds new public APIs that should be reviewed. feature labels Jan 30, 2025
@timunie
Copy link
Contributor

timunie commented Jan 30, 2025

@robloo I was also like you at the beginning but am mostly sold on it for this control. Take StackPanel, it has only Spacing and will lead to the same results regardless of the orientation. That should be the same here imo. But the team also needs to agree on it

@robloo
Copy link
Contributor

robloo commented Feb 1, 2025

I'm basically sold that we shouldn't be using H/V Spacing terminology for the above mentioned reasons.

Additional thoughts I've had on naming these properties are:

  • ItemSpacing
    • At first look this terminology seems fine
    • But then perhaps "Item" is redundant. The standard spacing property of course will control space between children.
    • Perhaps just the term Spacing is enough. It is pretty intuitive what a Spacing property would be for and the line/row spacing property is clearly differentiated by another name.
    • Spacing would align the property with StackPanel. Both properties actually do the same thing.
    • ItemSpacing does however align well with existing Item* properties on the control.
  • LineSpacing
    • The first thought when I see LineSpacing is for spacing between multi-line text. I suspect I wouldn't be the only one thinking that and there could be confusion caused by this.
    • We also have RowSpacing for UniformGrid/Grid now. Both properties actually do the same thing.
    • The WrapPanel has no usage of the term "Line" within the control. Also for that matter it has no usage of "Row". Therefore, these terms seem arbitrary.
    • I think we should consider using the term RowSpacing here just like Grid and UniformGrid rather than LineSpacing.
    • Yes, this would deviate from the WinUI example of the LinedFlowLayout but it also aligns better with existing controls.

So bottom line, we could align with existing controls/properties more if we changed the wording:

  1. ItemSpacing -> Spacing
  2. LineSpacing -> RowSpacing (edit below)

Edit: On second thought, "Row" does have an orientation implied. "Line" might make more sense for both orientations as a line could either be horizontal or vertical.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature needs-api-review The PR adds new public APIs that should be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants