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.

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

1376: Add CSS to current navigation menu item #1379

Merged
merged 20 commits into from
Apr 22, 2022

Conversation

pedro-mendonca
Copy link
Member

@pedro-mendonca pedro-mendonca commented Apr 1, 2022

This allows to clearly identify the active navigation menu item.

  • Change home h1 to nav#home-navigation for better :hover consistency
  • Set .current background-color underline color to --gp-color-primary
  • Set :hover background-color underline color to --gp-color-primary-200 (the color immediately above primary in the lighter spectrum)
  • Remove .gp-bar left and right padding to keep the navbar and content aligned

Update:

  • Don't change home h1 link as reviewed
  • Use styling Option 3 (see options below) for styling

Screenshot

Projects .current and Locales :hover

imagem

Fixes #1376

Copy link
Member

@ocean90 ocean90 left a comment

Choose a reason for hiding this comment

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

I'd prefer something more subtle here. Maybe box-shadow: inset 0 -5px 0 var( --gp-color-secondary-900 ) and change to white on hover instead of current text underline?

image

@pedro-mendonca
Copy link
Member Author

pedro-mendonca commented Apr 1, 2022

@ocean90 it has too low contrast, hardly noted.
I agree on removing the underline.

The GlotPress title is wrapped in a <h1>, maybe should be removed too?

@pedro-mendonca pedro-mendonca requested a review from ocean90 April 20, 2022 10:14
@pedro-mendonca
Copy link
Member Author

Below are a few possible navbar solutions.

These examples have the Projects as the .current item, and the Profile is the :hover/:focus status.

  1. The current PR status
    imagem

  2. The @ocean90 suggestion on this comment
    imagem

  3. A third option, using the below box-shadow but with higher contrast on .current and using the glotpress --gp-color-primary variations.
    imagem

Please give feedback.

@amieiro
Copy link
Member

amieiro commented Apr 20, 2022

In my opinion, the third option is the best: it is subtle, but it has more contrast than if you use gray or white.

Copy link
Member

@ocean90 ocean90 left a comment

Choose a reason for hiding this comment

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

Using the primary color as an underline for the current item sounds good too. But there's no need to use different colors for the left and right menu, they can use the same colors to keep it simple.

@ocean90 ocean90 added this to the 3.1 milestone Apr 20, 2022
@pedro-mendonca
Copy link
Member Author

pedro-mendonca commented Apr 22, 2022

@ocean90 As I've mentioned above, the left and right colors show different status, on the left is a primary color to show the current status, and on the right I've placed a primary-color-200 to differentiate on hover on the current item.
Having a slight lighter color on hover allows is to have an hover effect also on current item.

@pedro-mendonca
Copy link
Member Author

pedro-mendonca commented Apr 22, 2022

@ocean90 and @amieiro I've changed to the option 3 in 7b57892

@pedro-mendonca pedro-mendonca requested a review from ocean90 April 22, 2022 13:41
@ocean90 ocean90 merged commit 931547a into GlotPress:develop Apr 22, 2022
@pedro-mendonca
Copy link
Member Author

Thanks for fixing the loose ends :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Navigation menu doesn't show 'current' item
3 participants