-
Notifications
You must be signed in to change notification settings - Fork 1
chore: highlight menu items for individual blog articles #105
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
Conversation
padding: var(--space-s) var(--space-2xs); | ||
} | ||
|
||
.site-nav__links .is-active > a { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is causing a double line to appear since we have an underline on both the list element and the anchor element.
This happens since the identifier .site-nav__links .is-active
sometimes underlines a list item so it gets two underline lines now.
We can change the selector to ensure anchor elements are only ever underlined and also include the text-underline-offset
here so we can just get rid of the other selector later:
.site-nav__links a.is-active, .site-nav__links .is-active > a {
text-decoration-color: currentColor;
text-underline-offset: 8px;
}
But then we must also remove this as it appears below:
.site-nav__links .is-active {
text-decoration: underline;
text-decoration-color: currentColor;
text-decoration-thickness: 2px;
text-underline-offset: 8px;
- }
- ```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For context the first items in the menu weren't always links, so that may be part of why they used different selectors before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're now defining the text-underline-offset
at the top we can also remove the selector below:
.menu-item--level-1 > a {
text-underline-offset: 8px;
and move this one:
.menu-item--level-2 > a {
text-underline-offset: 4px;
The 4px offset only ever applies for small screens so we can move that under the @media screen and (max-width: 1159px)
query.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also remove:
.menu-item--level-2 > .is-active {
text-decoration: underline;
}
Since we've applied that to the top and everything we underline is an anchor element so this is redundant now.
Can you also explain the Safari issue that you found? I saw a mention of it in your commit messages. |
Highlight menu items (Content Hub and Foundation Blog) for individual blog articles