-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
feat(gatsby-link): adds support for partiallyActive=true to Link #12495
Conversation
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 seems reasonable--thanks so much for doing this!
Would you be interested in updating the documentation for this, as well? This seems like a feature people have been wanting for a while--so documenting it would the cherry on top!
Here's the cherry! I won't have the time to make an egghead video, though 😄 |
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 a ton cleaner, and I appreciate the tests and the documentation updates.
We'll have to just table the Egghead videos ;)
Thanks for this! Merging and releasing in a sec.
|
curious about that gatsby-link version—I'm on latest Gatsby ( Are we not supposed to import gastby-link via Gatsby anymore? |
@dustinhorton try running |
@lmcjt37 what's the |
@dustinhorton sorry my fault, that's second nature to add the --save flag. |
Now on Tried with both |
@dustinhorton Do you have an example of how you're using Gatsby-Link? Maybe I can help identify the issue |
@lmcjt37 Thanks for the offer—caused me to take another fresh look and I realized the error in my implementation (just some leftover destructuring from when I was handling it myself). |
@dustinhorton Awesome! Glad you managed to fix it 👍 |
Thanks for doing this. As I'm bringing my navigation in dynamically I had to add a conditional to the activeClassName to stop the homepage menu item being rendered as active. Leaving this here as it might help someone.
|
I ran in to a similar issue where the home <Link
key={page.name}
className={navigationStyles.navLink}
to={page.link}
partiallyActive={true}
activeStyle={page.link === '/' && pathname !== '/' ? {} : { background: "#0084B4" }}
>
{page.name}
</Link> It aint pretty but it works... Here's the navigation component where I used it, for context. I also had to import import React from 'react'
import { Link, StaticQuery } from "gatsby"
import navigationStyles from "./navigation.module.scss"
import { Location } from '@reach/router';
const Navigation = props => {
const { menuLinks } = props.siteMetadata
const { pathname } = props.location
return (
<div className={navigationStyles.siteNavigation}>
<nav >
{menuLinks.map(page => (
<Link
key={page.name}
className={navigationStyles.navLink}
to={page.link}
partiallyActive={true}
activeStyle={page.link === '/' && pathname !== '/' ? {} : { background: "#0084B4" }}
>
{page.name}
</Link>
))}
</nav>
</div>
)
}
export default () => (
<Location>
{locationProps => <StaticQuery
query={graphql`
query {
site {
siteMetadata {
menuLinks {
name
link
}
}
}
}
`}
render={data => <Navigation {...locationProps} siteMetadata={data.site.siteMetadata} />}
/>}
</Location>
) Thanks for the tip, @imshuffling |
Description
Implements a new option in the
<Link/>
component:partiallyActive={bool}
. If true, the route will activate itsactiveStyle
andactiveClassName
parameter even if only partially active.I've put the settings on the component rather than the route, as there might be places where you want this settings to be on and others where you want it to be off. Also it was simpler 🙂
I've also moved
@reach/router
into the peerDependencies (from the regular dependencies) since it's a stateful package (due to the context ref) and you don't want to run the risk of having version conflicts. It shouldn't be a problem because thegatsby
package already provides@reach/router
.Related Issues
Fixes #7208