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

CollectionView Items display issue when Header is resized on iOS #21812

Merged
merged 2 commits into from
Jan 14, 2025
Merged

CollectionView Items display issue when Header is resized on iOS #21812

merged 2 commits into from
Jan 14, 2025

Conversation

kubaflo
Copy link
Contributor

@kubaflo kubaflo commented Apr 13, 2024

Issues Fixed

Fixes #20538
Fixes #12429

Before After
Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-04-13.at.17.21.10.mp4
Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-04-13.at.17.17.05.mp4

@kubaflo kubaflo requested a review from a team as a code owner April 13, 2024 15:23
@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Apr 13, 2024
@jsuarezruiz jsuarezruiz added platform/iOS 🍎 area-controls-collectionview CollectionView, CarouselView, IndicatorView labels Apr 15, 2024
@rmarinho
Copy link
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@rmarinho
Copy link
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jsuarezruiz
Copy link
Contributor

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jsuarezruiz
Copy link
Contributor

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@dotnet dotnet deleted a comment from azure-pipelines bot Jun 12, 2024
@dotnet dotnet deleted a comment from azure-pipelines bot Jun 12, 2024
@dotnet dotnet deleted a comment from azure-pipelines bot Jun 12, 2024
mattleibow
mattleibow reviewed Jul 18, 2024
View reviewed changes
Copy link
Member

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

I am just wondering about the fact that now we have 2 events that are triggering the layout code when the first did not work. Is there a reason for both?

src/Controls/src/Core/Handlers/Items/iOS/StructuredItemsViewController.cs Outdated
Comment on lines 49 to 52
if (_headerUIView is MauiView hv)
{
hv.LayoutChanged -= HeaderViewLayoutChanged;
}
Copy link
Member

Choose a reason for hiding this comment

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

If we are now watching the actual method that is doing layout, do we still need the original _footerViewFormsElement.MeasureInvalidated? The code seems to end up calling the same HandleFormsElementMeasureInvalidated method.

Copy link
Member

Choose a reason for hiding this comment

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

IS grabbing the element for footer and header not the all view right? some still need for each one .

src/Core/src/Platform/iOS/MauiView.cs Outdated
@@ -134,6 +134,7 @@ public override void LayoutSubviews()
}

CrossPlatformArrange(bounds);
OnLayoutChanged();
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should be renamed to match the method. Also, no need to have the method, just invoke the event directly. Maybe use a name of ArrangingSubviews or even just OnLayoutSubviews since it is internal. I see MovedToWindow got away with the same name because it was an explicit interface member.

Copy link
Contributor Author

@kubaflo kubaflo Jul 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I changed it to invoke the event directly

PureWeen
PureWeen requested changes Aug 3, 2024
View reviewed changes
Copy link
Member

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

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

We should revisit if this is needed once we test on net9 with CollectionViewHandler2 and once we merge this PR #23052

@rmarinho
Copy link
Member

/rebase

@rmarinho
Copy link
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jsuarezruiz
Copy link
Contributor

@kubaflo Could you rebase and fix the conflicts? Thanks in advance.

@kubaflo
Copy link
Contributor Author

kubaflo commented Dec 17, 2024

@rmarinho it turned out that I had to make some changes to fix it. So, it is not only a UI test now

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@rmarinho
Copy link
Member

Seems is failing for CV2

@kubaflo
Copy link
Contributor Author

kubaflo commented Dec 17, 2024 •
edited
Loading

@rmarinho Luckily, it is only because the iOS screenshot needs to be updated. Could you please /azp?

@rmarinho
Copy link
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

rmarinho
rmarinho requested changes Dec 18, 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.

Seems the tests are failing for CV2 HeaderShouldNotCollapseWithItems

@kubaflo
Copy link
Contributor Author

kubaflo commented Dec 19, 2024

@rmarinho hmm, locally it works well with the following change:
Screenshot 2024-12-19 at 14 52 52

Maybe let's try run tests again

@rmarinho
Copy link
Member

rmarinho commented Jan 7, 2025

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@rmarinho
Copy link
Member

rmarinho commented Jan 7, 2025

@rmarinho hmm, locally it works well with the following change: Screenshot 2024-12-19 at 14 52 52

Maybe let's try run tests again

Still failing.
https://dev.azure.com/xamarin/public/_build/results?buildId=131698&view=ms.vss-test-web.build-test-results-tab&runId=3557296&resultId=100092&paneView=attachments

@kubaflo
Copy link
Contributor Author

kubaflo commented Jan 9, 2025

@rmarinho so this is actually not working on CV2. I will try to get to the bottom of why it doesn't work. But in the meantime can we merge this fix for cv1?

@kubaflo
Copy link
Contributor Author

kubaflo commented Jan 9, 2025

@rmarinho I've added [FailsOnMacWhenRunningOnXamarinUITest("This is not working for CV2 yet")], so that a PR that fixes it for CV2 will only have to remove it

@rmarinho
Copy link
Member

rmarinho commented Jan 9, 2025

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@kubaflo kubaflo mentioned this pull request Jan 11, 2025
Open
@kubaflo
Copy link
Contributor Author

kubaflo commented Jan 13, 2025

Hi @rmarinho! Could it be merged so I can proceed with this one? #27076

@rmarinho rmarinho merged commit 70f5aae into dotnet:main Jan 14, 2025
98 of 104 checks passed
@rmarinho rmarinho added this to the .NET 9 SR4 milestone Jan 14, 2025
@Tamilarasan-Paranthaman Tamilarasan-Paranthaman mentioned this pull request Jan 21, 2025
Merged
@rmarinho
Copy link
Member

/backport to release/9.0.1xx-sr3

@github-actions GitHub Actions
Copy link
Contributor

Started backporting to release/9.0.1xx-sr3: https://github.com/dotnet/maui/actions/runs/12911585074

@github-actions github-actions bot mentioned this pull request Jan 22, 2025
Closed
@kubaflo kubaflo mentioned this pull request Feb 3, 2025
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Feb 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mattleibow mattleibow mattleibow left review comments

@jsuarezruiz jsuarezruiz jsuarezruiz left review comments

@PureWeen PureWeen PureWeen requested changes

@rmarinho rmarinho rmarinho approved these changes

@StephaneDelcroix StephaneDelcroix Awaiting requested review from StephaneDelcroix StephaneDelcroix is a code owner automatically assigned from dotnet/dotnet-maui-reviewers

@jfversluis jfversluis Awaiting requested review from jfversluis jfversluis is a code owner automatically assigned from dotnet/dotnet-maui-reviewers

Assignees
No one assigned
Labels
area-controls-collectionview CollectionView, CarouselView, IndicatorView community ✨ Community Contribution platform/iOS 🍎
Projects
MAUI SDK Ongoing
Status: Done
Milestone
.NET 9 SR4
5 participants
@kubaflo @rmarinho @jsuarezruiz @mattleibow @PureWeen