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

[Windows] Fix CollectionView broken test #15584

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
<?xml version="1.0" encoding="utf-8" ?>
<ContentPage
xmlns="http://schemas.microsoft.com/dotnet/2021/maui"
xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml"
x:Class="Maui.Controls.Sample.Pages.CollectionViewGalleries.SpacingGalleries.DefaultItemSpacing"
Title="Default ItemSpacing">
<Grid
RowDefinitions="Auto, *"
Padding="12">
<Label
Text="By default, there is no space between each item in a CollectionView."/>
<CollectionView
Grid.Row="1"
ItemsLayout="VerticalList"
ItemsSource="{Binding Items}"
BackgroundColor="Red">
<CollectionView.ItemTemplate>
<DataTemplate >
<Label
Text="{Binding .}"
TextColor="Black"
FontSize="14"
BackgroundColor="Yellow"/>
</DataTemplate>
</CollectionView.ItemTemplate>
</CollectionView>
</Grid>
</ContentPage>
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
using System.Collections.Generic;
using Microsoft.Maui.Controls;

namespace Maui.Controls.Sample.Pages.CollectionViewGalleries.SpacingGalleries
{
public partial class DefaultItemSpacing : ContentPage
{
public DefaultItemSpacing()
{
InitializeComponent();
Items = new();
AddItems();
BindingContext = this;
}

public List<string> Items { get; set; }

void AddItems()
{
for (var i = 0; i < 10; i++)
{
Items.Add($"Item {i + 1}");
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ public ItemsSpacingGallery()
{
Children =
{
descriptionLabel,
descriptionLabel,
GalleryBuilder.NavButton("Default Spacing", () =>
new DefaultItemSpacing (), Navigation),
GalleryBuilder.NavButton("Vertical List Spacing", () =>
new SpacingGallery (LinearItemsLayout.Vertical), Navigation),
GalleryBuilder.NavButton("Horizontal List Spacing", () =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,20 @@ public static class CollectionViewExtensions
if (layout is null)
return null;

var h = layout?.ItemSpacing ?? 0;
var v = layout?.ItemSpacing ?? 0;
var margin = WinUIHelpers.CreateThickness(h, v, h, v);

var style = new WStyle(typeof(GridViewItem));

style.Setters.Add(new WSetter(FrameworkElement.MarginProperty, margin));
if (layout?.Orientation == ItemsLayoutOrientation.Vertical)
{
style.Setters.Add(new WSetter(FrameworkElement.MinHeightProperty, 0));

var h = layout?.ItemSpacing ?? 0;
var v = layout?.ItemSpacing ?? 0;
var margin = WinUIHelpers.CreateThickness(h, v, h, v);
style.Setters.Add(new WSetter(FrameworkElement.MarginProperty, margin));
Copy link
Member

Choose a reason for hiding this comment

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

Why do we only set Margin on Vertical ? what happens if i give ItemsSpacing in a Horizontal layout ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to do more tests to know exactly why, but could have sense to use in both cases. The code comes from XF https://github.com/xamarin/Xamarin.Forms/blob/4f3acdfb0bc98e3ba8e2f3eae86e88602350ee84/Xamarin.Forms.Platform.UAP/CollectionView/StructuredItemsViewRenderer.cs#L241

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the test is broken because in this PR when the methods were moved from the handler to the extension method class, the MinHeight and MinWidth property settings weren't moved along with everything else. The code that was removed just needs to be added back in, the rest of this is unnecessary.

}
else
style.Setters.Add(new WSetter(FrameworkElement.MinWidthProperty, 0));

style.Setters.Add(new WSetter(Control.PaddingProperty, WinUIHelpers.CreateThickness(0)));
style.Setters.Add(new WSetter(Control.HorizontalContentAlignmentProperty, HorizontalAlignment.Stretch));

Expand All @@ -31,13 +38,18 @@ public static class CollectionViewExtensions
if (layout is null)
return null;

var style = new WStyle(typeof(GridViewItem));

if (layout?.Orientation == ItemsLayoutOrientation.Vertical)
Copy link
Member

Choose a reason for hiding this comment

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

What's the default value here for MinHeight? why setting it fixes the issue?

style.Setters.Add(new WSetter(FrameworkElement.MinHeightProperty, 0));
else
style.Setters.Add(new WSetter(FrameworkElement.MinWidthProperty, 0));

var h = layout?.HorizontalItemSpacing ?? 0;
var v = layout?.VerticalItemSpacing ?? 0;
var margin = WinUIHelpers.CreateThickness(h, v, h, v);

var style = new WStyle(typeof(GridViewItem));

style.Setters.Add(new WSetter(FrameworkElement.MarginProperty, margin));

style.Setters.Add(new WSetter(Control.PaddingProperty, WinUIHelpers.CreateThickness(0)));
style.Setters.Add(new WSetter(Control.HorizontalContentAlignmentProperty, HorizontalAlignment.Stretch));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ await CreateHandlerAndAddToWindow<LayoutHandler>(layout, (handler) =>
});
}

[Fact(Skip = "FIX FOR .NET8")]
[Fact]
public async Task ValidateItemContainerDefaultHeight()
{
SetupBuilder();
Expand Down