Add guidance on how to use EUI with react-router. #810

cjcenizal · 2018-05-09T22:04:39Z

Fixes #507

@mattapperson You're using RR v4 right? Could you verify that my guidance here is technically correct?

@bevacqua, @pugnascotia, @zinckiwi Would you mind sharing your thoughts on this?

bevacqua · 2018-05-09T22:16:32Z

wiki/react-router.md

+use either the `href` or `onClick` values, or both. It's also terser than the second option.
+
+```jsx
+<EuiLink {...toHrefAndOnClick('/location')}>Link</EuiLink>


I think toHrefAndOnClick might be a bit too precise / attached to implementation details. Would something along the lines of getRouterLinkProps('/location') make more sense?

Good idea! Will change.

bevacqua · 2018-05-09T22:18:09Z

wiki/react-router.md

+[`render` prop](https://reactjs.org/docs/render-props.html).
+
+```jsx
+const RrAdapter = ({to, render}) => {


I like this pattern, but not a fan of the Rr* naming, I think it's pretty obscure, and it's not like people are going to have more than a single routing library in their codebase (here's hoping), so how about naming this RouteAdapter instead?

bevacqua · 2018-05-09T22:20:35Z

wiki/react-router.md

+
+  componentDidUpdate() {
+    // Re-expose the router after a hot reload.
+    this.exposeRouter();


In Cloud we check for production and hot loading, maybe we should mirror that here as well:

if (process.env.NODE_ENV !== `production` && module.hot) { // … }

Good point. I'll make a comment for this, but I'd like to avoid adding code that's not germane to routing, in an effort to keep the examples focused.

We can move the whole thing to a side note then, right? I mean, hot reloading isn't strictly speaking related to routing either

Sure, that sgtm. What does a side note look like?

bevacqua · 2018-05-09T22:22:09Z

wiki/react-router.md

+
+let router;
+export const registerRouter = reactRouter => {
+  router = reactRouter;


I don't think we need to bother returning an unregister callback and implementing a componentDidUnmount that frees this up, right?

I don't think so. I think that would be an unusual situation should it arise, but I also think that if it did come up the consumer would be able to figure out that need and make this change without guidance.

bevacqua · 2018-05-09T22:22:56Z

wiki/react-router.md

+
+// App is your app's root component.
+class App extends Component {
+  static contextTypes = {


Should we bother with propTypes for a simple usage example? I'd avoid doing this

Not sure about the new context API, but the original context API requires you to declare contextTypes for you to access this.context.

bevacqua

This is great work, and certainly helps newcomers to onboard themselves with EUI!

There's a bit of duplication between the examples, but I'm not entirely sure how that could be improved.

cjcenizal · 2018-05-09T23:01:47Z

Thanks @bevacqua, I addressed your feedback.

cjcenizal · 2018-05-09T23:12:09Z

wiki/react-router.md

+
+<RouterLinkAdapter
+  to="/location"
+  render={(onClick, href) => <EuiLink onClick={onClick} href={href}>Link</EuiLink>}


@chandlerprall I stuck with the render prop instead of using children, because that's how they do it in the examples in the React docs, which I link to above.

bevacqua · 2018-05-09T23:48:40Z

wiki/react-router.md

+    this.registerRouter();
+  }
+
+  componentDidUpdate() {


I'd move this to the bottom of the react-router 3.xsection

- componentDidUpdate() { - // If using HMR, you'll need to re-register the the router after a hot reload. Note that - // you may want to add some conditions here to cull this logic from a production build, - // e.g. `if (process.env.NODE_ENV !== `production` && module.hot)` - this.registerRouter(); - } // … + #### Hot Module Reloading + + When using HMR, you'll need to re-register the router after a hot reload. + We encourage adding conditionals here to cull this logic from a production build, like so: + + ```js + componentDidUpdate() { + if (process.env.NODE_ENV !== `production` && module.hot) { + this.registerRouter(); + } + } + ```

(Probably just after this file rather than the bottom of this section)

Great, thanks!

pugnascotia · 2018-05-10T10:00:14Z

wiki/react-router.md

+
+### Hot module reloading
+
+Note that if using HMR, you'll need to re-register the router after a hot reload.


Be aware that v3 won't hot-reload changes to the routes (this is deliberate). A hard refresh is needed.

++ Good point, but I don't think we need to mention this here since that won't have an effect on how EUI integrates with the router.

pugnascotia · 2018-05-10T10:19:16Z

wiki/react-router.md

+      return;
+    }
+
+    // Prevent regular link behavior, which causes a browser refresh.


At this point, react-router also executes the following, which seems sensible.

// If target prop is set (e.g. to "_blank"), let browser handle link. if (this.props.target) return;

Nice spot! Though this function has no knowledge of props so we can't do the same kind of logic. If a consumer sets target="_blank" then they don't need to use this conversion function anyway since they're no longer integrating with the router (they can just use a plain old EuiLink or whatever).

The function can look for a target attribute from the onClick's event.target.getAttribute('target'). Probably should add that to be safe.

Good idea! Done.

pugnascotia · 2018-05-10T10:54:32Z

wiki/react-router.md

+
+### Share `router` globally
+
+To enable these techniques, you'll need to make the `router` instance available outside of React's


Did you try using the withRouter HOC on App? That provides router, only in v3 though.

Oh cool, I didn't know about this HOC. Looking through the docs, it looks like the only useful object this provides you with is history, which does provide the push method but not the createHref method, which seems to be only available on router.

I haven't dug deep enough into this method to understand its role and decide if we can work around it, but I think it's better to provide reliable guidance than clever guidance so I think we should just use the current example, since it's drawn directly from the react-router source.

I did specifically say v3 - in that version, withRouter() gives you a router prop, but they don't expose that in v4. AFAIR it's exactly the object you'd hope it is.

Ahh I didn't realize I was looking at the v4 docs. Do you know where I can find the v3 docs? All I could find wrt withRouter was the source.

https://github.com/ReactTraining/react-router/blob/v3/docs/API.md#withroutercomponent-options

Thanks, added a note.

pugnascotia · 2018-05-10T11:05:49Z

For the typical case of "I just want to style my links correctly", this feels like a lot of work. The docs say,

EUI doesn't prescribe the use of any particular routing library

but then goes on to describe various strategies and code examples, and you just know that someone is going to think, "Ach, I'll just put the classes on the router's <Link> component." So perhaps this guide needs to address that, and explicitly make the case against doing that?

Can EUI provide some of the onClick code in a service, perhaps?

I still like the idea of supporting a function-as-child-prop, e.g.

<EuiLink>
  { props => <Link to='/some/router/path' { ...props } /> }
</EuiLink>

but I appreciate that this isn't so useful with EUI components that wrap links. That being said, the breadcrumbs component could support the same pattern? I noticed that description_list.js allows you to specify either a data structure or children, and I appreciated that level of flexibility recently.

You could go even more bananas and move the majority of the EuiLink logic into a service. That would be interesting.

I'll stop now.

mattapperson · 2018-05-10T12:41:51Z

This is going to be a much... bigger... idea. But what if we used reacts context feature?

Do something like:

<EUIProjectContext options={{
  onNavigation={href => this.props.history.push(href)}
}}>
  <EuiLink href="http://www.elastic.co">
     Elastic website
  </EuiLink>
</EUIProjectContext>

EuiLink and other linkable components could then just call onNavigation from context if it exists.

This way:

every link, tab, step, and button does not need to be wrapped
The pattern allows for additional wrapping/bindings as EUI grows

Thoughts?

cjcenizal · 2018-05-10T12:56:42Z

@pugnascotia

"Ach, I'll just put the classes on the router's component." So perhaps this guide needs to address that, and explicitly make the case against doing that?

Good call, will do!

Can EUI provide some of the onClick code in a service, perhaps?

We could, but should we target RR 3 or 4? How about 5 when it comes out? What about other routers? How do we create tests or examples for these services? I think once we make EUI responsible for providing routing solutions, we assume a significant long-term maintenance burden which is definitely outside of EUI's mandate. The alternative is to let the consumer copy-paste the code examples we've shared and put them somewhere in their codebase. At that point, the maintenance burden for them is 0, at least until they change or upgrade their router. Comparing the two, the latter option seems optimal.

I still like the idea of supporting a function-as-child-prop

I think this is an interesting idea too. But it's a fairly fundamental change in how we approach component design and we'd need to make sure our interfaces are consistent across all our components. So if we can identify some other cases where this is useful, beyond just supporting react-router's Link component (which is becoming the bane of my existence! 😄), then I would be interested in pursuing this. But if this change only benefits consumers of Link then I don't think the juice is worth the squeeze.

@mattapperson I think this is a VERY intriguing idea. @chandlerprall Is this close to the idea you had, too? Thinking it through a bit, this seems like a terrific idea. It complements the ideas in this PR, but it reduces the integration points between react-router and EUI from n to 1. Could you break your comment out into its own issue and cross-reference this PR? It seems like something we can explore separately without blocking this PR.

mattapperson · 2018-05-10T13:01:16Z

@cjcenizal done. with that in mind ill approve this PR as my only other comments are addressed by others already

cjcenizal · 2018-05-10T13:23:28Z

@pugnascotia Added a note about putting EUI classes on <Link>.

chandlerprall

Update the generated onClick handler to watch for target attribute

chandlerprall · 2018-05-10T15:43:47Z

wiki/react-router.md

+      return;
+    }
+
+    // Prevent regular link behavior, which causes a browser refresh.


The function can look for a target attribute from the onClick's event.target.getAttribute('target'). Probably should add that to be safe.

chandlerprall · 2018-05-10T16:14:10Z

I would like to see the discussion around @mattapperson's proposal in #814 before continuing improving this PR. The two ideas are very different approaches to solving the same problem and it doesn't make sense to support both.

cjcenizal requested review from chandlerprall, nreese, bevacqua, jen-huang, zinckiwi, pugnascotia and mattapperson May 9, 2018 22:04

cjcenizal force-pushed the react-router branch 3 times, most recently from 8b53525 to 68845df Compare May 9, 2018 22:07

Add guidance on how to use EUI with react-router.

56f2c8c

cjcenizal force-pushed the react-router branch from 68845df to 56f2c8c Compare May 9, 2018 22:08

bevacqua reviewed May 9, 2018

View reviewed changes

bevacqua approved these changes May 9, 2018

View reviewed changes

cjcenizal force-pushed the react-router branch 2 times, most recently from 13643b9 to e442c58 Compare May 9, 2018 23:06

cjcenizal commented May 9, 2018

View reviewed changes

Rename things and add comments for clarity.

c46ed1c

cjcenizal force-pushed the react-router branch from e442c58 to c46ed1c Compare May 9, 2018 23:43

bevacqua reviewed May 9, 2018

View reviewed changes

cjcenizal force-pushed the react-router branch from 0b4754c to 33e2d97 Compare May 9, 2018 23:55

Move HMR stuff into a side note.

2a1bf27

cjcenizal force-pushed the react-router branch from 33e2d97 to 2a1bf27 Compare May 9, 2018 23:59

Use fn-as-children form of render props.

fee1dde

pugnascotia reviewed May 10, 2018

View reviewed changes

mattapperson mentioned this pull request May 10, 2018

Add Context component to enable single point of integration with react-router #814

Closed

mattapperson approved these changes May 10, 2018

View reviewed changes

Add note re non-recommended techniques.

1d01753

Add note re withRouter.

d0dfcb4

chandlerprall suggested changes May 10, 2018

View reviewed changes

Add check for target attribute.

21c4a17

chandlerprall approved these changes May 16, 2018

View reviewed changes

cjcenizal merged commit 1b17e33 into elastic:master May 16, 2018

cjcenizal deleted the react-router branch May 16, 2018 19:40


		### Hot module reloading

		Note that if using HMR, you'll need to re-register the router after a hot reload.


		### Share `router` globally

		To enable these techniques, you'll need to make the `router` instance available outside of React's

Add guidance on how to use EUI with react-router. #810

Add guidance on how to use EUI with react-router. #810

Conversation

cjcenizal commented May 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal May 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal May 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bevacqua left a comment

Choose a reason for hiding this comment

cjcenizal commented May 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pugnascotia commented May 10, 2018

mattapperson commented May 10, 2018 • edited Loading

cjcenizal commented May 10, 2018 • edited Loading

mattapperson commented May 10, 2018

cjcenizal commented May 10, 2018

chandlerprall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chandlerprall commented May 10, 2018

cjcenizal May 9, 2018 •

edited

Loading

cjcenizal May 9, 2018 •

edited

Loading

mattapperson commented May 10, 2018 •

edited

Loading

cjcenizal commented May 10, 2018 •

edited

Loading