-
Notifications
You must be signed in to change notification settings - Fork 10k
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
Improve new Blazor project templates. #50927
Comments
@EdCharbeneau well said ... when are you posting a pull request with the above changes? 😛 (joking) |
Thanks for contacting us. We're moving this issue to the |
Thanks for bringing this up, @EdCharbeneau. Feel free to send us a PR (in main) and we will try to merge it for .NET 9. |
@mkArtakMSFT Will the template shipping in .NET 8 be backwards compatible with .NET 7, or will it only apply to 8+? |
Before submitting a PR, it would help to gather some feedback. There is a sample repo linked above, the refactored version eliminated roughly 100 lines of CSS code. It utilizes CSS variables built into Bootstrap 5.3, and auto detects and applies light/dark themes (no interactivity required). If anyone would like to give it a quick review, I'll start working on a PR. gragra33 Care to give it a look? |
@EdCharbeneau Happy to... |
@EdCharbeneau I had a look and looks great! 4 minor things:
|
This is awesome, thanks. I'll fix these before submitting a PR. Note: The checkbox UI toggles need to be replaced by something accessible. |
Fixed:
Not Fixing:
Doing this properly with a11y in mind will require JavaScript. I feel like it's a nice-have feature but adds unnecessary complexity.
This is already an issue in the Bootstrap repo. It's better to let them handle it there and benefit from it in the next version of Bootstrap. twbs/bootstrap#29422 |
@EdCharbeneau as a thought, it might be advisable to add a comment line in the header to indicate how to turn off auto-dark mode. |
Great minds think alike. It's already there. 😀 |
@EdCharbeneau Great work as usual. Hope to see the PR accepted for the final release of 8.0. |
Thanks. It looks like (if accepted) it will come in an SDK update later on. .NET 8 is locked, got to respect the existing plans. |
Overall better HTML semantics by changing the structure of the layout in markup. Visually the rendering is the same as .NET 7. A new Header.razor component was added to accommodate the new layout. Collapsible navigation was rewritten to use a small bit of inline JavaScript instead of the improvised checkbox approach. The menu has proper aria-* attributes and sets focus to the first link when opened, per WCAG guidelines. Removed :focus override, this is bad practice for a11y. Bootstrap 5.3 / themes Updated to Bootstrap 5.3 and embraced Bootstrap throughout the sample layout. This eliminated ~100 lines of unnecessary CSS code and simplified the HTML too. Now MainLayout.razor.css only contains error ui CSS code. Enabled theming via Bootstrap 5.3's CSS variables (aka Custom Properties). This allows runtime theme changes and future proofs the code for Bootstrap vnext. Automatic light/dark mode detected and applied via user preferences. This is a no-code solution enabled by CSS variables. Developers can remove this feature by uncommenting a single line of code in App.razor Made use of inline svg icons. This enables light/dark themes to control icon color. Added --bl-* (opt-in) CSS variables to control the theme of the NavBar component. This allows devs to easily ditch the gradient if they want to customize the template. Code improvements Reduced complexity of CSS code throughout, app.css is much smaller and cleaner. Used updated semantics for mediaqueries. Avoided CSS variables where code could be generated in SampleContent == false templates. Add/Removed blazor-error-ui CSS code based on template interactivity. The markup was removed in the prior version, but not the CSS. Fixes issue: dotnet#50927
Thanks for contacting us. We're moving this issue to the |
Thanks for your feedback. We've decided that we will be modernizing the template during .NET 9 and we will incorporate your feedback as part of that work, that we track using #53142 |
Summary
File > New templates are the first thing developers see when they try Blazor. The templates shouldn't promote bad practice and poor accessibility. Fixing a few issues would improve the overall optics of Blazor for people onboarding, especially if they're coming from other frameworks that are detail oriented in terms of HTML/CSS/A11y.
Motivation and goals
The
!important
keyword in CSS is a code smell. In this case, the template has used the Bootstrap utilitypx-4
class to set the padding in the markup. In the CSS, the class is overridden using a new selector and padding value. This causes confusion when customizing the template.This is just one simple example, but overall it is a lot of CSS and Bootstrap fighting each other. My recommendation would be to go all in on Boostrap, or remove it completely. I've been working on an example that leans into Boostrap and eliminates redundant CSS like the example above. As much as I personally would like to see Boostrap sunset, it is still one of the easiest ways to onboard new devs.
The structure of the topbar, sidebar and main page are oddly constructed in markup. I feel like this might be related to the Boostrap and CSS duality.
There is at least one priority item, like turning off a11y features directly through CSS, this focus effect.
*:focus
is one of the most common a11y criticisms. We should look at color contrast as well, since this is another high priority and easily remedied.There's also an interesting and creative 💖 use of a checkbox to control the responsive menu. I feel that this was implemented to help with static rendering, so the nav could be hidden and shown on mobile without interactivity (C#/JS), however the a11y aspect of it is questionable. In the short term, this item should have aria-controls attributes added to it. In the long term, something needs to address the missing
aria-expanded
attribute (likely requires interactivity to toggle).Do a full review of the HTML semantics to see if the page is constructed properly. Semantic HTML structure plays a critical role in a11y. The usage of
main
andarticle
in the template could possibly be an issue.Once the aforementioned items are cleaned up, it should be easier to add some modern features like CSS variables that will enable basic customizations that are accessible to Junior level HTML/CSS devs.
Having some examples of the SectionOutlet might be nice to see in the sample content, but the state of the current markup makes this difficult to add.
In scope
SectionOutlet
sample where appropriate.Risks / unknowns
Unsure if there is an accessible alternative to the navbar show/hide feature using a checkbox. Is it an accessibility issue? If so, can it be fixed without introducing JavaScript or C#?
Are there any dependencies outside of Blazor in ASP.NET that will be impacting by the changes? Ex: Identity templates.
Sample repo
The following repo contains a refactored version of the static rendered project template. (No interactivity selected)
https://github.com/EdCharbeneau/BlazorAppTemplateReview
Sample repo Update: The refactored version eliminated roughly 100 lines of CSS code. It utilizes CSS variables built into Bootstrap 5.3, and auto detects and applies light/dark themes (no interactivity required).
Sample repo Update: Rewrote the toggle menu for a11y. It required some inline JavaScript but not enough to warrant a whole script file or tag. The new menu uses proper aria-* attributes and sets focus to the first menu item when shown.
The text was updated successfully, but these errors were encountered: