Enhancement: added navItem hover

[mAmineChniti]

Description

Just a very simple change, added a couple of lines of css to have the top navBar that has Learn, About, etc.. have the same hover effect as the footer bar

Validation

Related Issues

This was suggested in:

6752

Check List

• [X] I have read the Contributing Guidelines and made commit messages that follow the guideline.
• [X] I have run npm run format to ensure the code follows the style guide.
• [X] I have run npm run test to check if all tests are passing.
• [X] I have run npx turbo build to check if the website builds without errors.
• [X] I've covered new added functionality with unit tests if necessary.

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
nodejs-org	✅ Ready (Inspect)	Visit Preview	Jun 14, 2024 1:17pm

[mikeesto]

Thanks for the PR. Personally I think this is a nice addition - I'm interested to hear others' thoughts

[AugustinMauroy]

+1 for theses change, I remember we already have discussion about hover effect but I didn't know in which pr.

[ovflowd]

I thought we discussed it before and decided not to do this. What changed now? 🤔

[AugustinMauroy]

I thought we discussed it before and decided not to do this. What changed now? 🤔

I used to agree with that. And if I remember correctly last time the proposal was that the hover effect should be the same as the active style for navigation, which was for ‘heavy’.

[canerakdas]

I do not think the links in the header and footer navigation items are buttons, therefore I do not think it is right for these elements to behave like buttons in the hover state

Here is why; https://a11y-101.com/design/button-vs-link

Maybe changing the text color would be better on hover, I'm not 100% against it as the current solution only works on hover 🙈

[ovflowd]

Maybe changing the text color would be better on hover, I'm not 100% against it as the current solution only works on hover 🙈

Agreed, and that's what I said on the last PR. This should at maximum change the text on hover, but this is a link, not a button.

[AugustinMauroy]

and why not just underline it when hover it's discreet and allow user to know there are possible interaction.

[mAmineChniti]

The footer links have this same effect on hover, should i go with underline or text color change and should i do it for the top navBar or both it and the footer?

[canerakdas]

and why not just underline it when hover it's discreet and allow user to know there are possible interaction.

Since we use horizontal lines to divide the design into sections(header, content, footer), I think the use of underlines in the texts would be overkill / retro look 👀 Wouldn't it be enough to indicate it with text color and cursor pointer?

[mAmineChniti]

@canerakdas I made the text color change to the nodejs green (text-green-400) on hover in both dark and light mode, its visible in both modes

[mAmineChniti]

I'm still very much a beginner but i have a take on this that i'm not sure if valid or not so please correct me on this: I think that the contrast between the previous color and the color on hover is much more important than the contrast between the new color and the background, having the text change from black to a lighter color that still looks visible enough with a white background draws the eyes better than going from black to another dark color that the user won't pick up on, the darker color has really good contrast with the bg but is not different enough from the previous color (black) so the user won't pick up on it that well

[ovflowd]

I think that the contrast between the previous color and the color on hover is much more important than the contrast between the new color and the background, having the text change from black to a lighter color that still looks visible enough with a white

Unfortunately, not. For accessibility standards, the contrast on the background color is way more important. There are a bunch of reasons why, and I believe you could study those, as these are standard UX and accessibility practices.

Maybe this blog post can help you out :) https://medium.com/@think_ui/why-color-contrast-is-not-as-black-and-white-as-it-seems-94197a72b005

[mAmineChniti]

@ovflowd Thank you.

[bmuenzenmeyer]

honestly, compared to where the PR has gone, I like the original proposal. The very reasons we shot down the proposal are now being ignored in the footer. So the inconsistency bothers me.

[canerakdas]

honestly, compared to where the PR has gone, I like the original proposal. The very reasons we shot down the proposal are now being ignored in the footer. So the inconsistency bothers me.

As I stated, I am not 100% against the original proposal. Instead of changing this development and leaving the original proposal completely, I can accept this proposal as it is and prepare a new proposal for a practice that I think is better

I should have stated these considerations and provided feedback earlier in the original proposal. After all, sorry for the late review/response 🫠

