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

gatsby-link: add forwardRef #8399

Closed
wants to merge 1 commit into from

Conversation

ryanflorence
Copy link
Contributor

This allows other components that manage focus (like a dropdown) to manage focus on a gatsby link

This allows other components that manage focus (like a dropdown) to manage focus on a gatsby link
@ryanflorence
Copy link
Contributor Author

Flew this one in blind through the github edit interface as I haven't actually set up gatsby locally on my new machine, looks like all the test blew up, will get back to this soon.

Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

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

Hi Ryan 👋

Thanks for the PR. Yeah, as you mentioned, you'll probably want to get local version of Gatsby set up. I'd recommend checking out the How to Contribute docs if you haven't already. One of the unit tests is actually failing and the e2e tests are failing because of my mistake 😅

Particularly noteworthy is the gatsby-dev-cli section, which will be super helpful here. So what I'd recommend doing will look something like this:

  1. Set up gatsby-dev-cli and globally install it
  2. Create a test repo using this feature (doesn't need to be in this PR at all)
  3. yarn watch --scope=gatsby,gatsby-link
  4. Run gatsby-dev --set-path-to-repo [your-path-to-local-gatsby]
  5. Run gatsby-dev --scan-once --packages gatsby gatsby-link
  6. Run yarn develop to test the gatsby-link component 🎉

Let me know if you have any questions, hopefully this helps!

@pieh
Copy link
Contributor

pieh commented Sep 25, 2018

Is this still needed now that we don't wrap link component in Location HoC? ( https://github.com/gatsbyjs/gatsby/pull/8061/files ). Users can now do this in their code, right? I didn't use forwardRef so not sure here

@m-allanson
Copy link
Contributor

I think it's ok to close this? When #7546 is merged forwardRef handling will be added, but for now I think we're ok without. I put together a little test here: https://focus-gatsby-link.netlify.com/

Please re-open if I've got this wrong 👍

@m-allanson m-allanson closed this Dec 12, 2018
@m-allanson
Copy link
Contributor

Oh there's a newer PR for adding forwardRef also: #9892

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