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

Add legacy Grid method (and a better one) #13408

Merged
merged 5 commits into from
Feb 28, 2023
Merged

Add legacy Grid method (and a better one) #13408

merged 5 commits into from
Feb 28, 2023

Conversation

hartez
Copy link
Contributor

@hartez hartez commented Feb 17, 2023

Description of Change

Lots of folks are missing the 5-parameter overload of Add from Xamarin.Forms when migrating applications. While it's not possible to have the exact method signature (as the MAUI version of Grid does not use the IGridList interface or the internal GridElementCollection from Forms for performance reasons), we can add a similar extension method with the same behavior for Grid.

So this pull request includes a 5-parameter overload of Add() with the same behavior as the Forms version. It's intended to aid migration; I would personally not recommend it for any other purpose because of its confusing behavior.

Also included in this API is another method, AddWithSpan(), which does the exact same thing as Add() but has less confusing parameter names and more straightforward behavior.

This PR also includes tests for both methods, plus documentation.

I'm opening this PR so we have a stable point around which to discuss whether we simply keep the weird version of Add() or replace it with a more straightforward method, and how that impacts the migration tools.

Issues Fixed

Fixes #780
Closes #9891

@github-actions
Copy link
Contributor

Thank you for your pull request. We are auto-formating your source code to follow our code guidelines.

@jsuarezruiz jsuarezruiz added area-layout StackLayout, GridLayout, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter layout-grid labels Feb 17, 2023
@davidbritch
Copy link
Contributor

davidbritch commented Feb 17, 2023

If we're going to have the 5 argument overload, can we at least make it obsolete? So it's there to aid migrations but will be removed in a future release. What I'd hate to see is new users start using this overload (I think it requires mental gymnastics).

If we're adding this overload for migration purposes, then presumably we'd also need to add the 3 argument Add overload that's missing from AbsoluteLayout.

If we do add overloads, will this only land in a .NET 8 build?

If we don't add the overloads, I'll add them to the migration docs. However, I get that this isn't the smoothest path for users. @Sweekriti91 Would there be a way for UA to detect that the user is attempting to use the 5 arg overload, and add the extension method code to the project?

@jfversluis
Copy link
Member

can we at least make it obsolete?

I don't think this is a bad idea. Immediately ty and get people to another, better method and we can remove for .NET 9 if we want to.

If we do add overloads, will this only land in a .NET 8 build?

Typically yes, if we decide this is crucial to migration we might decide otherwise, but I'll defer that to EZ or Jon.

Would there be a way for UA to detect that the user is attempting to use the 5 arg overload, and add the extension method code to the project?

Note that this does add back the functionality, but there is a difference already with what it was in Forms. In Forms the Add method was on Grid.Children this one is on Grid so UA needs to, ideally, detect those and basically just remove .Children and it will work.

@hartez
Copy link
Contributor Author

hartez commented Feb 23, 2023

@Sweekriti91 is it going to complicate/impair migrations if we make the 5-parameter version of this method obsolete?

@jfversluis
Copy link
Member

Should we also update the documentation on the Grid? I think that is what initially sparked this discussion.

@davidbritch
Copy link
Contributor

I'll be updating the conceptual doc once this is merged and in a public, non-preview release. So unless this gets back ported to .NET 7, I won't be adding this to the docs until Nov time.

@Sweekriti91
Copy link

@Sweekriti91 is it going to complicate/impair migrations if we make the 5-parameter version of this method obsolete?

Nope, will not complicate migrations. This is great, marking is Obsolete is preferred, that way they can make adjustments as they do the migration, knowing this method is obsolete and 🤞🏽 encourage folks to use 3-parameter overload instead.

@hartez
Copy link
Contributor Author

hartez commented Feb 23, 2023

@Sweekriti91 is it going to complicate/impair migrations if we make the 5-parameter version of this method obsolete?

Nope, will not complicate migrations. This is great, marking is Obsolete is preferred, that way they can make adjustments as they do the migration, knowing this method is obsolete and 🤞🏽 encourage folks to use 3-parameter overload instead.

Awesome, I'll update this with the obsolete attribute later today.

@hartez
Copy link
Contributor Author

hartez commented Feb 27, 2023

@jfversluis @Sweekriti91 @davidbritch Marked the method obsolete. Anything else need to happen before this gets merged?

@jfversluis
Copy link
Member

jfversluis commented Feb 28, 2023

@hartez only added a small change to also make the API docs reflect this change so that it's in line again. For the rest looks good to me!

@ghost
Copy link

ghost commented Feb 28, 2023

🚨 Breaking change detected @davidbritch FYI

@davidbritch
Copy link
Contributor

All good with me. Completely different issue, but as this is being added to aid migrations, is there a case for adding back the AbsoluteLayout.Add three param overload to also aid migrations?

@github-actions github-actions bot locked and limited conversation to collaborators Dec 13, 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-layout StackLayout, GridLayout, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter fixed-in-8.0.0-preview.3.8149 Look for this fix in 8.0.0-preview.3.8149! layout-grid t/breaking 💥
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Grid.Children missing Add convenience methods
8 participants