[bmuenzenmeyer]

@canerakdas yeah, it seems like we don't have consensus, even amongst ourselves. this means we should slow down and think about the experience we want before implementing anything.

if time allows, I'd love to see what you think.

and @mAmineChniti - sorry if this is stressful or comes across frustrating, our shared governance model here is truly trying to find the right outcome, and sometimes it's messy

[mAmineChniti]

@bmuenzenmeyer its really okay not stressful, @canerakdas if i may give my opinion from a simpleton user prespective i think flipping the current (this PR's) effects for footer and header would be better, from the header's side we do need the indicator for which page we're in (the green rectangle) and it'd be satisfying from a user prespective to hover on that link having that grey low opacity rectangle effect then clicking it and it fills out green, on the footer's side it'd be better to just have that simple color change since those link simply lead to documentations that are outside the site (pdf and github) so we don't need that rectangle shape page indicator, just an indicator that they're links (the color change)

[canerakdas]

I think we should proceed with the original proposal for this PR and then prepare a new proposal to evaluate in a different PR to address any UX concerns I mentioned

I believe/agree the current proposal has been accepted by the majority of the team and makes our components more consistent

My UX concerns are about a two out of ten (@bmuenzenmeyer I learned this from one of your reviews, I think we can use it here 😄)

[mAmineChniti]

@canerakdas so should i revert this to my original proposal?

[canerakdas]

@canerakdas so should i revert this to my original proposal?

Let's wait a bit and get feedback from the rest of the team 👀

I'm sorry for prolonging the review process a bit, we aim for an infrastructure that is accessible and easily maintainable. Thanks for being a part of this ❤️

[mikeesto]

FWIW I'm happy to support the initial proposal

[bmuenzenmeyer]

Same

[bmuenzenmeyer]

@ovflowd I know you are busy and feel free to ignore, but curious if you are accepting of this change. We've gone around and around, but want to make sure you had a chance to review it too.

if I don't hear from you, I will merge tomorrow

[ovflowd]

Appreciate tagging me. Honestly speaking I'm a -1 for the current proposal (how the PR stands)

But I won't go out my way to block the PR if I get reassurance the concerns Caner and I shared initially get addressed in a follow-up.

I understand the why's, but these are not buttons, but links.

Buttons do actions. Links do navigation. Anyhow, feel free to merge.

[AugustinMauroy]

I tend to agree with Claudio, but with these changes the footer and header will be in line. I'd be in favour of merging this pr and then opening an issue like ‘difference between link and action isn't clear on header/footer’.

[ovflowd]

It's less about being clear and more about being accessible and compliant imo

[bmuenzenmeyer]

It's less about being clear and more about being accessible and compliant imo

it is keyboard accessible.

the heart of the linked resource suggests that actual controls, like input elements shouldn't look like links.

[github-actions[bot]]

Lighthouse Results URL	Performance	Accessibility	Best Practices	SEO	Report
/en	🟢 97	🟢 100	🟢 96	🟢 91	🔗
/en/about	🟢 100	🟢 100	🟢 96	🟢 91	🔗
/en/about/previous-releases	🟢 96	🟢 100	🟢 100	🟢 92	🔗
/en/download	🟢 98	🟢 100	🟢 96	🟢 91	🔗
/en/blog	🟢 99	🟢 100	🟢 96	🟢 92	🔗

[github-actions[bot]]

Unit Test Coverage Report

Lines	Statements	Branches	Functions
	90.67% (593/654)	76.08% (175/230)	94.57% (122/129)

Unit Test Report

Tests	Skipped	Failures	Errors	Time
131	0 :zzz:	0 :x:	0 :fire:	5.254s :stopwatch:

[bmuenzenmeyer]

@mAmineChniti can you sync your fork with nodejs.org/main ?

[mAmineChniti]

@mAmineChniti can you sync your fork with nodejs.org/main ?

Done.

[bmuenzenmeyer]

thanks @mAmineChniti for coming with us on this journey. I hope you find more ways to contribute to the project!