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

Add active state styles for top navbar items #7429

Conversation

arzafran
Copy link

Background

The active item in doc's top navbar wasn't being highlighted, hence #7425

How I've fixed it:

  • Added activeClassName="active" to navigation.js items
  • Added styles for items with the class active

Screenshots:

BEFORE
screen shot 2018-08-17 at 19 03 18

AFTER
screen shot 2018-08-17 at 19 03 27

@Manoz Manoz requested a review from fk August 18, 2018 06:08
@Manoz Manoz added the type: maintenance An issue or pull request describing a change that isn't a bug, feature or documentation change label Aug 18, 2018
@Manoz
Copy link
Contributor

Manoz commented Aug 18, 2018

Good catch @arzafran 👍

@arzafran
Copy link
Author

I'm not sure why tests are not passing, and didn't understand how to debug failing ci.
Any help would be appreciated :)

@KyleAMathews
Copy link
Contributor

There are some failing tests on windows (appveyor). Doesn't seem related to this PR so they're probably problems on master that are affecting your PR as well. Sorry about the confusion!

@arzafran
Copy link
Author

Thanks for the clarification!

Copy link
Contributor

@fk fk left a comment

Choose a reason for hiding this comment

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

Hey @arzafran!
Thank you so much for jumping on this! 🤗

As mentioned in the issue, I was sceptic if font-weight: bold would be a good idea, but this is working quite well and I think we should get this in while discussing if we want to go down a different path in terms of styling.

However, one thing that I think we should take care of is that the active styles currently only are applied if the location.pathname exactly matches the href of the link -- e.g when visiting /docs/gatsby-starters/, "Docs" won't be active -- currently it only is for /docs/. Would be 💯 if you could take a look at this as part of this PR!

I asked @KyleAMathews how to do this and he pointed me to https://next.gatsbyjs.org/docs/migrating-from-v1-to-v2/#the-following-props-are-no-longer-available-on-link and @reach/router's getProps.

@arzafran
Copy link
Author

hey! sure, I'll take a look later today! thanks.

@fk
Copy link
Contributor

fk commented Aug 20, 2018

Yay! Thank you @arzafran! 🤗

I don't know anything about getProps until now and programming doesn't come easy to me, so I might not be able to help, but others surely can -- so please let us know if you're running into problems!

@shannonbux
Copy link
Contributor

shannonbux commented Aug 21, 2018

@fk #899 is in ichabod-cranes so don't think people can see it perhaps?

Also I moved this issue and #899 into the [EPIC] Docs IA Redesign cleanup epic here:
#7427

Thanks for working on this, @arzafran!

@fk
Copy link
Contributor

fk commented Aug 22, 2018

Hey @arzafran, I just stumbled upon #7526 (comment) which shows how to do this.

@fk
Copy link
Contributor

fk commented Aug 22, 2018

@shannonbux Added a new issue to capture the main navigation expansion: #7544

@fk
Copy link
Contributor

fk commented Sep 7, 2018

Hey @arzafran, I just merged #7933 by @kkemple which adds the active state for the top navbar items. I hope you don't mind 🙏— thanks for your work on this! 🤗

@fk fk closed this Sep 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: maintenance An issue or pull request describing a change that isn't a bug, feature or documentation change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants