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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

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
@@ -46,6 +46,16 @@ protected override void Dispose(bool disposing)
_footerViewFormsElement.MeasureInvalidated -= OnFormsElementMeasureInvalidated;
}

if (_headerUIView is MauiView hv)
{
hv.LayoutChanged += HeaderView_LayoutChanged;
Copy link
Member

Choose a reason for hiding this comment

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

What if you subscribe to SizeChanged on the header/footer xplat elements?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I try to do the following:
_headerViewFormsElement.SizeChanged += _headerViewFormsElement_SizeChanged;

_headerViewFormsElement_SizeChanged never triggers

@jsuarezruiz jsuarezruiz added platform/iOS 🍎 area-controls-collectionview CollectionView, CarouselView, IndicatorView labels Apr 15, 2024
@rmarinho
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@rmarinho
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jsuarezruiz
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jsuarezruiz
Copy link
Contributor

/azp run

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
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?

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 .

@@ -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

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

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

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 kubaflo changed the title CollectionView Items display issue when Header is resized on iOS - Fix CollectionView Items display issue when Header is resized on iOS - UI-Test Oct 31, 2024
@kubaflo
Copy link
Contributor Author

kubaflo commented Nov 15, 2024

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

@PureWeen you were right!
But I think we can include the UI test from this PR. What do you think?

@rmarinho
Copy link
Member

rmarinho commented Dec 9, 2024

/rebase

@rmarinho
Copy link
Member

/rebase

@rmarinho
Copy link
Member

/azp run

@dotnet dotnet deleted a comment from azure-pipelines bot Dec 11, 2024
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@rmarinho
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@rmarinho
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@kubaflo kubaflo changed the title CollectionView Items display issue when Header is resized on iOS - UI-Test CollectionView Items display issue when Header is resized on iOS Dec 17, 2024
@rmarinho
Copy link
Member

/azp run

@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

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

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

@rmarinho
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-controls-collectionview CollectionView, CarouselView, IndicatorView community ✨ Community Contribution platform/iOS 🍎
Projects
None yet
5 participants