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

Investigate using Reach Router #5656

Closed
m-allanson opened this issue Jun 1, 2018 · 11 comments · Fixed by #6918
Closed

Investigate using Reach Router #5656

m-allanson opened this issue Jun 1, 2018 · 11 comments · Fixed by #6918
Assignees
Labels
help wanted Issue with a clear description that the community can help with.

Comments

@m-allanson
Copy link
Contributor

Summary

Gatsby uses React Router under the hood. Reach Router is a new DOM-based router for React with accessibility built in. Influenced by React Router and built by Ryan Florence: https://reach.tech/router/accessibility

Investigate the tradeoffs involved in swapping out React Router for Reach Router.

Motivation

Adding 'accessiblity by default' is a win for developers and site visitors. As a bonus it looks like Reach Router has a smaller bundle size compared to React Router.

@m-allanson m-allanson added help wanted Issue with a clear description that the community can help with. 🏷 type: feature labels Jun 1, 2018
@agarwalkartik
Copy link

Hey @m-allanson , I would love to help on this issue. I am new to Gatsby but I know React and React Router.
Please mention more about -

  • What metrics do we need to consider while evaluating the tradeoffs.
  • What do you mean by 'accessibility by default'

Thanks!

@ryanditjia
Copy link
Contributor

This recent issue talks about accessibility. And here is Reach Router’s section on accessibility.

@kartik-koinex
Copy link

@ryanditjia Thanks for the information. Can you also tell "What metrics do we need to consider while evaluating the tradeoffs".

@KyleAMathews
Copy link
Contributor

I've been chatting with @ryanflorence some about this change and it seems pretty compelling. It's a mostly drop-in replacement which fixes our current accessibility problems + reduces the size of our routing layer fairly significantly. Relative link support also sounds nice as it'll allow people to build more mini-apps that they can just drop into Gatsby sites wherever as needed.

@KyleAMathews
Copy link
Contributor

Update on this! @ryanflorence and I are pairing tomorrow to work on getting @react/router in.

@DylanVann
Copy link

@KyleAMathews People just figured out how to work around lack of layout components on this issue: #6127

This change now breaks that. Not being able to tell wether something is a push or a pop will also affect people trying to do animations based on routing.

There are some compelling reasons for making this change, but the number of breaking changes for V2 is getting to be a bit much.

I also have a situation where I need to listen for history changes which I can't see how to do with this @reach/router setup because there's no access to history.

@KyleAMathews
Copy link
Contributor

@DylanVann Hey! Agree that things have been a bit rough the past couple of days. But it's very fixable. Could you file an issue w/ more details about what you're seeing + a reproduction site showing where things are broken? We can look at things and @ryanflorence has some time to help make fixes to @reach/router where necessary.

@ryanflorence
Copy link
Contributor

ryanflorence commented Aug 8, 2018

I can add push/pop, but I'd love to know how people use them with animations, since with "pop" you don't know if it's forward or backward or even how far forward or backward in the stack you went. I suspect those animations are broken. You probably ought to rely on location.key in your own stack, if you haven't seen a key, it's a push, if you have, it's a pop.

Anyway, we can add history.action but I'd like to see the use cases to make sure it meets your needs.

As for listening to history for changes, are you sure you can't do the same thing with components?

<Location>
  {({ location }) => (
    <ThingThatDoesStuffWith location={location}/>
  )}
</Location>

class ThingThatDoesStuffWithLocation extends React.Component {
  componentDidUpdate(prevProps) {
    if (prevProps.location !== this.props.location) {
      // do the thing
    }
  }
  render() {
    return null
  }
}

But indeed, you can listen to the history if you'd like to:

import { globalHistory } from '@reach/router/lib/history'

globalHistory.listen(() => {
  // do your thing
})

@DylanVann
Copy link

@KyleAMathews @ryanflorence Thanks so much for the help.

As far as specifics the three things I'm trying to do right now are:

  1. The specific issue around listening to history is that I have a menu with some buttons, when you click one it needs to close the menu. I had used history.listen before, but it's also possible to use a generic event emitter and onClick to get the same thing, which is what I've switched to doing. Thanks for pointing out globalHistory though, that would also work.
  2. In order to get CSS transitions working for something like a top bar you need a layout component wrapping your pages, at least it seems this way with how Gatsby currently works. That's what the linked issue is about, at least partially. With the current API's (gatsby-ssr.js & gatsby-browser.js) (after this change), I haven't been able to get that working.
  3. Aside from these CSS transitions I need to get more complex page transitions working, but I haven't got started on trying this since I need to solve 2. first.

For animations I vaguely remember using action types with react-router for React Native to do different animations depending on whether a user was going backwards or forwards. I'm not sure I'll even need this with Gatsby I was just anticipating that others may need those. Just a possible unintended consequence of this change, but maybe it's not relevant.

@fabe
Copy link
Contributor

fabe commented Aug 9, 2018

@DylanVann @KyleAMathews @ryanflorence I've been trying to implement component-specific page transitions the past couple days and wasn't able to get it working with Reach Router. Adding a TransitionGroup around the component renderer works fine and I'm able to fade-in/out from page to page, but I'm more after something like this. To get it working in react-router before, we were using getUserConfirmation together with a setTimeout, taken from the official example:

gatsby-browser.js at examples/using-page-transitions:
const getUserConfirmation = (pathname, callback) => {
  const event = new CustomEvent(historyExitingEventType, { detail: { pathname } })
  window.dispatchEvent(event)
  setTimeout(() => {
    callback(true)
  }, timeout)
}

...which is not possible anymore. I'm not sure if something like this would go against the accessibility concern of Reach Router, but I think this should be possible in Gatsby. Any thoughts?


EDIT: Finally found a way using React Pose, here's a great guide. I implemented this in one of my starters.

@Kadrian
Copy link

Kadrian commented Sep 13, 2018

Hey, I've been using connected-react-router before to dispatch redux actions like push(). Is there a way to do this with reach router?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Issue with a clear description that the community can help with.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants