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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from
Dec 3, 2024
Merged

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

merged 4 commits into from
Dec 3, 2024

Conversation

Tamilarasan-Paranthaman
Copy link
Contributor

@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 Dotnet Policy Service
Copy link
Contributor

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
Contributor

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
Contributor Author

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
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

rmarinho
rmarinho requested changes Nov 27, 2024
View reviewed changes
Copy link
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
Contributor Author

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
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
109 checks passed
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
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 is a code owner 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