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

BlazorWebView API review changes: Shared sources #5982

Merged
merged 5 commits into from
Apr 14, 2022

Conversation

MackinnonBuck
Copy link
Member

@MackinnonBuck MackinnonBuck commented Apr 11, 2022

Description of Change

Addresses changes to the BlazorWebView API for shared sources.

It should be noted that there are a few items in the linked issue that this PR does not address:


IBlazorWebView.cs: Confirm how mandatory it is to have this interface instead of deleting it and declaring the handler in terms of the concrete BlazorWebView type. Remove the interface if it's not really needed in our case.

Addressed in #5984.


RootComponent.cs: Change Parameters to be IReadOnlyDictionary<string, object?>? as long as this doesn't break setting parameters from XAML.

AFAICT, there isn't a convenient way to create a ParameterView from an IReadOnlyDictionary<string, object?> (the ParameterView.FromDictionary() method only accepts an IDictionary<string, object?>).

So, it doesn't seem we would be able to make this change without either copying to a new dictionary or making changes to ParameterView in dotnet/aspnetcore. The former is undesirable and the latter might not be feasible this late in the release cycle.


RootComponentsCollection.cs: Since the collection is typed as observable, check we do in fact react if the collection is modified. If not we should make it not observable.

We do react if the collection is modified, so no change is required here.

Issues Fixed

Fixes #5905

@MackinnonBuck MackinnonBuck requested a review from a team as a code owner April 11, 2022 22:40
@Eilon Eilon added the area-blazor Blazor Hybrid / Desktop, BlazorWebView label Apr 11, 2022
Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

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

Great, thanks @MackinnonBuck!

I'd personally prefer it if we still changed https://github.com/dotnet/maui/pull/5982/files#r848252057 but approving now to ensure this can be merged whenever you are ready.

Copy link
Member

@Eilon Eilon left a comment

Choose a reason for hiding this comment

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

Beautiful!

@MackinnonBuck MackinnonBuck merged commit 5d942a1 into main Apr 14, 2022
@MackinnonBuck MackinnonBuck deleted the mbuck/blazorwebview-api-changes-shared-sources branch April 14, 2022 17:26
@github-actions github-actions bot locked and limited conversation to collaborators Dec 21, 2023
@samhouts samhouts added the fixed-in-6.0.300-rc.2 Look for this fix in 6.0.300-rc.2! label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Blazor Hybrid / Desktop, BlazorWebView fixed-in-6.0.300-rc.2 Look for this fix in 6.0.300-rc.2!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BlazorWebView API review: Shared sources
4 participants