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] Notify changes in CollectionView Layouts #13137

Merged
merged 9 commits into from
Mar 15, 2023
Merged

[Windows] Notify changes in CollectionView Layouts #13137

merged 9 commits into from
Mar 15, 2023

Conversation

jsuarezruiz
Copy link
Contributor

@jsuarezruiz jsuarezruiz commented Feb 6, 2023

Description of Change

Notify changes in CollectionView Layouts (Span, Spacing etc).

Issue fixed on Windows (changing Spans).
fix-10144

To validate the changes, launch the .NET MAUI Gallery and navigate to the CollectionView section. Select the "Vertical Grid" sample and update the Span property value.
test-span

Issues Fixed

FIxes #4112
Fixes #8387
Fixes #10144

@jsuarezruiz jsuarezruiz added t/bug Something isn't working platform/windows 🪟 area-controls-collectionview CollectionView, CarouselView, IndicatorView labels Feb 6, 2023
else if (e.PropertyName == GridItemsLayout.HorizontalItemSpacingProperty.PropertyName || e.PropertyName == GridItemsLayout.VerticalItemSpacingProperty.PropertyName)
UpdateItemsLayoutItemSpacing();
else if (e.PropertyName == LinearItemsLayout.ItemSpacingProperty.PropertyName)
UpdateItemsLayoutItemSpacing();
Copy link
Member

Choose a reason for hiding this comment

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

Would look a lot nicer with a switch expression and the removal of the else clauses.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree with that. In this case, just continued following the if-else approach used so far in Xamarin. https://github.com/xamarin/Xamarin.Forms/blob/caab66bcf9614aca0c0805d560a34e176d196e17/Xamarin.Forms.Platform.Android/Renderers/ButtonRenderer.cs#L140

Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing that was an old code and did not have switch statements, since we are dealing with new code, lets try to update or code base, right?

jsuarezruiz and others added 2 commits February 7, 2023 15:54
…r.Windows.cs

Co-authored-by: Manuel de la Pena <mandel@microsoft.com>
Copy link
Member

@mandel-macaque mandel-macaque left a comment

Choose a reason for hiding this comment

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

Looks good. Some comments about using a switch expression, but that has nothing to do with how the code will behave and is just a note.

@jsuarezruiz jsuarezruiz marked this pull request as ready for review February 17, 2023 08:26
@jsuarezruiz jsuarezruiz changed the title [All platforms] Notify changes in CollectionView Layouts [Windows] Notify changes in CollectionView Layouts Feb 17, 2023
@hartez hartez merged commit d89ef37 into main Mar 15, 2023
@hartez hartez deleted the fix-8387 branch March 15, 2023 16:54
@jsuarezruiz jsuarezruiz added the backport/suggested The PR author or issue review has suggested that the change should be backported. label Apr 12, 2023
@hartez
Copy link
Contributor

hartez commented May 24, 2023

/backport to net7.0

@github-actions
Copy link
Contributor

Started backporting to net7.0: https://github.com/dotnet/maui/actions/runs/5070755171

@github-actions
Copy link
Contributor

@hartez backporting to net7.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Notify changes in CollectionView Layouts on Windows
Using index info to reconstruct a base tree...
M	src/Controls/src/Core/Handlers/Items/StructuredItemsViewHandler.Windows.cs
M	src/Controls/src/Core/PublicAPI/net-windows/PublicAPI.Unshipped.txt
Falling back to patching base and 3-way merge...
Auto-merging src/Controls/src/Core/PublicAPI/net-windows/PublicAPI.Unshipped.txt
CONFLICT (content): Merge conflict in src/Controls/src/Core/PublicAPI/net-windows/PublicAPI.Unshipped.txt
Auto-merging src/Controls/src/Core/Handlers/Items/StructuredItemsViewHandler.Windows.cs
CONFLICT (content): Merge conflict in src/Controls/src/Core/Handlers/Items/StructuredItemsViewHandler.Windows.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Notify changes in CollectionView Layouts on Windows
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@github-actions
Copy link
Contributor

@hartez an error occurred while backporting to net7.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

hartez added a commit that referenced this pull request May 24, 2023
github-actions bot pushed a commit that referenced this pull request May 28, 2023
rmarinho pushed a commit that referenced this pull request May 28, 2023
@hartez hartez added the backport/approved After some discussion or review, this PR or change was approved to be backported. label May 31, 2023
@samhouts samhouts added the partner/cat 😻 this is an issue that impacts one of our partners or a customer our advisory team is engaged with label Aug 21, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 12, 2023
@samhouts samhouts added the fixed-in-8.0.0-preview.3.8149 Look for this fix in 8.0.0-preview.3.8149! label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-collectionview CollectionView, CarouselView, IndicatorView backport/approved After some discussion or review, this PR or change was approved to be backported. backport/suggested The PR author or issue review has suggested that the change should be backported. fixed-in-8.0.0-preview.3.8149 Look for this fix in 8.0.0-preview.3.8149! partner/cat 😻 this is an issue that impacts one of our partners or a customer our advisory team is engaged with platform/windows 🪟 t/bug Something isn't working
Projects
None yet
4 participants