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

Blazor Web template updates #49801

Merged
merged 36 commits into from
Aug 3, 2023
Merged

Conversation

SteveSandersonMS
Copy link
Member

@SteveSandersonMS SteveSandersonMS commented Aug 2, 2023

Fixes #49556
Fixes #49299
Fixes #49056
Fixes #49053
Fixes #48983
Fixes #49412

This is a hopefully complete implementation. I'll update the template tests tomorrow, but it should be possible to review already. Update: tests updated.

Almost all the restructuring here has already been discussed and agreed with @danroth27. Hope I didn't forget any of the changes.

@SteveSandersonMS SteveSandersonMS requested a review from a team as a code owner August 2, 2023 16:29
@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Aug 2, 2023
</div>
</div>

<div class="@NavMenuCssClass nav-scrollable" @onclick="ToggleNavMenu">
<input type="checkbox" title="Navigation menu" class="navbar-toggler" />
Copy link
Member Author

Choose a reason for hiding this comment

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

This is visually the same as the toggler we had before, but now it doesn't require .NET (it's reduced to CSS and a single JS onclick on line 9) and still works correctly with keyboard/focus/tooltips for accessibility.

@SteveSandersonMS SteveSandersonMS added area-blazor Includes: Blazor, Razor Components feature-full-stack-web-ui Full stack web UI with Blazor and removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Aug 2, 2023
@SteveSandersonMS SteveSandersonMS self-assigned this Aug 2, 2023
@SteveSandersonMS
Copy link
Member Author

SteveSandersonMS commented Aug 3, 2023

What's the reasoning behind renaming NavMenu to Sidebar? I agree it is a side bar (on the side), but it's also a nav menu (list of navigation links). Both names seem valid, so why change it?

It's always been confusing because it is actually the entire side of the screen, not just the nav menu. You couldn't just swap it out with some other "nav menu" because that would break the page layout. It has the top-left quadrant as well as the bottom-left. I think it's easier to understand the role of this component when it clarifies this.

As an alternative I could try putting more of this component's content into MainLayout and then reduce this to be purely a nav menu. I'll give it a go and see if that makes sense.

@SteveSandersonMS
Copy link
Member Author

SteveSandersonMS commented Aug 3, 2023

What if we kept all the component pages in the top-level Pages folder and moved the layout related components into Pages/Layout? The Pages folder would continue to contain all the routable components and the components that enable them, so the root component and the router component would go in Pages too, similar to how _Host.cshtml lives in Pages today. Thoughts?

Definitely happy to discuss this. However, I find the idea of putting the root component under Pages to be very counter-intuitive (it's not a page; it doesn't even imply the existence of a concept of pages). Simply moving Layouts under Pages sounds fine though.

@@ -62,7 +62,8 @@
<GeneratedContent Include="WebApi-FSharp.fsproj.in" OutputPath="content/WebApi-FSharp/Company.WebApplication1.fsproj" />
<GeneratedContent Include="Worker-CSharp.csproj.in" OutputPath="content/Worker-CSharp/Company.Application1.csproj" />
<GeneratedContent Include="Worker-FSharp.fsproj.in" OutputPath="content/Worker-FSharp/Company.Application1.fsproj" />
<GeneratedContent Include="Components-CSharp.csproj.in" OutputPath="content/Components-CSharp/Components-CSharp.csproj" />
<GeneratedContent Include="BlazorWeb-CSharp.csproj.in" OutputPath="content/BlazorWeb-CSharp/BlazorWeb-CSharp/BlazorWeb-CSharp.csproj" />
<GeneratedContent Include="BlazorWeb-CSharp.Client.csproj.in" OutputPath="content/BlazorWeb-CSharp/BlazorWeb-CSharp.Client/BlazorWeb-CSharp.Client.csproj" />
Copy link
Member

Choose a reason for hiding this comment

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

The other template that we have is components-webassembly could we have consistent naming between the two? (Either BlazorWebassembly or ComponentsWeb`)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'd like to change the other template's name to BlazorWebAssembly (to match everything else about its branding) and eliminate the "empty" variant in favour of a flag on BlazorWebAssembly. Just not trying to do that as part of this PR.

Comment on lines 11 to +17
"UseWebAssembly": {
"longName": "use-wasm",
"longName": "use-wasm"
},
"Empty": {
"longName": "empty"
},
"IncludeSampleContent": {
Copy link
Member

Choose a reason for hiding this comment

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

Do not block on this, but we were talking in the past of making blazor web bring in server and webassembly by default.

The reason we did opt-in as opposed to opt-out was that we didn't have the underlying features in place. Do we want to keep that model (opt-in) or do we want to switch to opt-out.

/cc: @danroth27

Copy link
Member Author

@SteveSandersonMS SteveSandersonMS Aug 3, 2023

Choose a reason for hiding this comment

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

Dan and I did discuss this and came to the conclusion that server-on-by-default, wasm-off-by-default was the preferred balance for .NET 8. We're certainly free to change that in the future if we so choose.

Copy link
Member

Choose a reason for hiding this comment

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

@javiercn I want interactivity to work by default, but I'm concerned about the complexity that WebAssembly currently adds. I'm thinking about the getting started experience here for new users. Granted, the downside of having WebAssembly off by default is that if you later want to add it, it's not easy to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment