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.

Sign up for GitHub

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

Fixing TheTheme newly intruduced accessibility rules and HTML rules violations (Lombiq Technologies: NEST-462) #14523

Merged
merged 2 commits into from
Oct 16, 2023
Merged

Fixing TheTheme newly intruduced accessibility rules and HTML rules violations (Lombiq Technologies: NEST-462) #14523

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
355549c
Fixing accessibility rule and HTML rule violations
wAsnk Oct 16, 2023
8fa2228
Using role instead of changing link to button
wAsnk Oct 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/OrchardCore.Themes/TheTheme/Views/ToggleTheme.cshtml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
<li class="nav-item nav-item dropdown text-end">
<a class="nav-link dropdown-toggle" id="bd-theme" aria-expanded="false" data-bs-toggle="dropdown" data-bs-display="static" aria-label="@T["Toggle theme"]">
<li class="nav-item dropdown text-end">
<button class="nav-link dropdown-toggle" id="bd-theme" aria-expanded="false" data-bs-toggle="dropdown" data-bs-display="static" aria-label="@T["Toggle theme"]">
Copy link
Member

Choose a reason for hiding this comment

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

add type="button" Have you tested this out on all screen sizes to make sure it does not break the view?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is used in all other built in themes. I haven't tested it on all screen sizes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought this was just a copy paste error.

Copy link
Member

Choose a reason for hiding this comment

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

Let do a test please specially on small screen. Let's not assume it'll work since CSS could be training a link not a button for styling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, it did change the styling in mobile view. Added role because type won't help here.

<span class="theme-icon-active"><i class="fa-solid fa-circle-half-stroke"></i></span>
<span class="d-none" id="bd-theme-text">@T["Toggle theme"]</span>
</a>
</button>
<ul class="dropdown-menu dropdown-menu-end" aria-labelledby="bd-theme-text">
<li>
<button type="button" class="dropdown-item" data-bs-theme-value="auto" aria-pressed="false">
Expand Down
2 changes: 1 addition & 1 deletion src/OrchardCore.Themes/TheTheme/Views/UserNotificationNavbar.cshtml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

@model UserNotificationNavbarViewModel

<li class="nav-item nav-item dropdown text-end">
<li class="nav-item dropdown text-end">
<div class="dropdown">
<button type="button" class="nav-link dropdown-toggle" href="#" role="button" data-bs-toggle="dropdown" data-bs-auto-close="outside" aria-expanded="false">
@if (Model.TotalUnread > 0)
Expand Down