-
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
Replace react-router with @reach/router #6918
Conversation
Deploy preview for using-postcss-sass failed. Built with commit 06d9c77 https://app.netlify.com/sites/using-postcss-sass/deploys/5b682dae73f2cf4f594f7750 |
Deploy preview for using-glamor failed. Built with commit 06d9c77 https://app.netlify.com/sites/using-glamor/deploys/5b682daf73f2cf4f594f7759 |
Deploy preview for using-contentful failed. Built with commit 06d9c77 https://app.netlify.com/sites/using-contentful/deploys/5b682db073f2cf4f594f775f |
Deploy preview for using-jss failed. Built with commit 06d9c77 https://app.netlify.com/sites/using-jss/deploys/5b682db373f2cf4f594f7777 |
Deploy preview for using-drupal failed. Built with commit 06d9c77 https://app.netlify.com/sites/using-drupal/deploys/5b682dae73f2cf4f594f774a |
Deploy preview for gatsbygram failed. Built with commit 06d9c77 https://app.netlify.com/sites/gatsbygram/deploys/5b682dae73f2cf4f594f7747 |
Deploy preview for using-remark failed. Built with commit 06d9c77 https://app.netlify.com/sites/using-remark/deploys/5b682db073f2cf4f594f7762 |
Deploy preview for gatsbyjs failed. Built with commit 06d9c77 https://app.netlify.com/sites/gatsbyjs/deploys/5b682dae73f2cf4f594f7744 |
I'm doing an overview call on this in 10 minutes if you'd like to join! https://zoom.us/j/100692813 |
packages/gatsby-link/src/index.js
Outdated
} | ||
} | ||
|
||
// Make sure the necessary scripts and data are | ||
// loaded before continuing. | ||
e.preventDefault() | ||
window.___push(this.state.to) | ||
if (process.env.NODE_ENV === `production`) { |
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.
Remove production check
|
||
setApiRunnerForLoader(apiRunner) |
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.
remove
Deploy preview for using-styled-components failed. Built with commit bc7035c https://app.netlify.com/sites/using-styled-components/deploys/5b682bab792f8943ae9ddcec |
packages/gatsby-link/src/index.js
Outdated
getProps = ({ isCurrent }) => | ||
isCurrent | ||
? { | ||
className: this.props.activeClassName, |
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.
I think we need to add default className
(similiar how styles are merged in line below)
react-router
's NavLink
does that - https://github.com/ReactTraining/react-router/blob/master/packages/react-router-dom/modules/NavLink.js#L41-L42 if we want to maintain backward compatibility
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.
Ah good catch, fixed in 33583a2
Deploy preview for image-processing failed. Built with commit 7c61690 https://app.netlify.com/sites/image-processing/deploys/5b681c3482d3f128bebdcdb4 |
Deploy preview for image-processing failed. Built with commit bc7035c https://app.netlify.com/sites/image-processing/deploys/5b682baa792f8943ae9ddce6 |
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.
LGTM!
Any idea on when docs would be updated for the client only routes using |
@KyleAMathews Thanks for that, and thanks for all of the work you are doing on Gatsby! |
Ohh no I got confused because of |
I think |
@jorgegonzalez tried |
* Get @reach/router working probably in development * Moer stuff working * Don't support to as an object * Add back support for activeClassName & activeStyle * Fix path for RouteHandlers * Pull in parse-path util from react-router * remove console.logs and add TODO * Remove now unused webpack rules to trim down react-router * Some fixes from merge * Fix problems identified in review earlier * Remove old typescript definitions * Restore and update the <Link> documentation for v2/@reach/router I also added it to the API reference section — which seemed the right place for it. @shannonbux agree? * Also set the className when the site is active per @pieh's advice * Upgrade client-only-paths example site * Upgrade simple-auth example site * Fix lint errors * Migration docs * Note that can't use relative routes w/ @reach/router * Fix/remove tests that are irrelevant now * Fix imports * Use v2 version of children for layout * mini typos * Document that history prop no longer passed to page components
fixes #5656
Smaller package + better accessibility + simplified APIs 👍
https://reach.tech/router/
TODO
Upgrade sites
Document
Breaking changes to document
to
any longer. Add search/hash yourself. Add state tostate
prop.path
propactiveClass
/activeStyle
props. If want more control, you can usegetProps
prop