added hover and focus for interactive element (header/footer) #217
Conversation
src/static/css/2019.css
Outdated
| @@ -235,6 +260,12 @@ footer .nav-items *:last-child { | |||
| .social-media img:hover { | |||
There was a problem hiding this comment.
Combine the selectors to avoid repeating the same styles.
| .social-media img:hover { | |
| .social-media img:hover, | |
| .social-media a:focus img { |
src/static/css/2019.css
Outdated
| filter: brightness(0) saturate(100%) invert(86%) sepia(4%) saturate(4357%) hue-rotate(16deg) brightness(136%) contrast(94%); | ||
| } | ||
|
|
||
| header.alt-bg a.interactive:hover, footer.alt-bg a.interactive:hover, |
There was a problem hiding this comment.
Why is a.interactive:hover needed and not simply a:hover? Are there some anchors that shouldn't inherit these interactive styles?
There was a problem hiding this comment.
Certainly, this is simple text a hover/focus, so this useless selector (interactive) can be removed.
I agree and fix it.
src/static/css/2019.css
Outdated
| a.navigation-logo img:hover { | ||
| filter: brightness(0) saturate(100%) invert(86%) sepia(4%) saturate(4357%) hue-rotate(16deg) brightness(136%) contrast(94%); | ||
| } | ||
| a.navigation-logo:focus img { | ||
| filter: brightness(0) saturate(100%) invert(86%) sepia(4%) saturate(4357%) hue-rotate(16deg) brightness(136%) contrast(94%); | ||
| } |
There was a problem hiding this comment.
Combine
| a.navigation-logo img:hover { | |
| filter: brightness(0) saturate(100%) invert(86%) sepia(4%) saturate(4357%) hue-rotate(16deg) brightness(136%) contrast(94%); | |
| } | |
| a.navigation-logo:focus img { | |
| filter: brightness(0) saturate(100%) invert(86%) sepia(4%) saturate(4357%) hue-rotate(16deg) brightness(136%) contrast(94%); | |
| } | |
| a.navigation-logo img:hover, | |
| a.navigation-logo:focus img { | |
| filter: brightness(0) saturate(100%) invert(86%) sepia(4%) saturate(4357%) hue-rotate(16deg) brightness(136%) contrast(94%); | |
| } |
rviscomi
changed the title
src/static/css/2019.css
Outdated
| header.alt-bg a.interactive:hover::after, footer.alt-bg a.interactive:hover::after, | ||
| header.alt-bg a.interactive:focus::after, footer.alt-bg a.interactive:focus::after { | ||
| content: ''; | ||
| border-bottom: solid 1px #F7F779; |
There was a problem hiding this comment.
What's the advantage of a border on the ::after pseudo-element versus text- decoration: underline on the anchor itself?
Underline:
Border:
I also noticed with the border, the link shifts vertically by a few px on hover. Is that intentional?
There was a problem hiding this comment.
@rviscomi
I'm sorry, there was no deep thought in this, it was my mistake.
(This is a remnant of trying out focus decorations)
I agree this should fix be text-underlined.
There was a problem hiding this comment.
No worries! This is looking great! 🙌
|
One question about border vs underline, otherwise this looks great. Thanks for working on it! |
| filter: brightness(0) saturate(100%) invert(86%) sepia(4%) saturate(4357%) hue-rotate(16deg) brightness(136%) contrast(94%); | ||
| } | ||
|
|
||
| header.alt-bg a:hover, footer.alt-bg a:hover, |
There was a problem hiding this comment.
The "Start Exploring" button in the header appears to have an underline when hovered/focused, which I like:
But the same button in the intro section does not:
Could you make sure this behavior is consistent for all buttons and an underline is shown?
There was a problem hiding this comment.
To clarify, I do like the underline for buttons, I just want to make sure they're shown consistently 😄
There was a problem hiding this comment.
@rviscomi
I will start this task!
Try to underline the button explicitly.
There was a problem hiding this comment.
@rviscomi
During the focus test, I found problem in my environment (chrome and firefox for mac os).
At a certain resolution(width: 1480px), when there is a overflow content of position: relative/absolute, can not touch it, so the button to disabled focus.
Setting z-index: 1 to button avoids the problem, but do you know a smarter way?
There was a problem hiding this comment.
notebook:
Even if width 599px, can not press some of Contributors buttons.
There was a problem hiding this comment.
Yeah I noticed that too. I have a fix in #212: https://github.com/HTTPArchive/almanac.httparchive.org/pull/212/files#diff-dcfbc15530630ce0591f21b62fbb1237R217 (disabling pointer events for the "91" text)
There was a problem hiding this comment.
^ That PR was just merged so this should be fixed for you now.
There was a problem hiding this comment.
@rviscomi
I was saved!
this I good experience and I will complete the rest of the tasks.
src/static/css/2019.css
Outdated
| color: #F7F779; | ||
| outline: none; | ||
| } | ||
| header.alt-bg a.interactive:hover, footer.alt-bg a.interactive:hover, |
There was a problem hiding this comment.
I think this can be simplified to a:hover, a:focus { text-decoration: underline; }
Are there any cases where we don't want a link to have an underline?
There was a problem hiding this comment.
I agreed 👍
Yes, I working on it now.
|
Looks great! Thanks for working on this @MSakamaki! |
|
Thank @rviscomi for checking at the code details! |
#210
A unified hover/focus has been applied to the interactive elements of the header and footer.