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 LinkContainer.js #7546

Closed
wants to merge 1 commit into from
Closed

Add LinkContainer.js #7546

wants to merge 1 commit into from

Conversation

jquense
Copy link
Contributor

@jquense jquense commented Aug 22, 2018

I’m not quite sure this the right setup, but I think Gatsby really needs to prove a way to create links without needing to always render a DOM <anchor>. BEFORE anyone jumps in to say that that is not accessible, that’s silly. There isn’t any way without this to render navigation links from UI components, components that do render an anchor but also have other logic on top of it. Like react-bootstraps NavLink component. A presentational wrapper around an <a>

@jquense
Copy link
Contributor Author

jquense commented Aug 22, 2018

also what value is @react/router Link doing here? GatsbyLink seems to be overriding all the click logic?

@pieh pieh self-assigned this Sep 4, 2018
@pieh
Copy link
Contributor

pieh commented Sep 4, 2018

I think @reach/router Link currently just make use of getProps to add conditional props to underlying a element depending if page is active ( https://reach.tech/router/api/Link )

}
}

GatsbyLinkContainer.propTypes = propTypes
Copy link
Contributor

Choose a reason for hiding this comment

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

propTypes here have required location which is not not there at exported component level - it get's added by <Location> in code below

@KyleAMathews KyleAMathews force-pushed the master branch 5 times, most recently from fc4ca3e to 2116fff Compare September 17, 2018 20:36
@LekoArts
Copy link
Contributor

Hey @jquense 👋 I'm not too sure what your PR is doing, so that's why I'm asking:
Does #9892 solve your problem?

@jquense
Copy link
Contributor Author

jquense commented Jan 10, 2019

unfortunately no. This is about controlling what component Link renders as an anchor, not getting access to the ref

DSchau added a commit to DSchau/router that referenced this pull request Jan 10, 2019
Note: I don't necessarily know how to document this, but happy to make
that change too.

This is in regards to gatsbyjs/gatsby#7546, where a custom element needs
to be passed (e.g. a bootstrap link) and this provides that
functionality.

The API is similar to something like styled-components, and this change
is a non-breaking, backwards compatible API.

Let me know if you have any questions!
@DSchau
Copy link
Contributor

DSchau commented Jan 10, 2019

@jquense I think we can improve this behavior upstream, rather than introducing this workaround in our own code.

I've opened a PR for reach-router (reach/router#226) to accept an as prop, which will allow you to pass in whatever element you'd like. In theory, as long as your component forwards the ref (and you can easily make a wrapper for this for a bootstrap link, or whatever) then this would allow you to implement this functionality, correct?

Since we forward all props directly to the Link element from @reach/router, I think this will work as is without merging this PR (once/if the one in @reach/router is merged).

I'm not super familiar with react-bootstrap, so if you have an example that would be 👌 .

@DSchau
Copy link
Contributor

DSchau commented Feb 12, 2019

@jquense I'm going to close this--we should defer to that upstream behavior PR modification (reach/router#226) that will hopefully get merged soon.

I think that's a little cleaner and it's nice to get the benefit for free rather than making changes here.

As always--thanks for the PR. Sorry for the delay on this one!

@DSchau DSchau closed this Feb 12, 2019
@DSchau DSchau deleted the link-container branch December 5, 2019 04:53
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.

4 participants