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

enhance(design): improve style of current item in menu #11061

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
+8 −8
Open

enhance(design): improve style of current item in menu #11061

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
0e5ad4d
enhance(design): merge --text-{active,inactive} + increase contrast
caugner May 3, 2024
67af8e8
enhance(menu): inverse highlight
caugner May 3, 2024
a73c4fe
fix(menu): increase contrast of active item
caugner May 3, 2024
adf5e67
enhance(menu): use opacity instead of --text-inactive
caugner May 3, 2024
de4ae4b
enhance(menu): combine opacity with --text-secondary
caugner May 3, 2024
f391b90
revert: back out curriculum changes
caugner May 3, 2024
2375085
Merge branch 'main' into improve-inactive-style
caugner May 20, 2024
b043dcc
Merge branch 'main' into improve-inactive-style
caugner Jun 14, 2024
5fd652f
fix(menu): exclude user-menu from menu highlighting
caugner Jul 4, 2024
516670c
Merge branch 'main' into improve-inactive-style
caugner Jul 4, 2024
6415c73
chore(style): remove unused variables
caugner Jul 10, 2024
7fb7cec
Update client/src/ui/molecules/menu/index.scss
caugner Jul 11, 2024
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
2 changes: 0 additions & 2 deletions client/src/ui/_vars.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,6 @@ $mdn-color-ads: #00d0aa;

$mdn-theme-light-text-primary: $mdn-color-neutral-90;
$mdn-theme-light-text-secondary: $mdn-color-neutral-70;
$mdn-theme-light-text-active: #{$mdn-color-neutral-50};
$mdn-theme-light-text-inactive: #{$mdn-color-neutral-40}a6;
$mdn-theme-light-text-link: $mdn-color-light-theme-blue-60;
$mdn-theme-light-text-invert: $mdn-color-white;
Expand Down Expand Up @@ -203,7 +202,6 @@ $mdn-theme-light-code-background-block: $mdn-color-neutral-light-80;

$mdn-theme-dark-text-primary: $mdn-color-white;
$mdn-theme-dark-text-secondary: $mdn-color-neutral-20;
$mdn-theme-dark-text-active: #{$mdn-color-neutral-50};
$mdn-theme-dark-text-inactive: #{$mdn-color-neutral-20}a6;
$mdn-theme-dark-text-link: $mdn-color-dark-theme-blue-30;
$mdn-theme-dark-text-invert: $mdn-color-neutral-90;
Expand Down
2 changes: 0 additions & 2 deletions client/src/ui/base/_themes.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
@mixin light-theme {
--text-primary: #{$mdn-theme-light-text-primary};
--text-secondary: #{$mdn-theme-light-text-secondary};
--text-active: #{$mdn-theme-light-text-active};
caugner marked this conversation as resolved.
Show resolved Hide resolved
--text-inactive: #{$mdn-theme-light-text-inactive};
--text-link: #{$mdn-theme-light-text-link};
--text-visited: #551a8b; // Source: https://searchfox.org/mozilla-central/rev/02841791400cf7cf5760c0cfaf31f5d772624253/modules/libpref/init/StaticPrefList.yaml#1787-1790
Expand Down Expand Up @@ -314,7 +313,6 @@
@mixin dark-theme {
--text-primary: #{$mdn-theme-dark-text-primary};
--text-secondary: #{$mdn-theme-dark-text-secondary};
--text-active: #{$mdn-theme-dark-text-active};
caugner marked this conversation as resolved.
Show resolved Hide resolved
--text-inactive: #{$mdn-theme-dark-text-inactive};
--text-link: #{$mdn-theme-dark-text-link};
--text-visited: #ffadff; // Source: https://searchfox.org/mozilla-central/rev/02841791400cf7cf5760c0cfaf31f5d772624253/modules/libpref/init/StaticPrefList.yaml#1794-1797
Expand Down
8 changes: 6 additions & 2 deletions client/src/ui/molecules/menu/index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
.top-level-entry-container {
&.active > a {
&:not(.active):not(.user-menu):not(:hover) {
caugner marked this conversation as resolved.
Show resolved Hide resolved
opacity: 0.775;
caugner marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Why are we using opacity rather than setting color?

Using opacity breaks Firefox's accessibility inspector, which doesn't take it into account when assessing contrast:

image

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 used opacity for simplicity, because it avoids having to define two additional colors for light and dark theme, and because it also applies to the "New" pill on the Tools menu (which would require another two colors).

TIL about Firefox's accessibility inspector ignoring opacity, but technically this PR increases contrast.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@LeoMcA Are you okay with merging this?

Copy link
Member

Choose a reason for hiding this comment

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

it also applies to the "New" pill on the Tools menu (which would require another two colors)

Why does it apply the the "New" pill? Isn't the point of that pill to draw attention? Why are we then making it stand out less?

but technically this PR increases contrast.

Does it? It decreases the contrast of the majority of links in the header. Taking a screenshot and colour picking the colour (#757575), then putting it in https://webaim.org/resources/contrastchecker/ shows we fail AAA contrast, which I don't think is good enough for some of the most prominent text on our site:

image

}

&:not(.active) > a {
&:link,
&:visited {
color: var(--text-active);
color: var(--text-secondary);
}

&:hover,
Expand Down
4 changes: 2 additions & 2 deletions client/src/ui/organisms/top-navigation-main/index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@
.top-level-entry {
background: none;
border-top: 1px solid var(--border-secondary);
color: var(--text-secondary);
color: var(--text-primary);
cursor: pointer;
display: flex;
padding: 1rem 0.5rem;
Expand All @@ -77,7 +77,7 @@

&:link,
&:visited {
color: var(--text-secondary);
color: var(--text-primary);
}

&.menu-toggle {
Expand Down