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 support for configuring WebViewSettings via DI #5059

Merged
merged 9 commits into from
Mar 7, 2022

Conversation

javiercn
Copy link
Member

@javiercn javiercn commented Mar 4, 2022

Adds support for configuring the Development tools via a settings object registered in DI.

A few things to note:

  • There are new top-level extension methods Add<<Maui|Wpf|WindowsForms>BlazorWebView that are the entry point on each flavor and perform all registrations necessary on the DI container.
  • The BlazorWebViewSettings is Settings and not Options because Maui by default doesn't support the Options pattern.
    • For this reason, the object is passed directly instead of using a callback as in more "idiomatic" ASP.NET Core extension methods.
    • The BlazorWebViewSettings is shared source a cross all platforms. Ideally this would have been defined in the abstract component package, but we already shipped that and we can't change it.
  • We use a global object in DI instead of on a per webview basis because some options are only configurable globally (like DevelomentMode in Android) and we expect in most cases to only host one WebView at a time.
    • We can revisit this in the future if we detect that many people want to have multiple webviews with different settings associated with them.

@javiercn javiercn requested a review from a team as a code owner March 4, 2022 15:14
@javiercn javiercn requested a review from Eilon March 4, 2022 15:16
@javiercn javiercn force-pushed the javiercn/dev-tools-configuration branch from 8875efa to 85c1ae7 Compare March 4, 2022 17:43
Copy link
Contributor

@TanayParikh TanayParikh left a comment

Choose a reason for hiding this comment

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

This looks great! Just some minor comments.

I believe I'll be able to leverage this to support DETAILED_ERRORS via #4441!

@Eilon Eilon added the area-blazor Blazor Hybrid / Desktop, BlazorWebView label Mar 7, 2022
@javiercn javiercn force-pushed the javiercn/dev-tools-configuration branch from 8c1520f to 2a8654b Compare March 7, 2022 18:11
return builder;
}

public static IServiceCollection ConfigureMauiHandlers(this IServiceCollection services, Action<IMauiHandlersCollection>? configureDelegate)
Copy link
Member

Choose a reason for hiding this comment

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

@PureWeen FYI we're adding an overload here for handler registration that goes off of IServiceCollection because in some Blazor scenarios that's all we have access to.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, I forgot to point it out.

@@ -9,14 +9,17 @@ public static MauiApp CreateMauiApp()
{
var builder = MauiApp.CreateBuilder();
builder
.RegisterBlazorMauiWebView()
Copy link
Member

Choose a reason for hiding this comment

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

🥳

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.

Looks good to me. I think overall this API is cleaner and easy to use.

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.

Awesome, thanks for handling all the feedback

@javiercn javiercn merged commit cfc3fab into main Mar 7, 2022
@javiercn javiercn deleted the javiercn/dev-tools-configuration branch March 7, 2022 20:50
@github-actions github-actions bot locked and limited conversation to collaborators Dec 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Blazor Hybrid / Desktop, BlazorWebView
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants