Skip to content

[iOS] Fix for ItemSpacing not properly applied between the item and group header in CollectionView Horizontal LinearItemsLayout#25882

Merged
rmarinho merged 4 commits intodotnet:mainfrom
Tamilarasan-Paranthaman:fix-25859
Dec 3, 2024
Merged

[iOS] Fix for ItemSpacing not properly applied between the item and group header in CollectionView Horizontal LinearItemsLayout#25882
rmarinho merged 4 commits intodotnet:mainfrom
Tamilarasan-Paranthaman:fix-25859

Conversation

@Tamilarasan-Paranthaman
Copy link
Copy Markdown
Member

@Tamilarasan-Paranthaman Tamilarasan-Paranthaman commented Nov 15, 2024 •
edited
Loading

Root Cause of the issue

  • The ItemSpacing for the section was not set in the GetInsetForSection method for LinearItemsLayout, defaulting to zero. This affected both horizontal and vertical orientations, resulting in no spacing between the group header and the item.

Description of Change

  • I have implemented logic to apply item spacing between the group section and the item within GetInsetForSection for LinearItemsLayout. This change ensures that the spacing is correctly applied for both horizontal and vertical orientations, based on the specified orientation.

Issues Fixed

Fixes #25859

Tested the behaviour in the following platforms

  • Android
  • Windows
  • iOS
  • Mac

Screenshot

  • Note: I have set the CollectionView background color to gray to make the differences more visually apparent. The gray areas between the items in the image represent the spaces.

1. LinearItemsLayout - Orientation="Horizontal"

Before Issue Fix After Issue Fix

2. LinearItemsLayout - Orientation="Vertical"

Before Issue Fix After Issue Fix

@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Nov 15, 2024
@dotnet-policy-service
Copy link
Copy Markdown
Contributor

dotnet-policy-service bot commented Nov 15, 2024

Hey there @Tamilarasan-Paranthaman! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@jsuarezruiz jsuarezruiz added area-controls-collectionview CollectionView, CarouselView, IndicatorView platform/ios labels Nov 15, 2024
jsuarezruiz
jsuarezruiz reviewed Nov 15, 2024
View reviewed changes
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue25859.cs Outdated
@@ -0,0 +1,25 @@
#if !MACCATALYST
Copy link
Copy Markdown
Contributor

@jsuarezruiz jsuarezruiz Nov 15, 2024

Choose a reason for hiding this comment

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

Why avoid Catalyst?, is crashing?

For now, this test will not do much on macOS because the VerifyScreenshot is not yet implemented.

Copy link
Copy Markdown
Member Author

@Tamilarasan-Paranthaman Tamilarasan-Paranthaman Nov 18, 2024

Choose a reason for hiding this comment

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

Since VerifyScreenshot was not implemented on Mac, I had previously restricted it. I have now removed that restriction

@Tamilarasan-Paranthaman Tamilarasan-Paranthaman marked this pull request as ready for review November 18, 2024 09:58
@Tamilarasan-Paranthaman Tamilarasan-Paranthaman requested a review from a team as a code owner November 18, 2024 09:58
@dotnet dotnet deleted a comment from azure-pipelines bot Nov 18, 2024
@dotnet dotnet deleted a comment from azure-pipelines bot Nov 18, 2024
@dotnet dotnet deleted a comment from jsuarezruiz Nov 18, 2024
@jfversluis
Copy link
Copy Markdown
Member

jfversluis commented Nov 19, 2024

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Nov 19, 2024

Azure Pipelines successfully started running 3 pipeline(s).

rmarinho
rmarinho requested changes Nov 27, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@rmarinho rmarinho left a comment

Choose a reason for hiding this comment

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

Does this work with CollectionView2 ?

@Tamilarasan-Paranthaman
Copy link
Copy Markdown
Member Author

Tamilarasan-Paranthaman commented Nov 27, 2024

Does this work with CollectionView2?

@rmarinho, No, it will not work. In CollectionView2, the issue was slightly different: the item spacing was not applied between the group header and the item on both sides. Additionally, the group header size did not fit the content size.

I have attached an image showing the post-fix state of CollectionView1 and the current state of CollectionView2 for your reference. For now, I have only fixed the issue in CollectionView1, which is why I used CollectionView1 in the test sample as well.

maui/src/Controls/tests/TestCases.HostApp/Issues/Issue25859.xaml

Line 8 in 3dae5a8

<cv1:CollectionView1 Grid.Row="0"

CV1 CV2

Note: I have set the background color of the CollectionView to gray to make the differences more visually noticeable. The gray areas between the items in the image represent the spacing.

rmarinho
rmarinho approved these changes Nov 28, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@rmarinho rmarinho left a comment

Choose a reason for hiding this comment

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

Can we create a new issue, or leave #25859 open to fix this on CollectionView2

@rmarinho rmarinho requested a review from jsuarezruiz November 28, 2024 15:02
@rmarinho rmarinho added this to the .NET 9 SR2 milestone Nov 28, 2024
@rmarinho rmarinho merged commit 4f522d0 into dotnet:main Dec 3, 2024
prakashKannanSf3972 added a commit to devanathan-vaithiyanathan/maui that referenced this pull request Dec 4, 2024
@prakashKannanSf3972
Reverted-Changes.
47e9893 
I implemented a fix for the CV VerticalSpacing issue to address a test case failure. However, since the same changes were recently merged. I have reverted my changes.

PR => dotnet#25882
@sheiksyedm sheiksyedm added the partner/syncfusion Issues / PR's with Syncfusion collaboration label Dec 6, 2024
rmarinho pushed a commit that referenced this pull request Dec 12, 2024
@devanathan-vaithiyanathan @prakashKannanSf3972
[iOS] Fixed HeaderFooterGrid to pass on CV1 and CV2 (#26022)
efa6895 
* Fixed-CollectionView-Size-Rendering-Issue

* Resolved-LastSection-ItemsSpacing-Issue

* Reverted-Changes.

I implemented a fix for the CV VerticalSpacing issue to address a test case failure. However, since the same changes were recently merged. I have reverted my changes.

PR => #25882

* Revert "Reverted-Changes."

This reverts commit 47e9893.

* Revert "Resolved-LastSection-ItemsSpacing-Issue"

This reverts commit 0e5aabf.

* Updated-Header-Footer-iOS-SnapShot

---------

Co-authored-by: prakashKannanSf3972 <127308739+prakashKannanSf3972@users.noreply.github.com>
@samhouts samhouts added the fixed-in-net8.0-nightly This may be available in a nightly release! label Dec 16, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jan 16, 2025
@Tamilarasan-Paranthaman Tamilarasan-Paranthaman deleted the fix-25859 branch June 16, 2025 11:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@rmarinho rmarinho rmarinho approved these changes

@PureWeen PureWeen Awaiting requested review from PureWeen PureWeen was automatically assigned from dotnet/dotnet-maui-reviewers

@jsuarezruiz jsuarezruiz Awaiting requested review from jsuarezruiz

Assignees

No one assigned

Labels

area-controls-collectionview CollectionView, CarouselView, IndicatorView community ✨ Community Contribution fixed-in-9.0.21 fixed-in-net8.0-nightly This may be available in a nightly release! partner/syncfusion Issues / PR's with Syncfusion collaboration platform/ios

Projects

MAUI SDK Ongoing
Status: Done

Milestone

.NET 9 SR2

Development

Successfully merging this pull request may close these issues.

Item spacing not properly applied between items in CollectionView Horizontal LinearItemsLayout.

6 participants

@Tamilarasan-Paranthaman @jfversluis @rmarinho @jsuarezruiz @samhouts @sheiksyedm