Fix the onClick override logic #1489
Conversation
Solution is almost a direct copy/paste from react-router's Link component (as suggested): https://github.com/ReactTraining/react-router/blob/676e8ac14da780e9fbd32 7576971fe549f3d0b8c/packages/react-router-dom/modules/Link.js#L40-L43 Now, cmd/ctrl-clicking opens links in a new tab, as expected! Also, target="_blank" now works.
|
Deploy preview failed. Built with commit 1fb1fb6 https://app.netlify.com/sites/using-styled-components/deploys/596810a3cf321c60dcf87113 |
|
The gatsby-link package currently doesn't have a test setup. Should I add a test for this change? Also, let me know if the code doesn't pass your desired style guide 👍 |
|
Deploy preview failed. Built with commit 980d098 https://app.netlify.com/sites/image-processing/deploys/5966c94a6f4c5074d8a1ddcd |
|
Deploy preview failed. Built with commit 45dd1a3 https://app.netlify.com/sites/gatsbyjs/deploys/596ea3698ebdd92c5150f3be |
|
Deploy preview ready! Built with commit 45dd1a3 |
|
Deploy preview ready! Built with commit 45dd1a3 |
|
Thanks for the fix! Looks like it's working now 🎉 Could you not break out the new code though into separate functions? There's very little code in the new functions so I think it makes things harder to follow. |
|
Sure thing! I'll adjust it 👍 |
|
Oh also the checks for production and if we should handle the click or not don't have anything to do with each other. Put the checks on handling the clicks at the top of the function. |
|
Right, this whole onClick handler should be wrapped inside an if statement. I'll adjust |
packages/gatsby-link/src/index.js
Outdated
| @@ -73,8 +55,27 @@ class GatsbyLink extends React.Component { | |||
| !e.ctrlKey && | |||
| !e.shiftKey | |||
| ) { | |||
There was a problem hiding this comment.
How's this looking to you now? :)
If I misunderstood your instructions, let me know and I can adjust further.
Note that I kept onClick && onClick(e) outside the if statement so that it still can be overridden if need be.
There was a problem hiding this comment.
Nope, looks great! Just been traveling recently. Merging now and will be prepping new release soon, thanks!
|
Yay! Thanks for merging @KyleAMathews 😄 |
|
Hiya @jakedeichert! 👋 This is definitely late, but on behalf of the entire Gatsby community, I wanted to say thank you for being here. Gatsby is built by awesome people like you. Let us say “thanks” in two ways:
If you have questions, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’. Thanks again! 💪💜 |
1 similar comment
|
Hiya @jakedeichert! 👋 This is definitely late, but on behalf of the entire Gatsby community, I wanted to say thank you for being here. Gatsby is built by awesome people like you. Let us say “thanks” in two ways:
If you have questions, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’. Thanks again! 💪💜 |
|
@jlengstorf wow, this is so nice! Thank you! 😄 |
I took @KyleAMathews suggestion and looked into react-router's Link component handleClick logic. This solution is almost a direct copy/paste of the important parts that gatsby-link needs in order for the click logic to behave more as expected.
Now, cmd/ctrl-clicking opens links in a new tab, as expected! Also, target="_blank" now works too.
Fixes #1417