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

Create HOC for creating react-router-compatible links #507

Closed
cjcenizal opened this issue Mar 13, 2018 · 6 comments
Closed

Create HOC for creating react-router-compatible links #507

cjcenizal opened this issue Mar 13, 2018 · 6 comments
Assignees

Comments

@cjcenizal
Copy link
Contributor

cjcenizal commented Mar 13, 2018

React-router proscribes the use of its Link component for creating links which are compatible with its routing API. This causes problems for consumers of EUI who use react-router. We can address this by publishing a react-router-specific HOC which acts as an adapter between react-router's routing API and our own components.

We just need to be able to translate the Link to prop into onClick and href props, which are then passed to the wrapped component. We can refer to the Link implementation for this.

Note that this solution also requires that all components which support onClick will also need to support href, and vice versa, on the same element.

Here's how I think the API would look:

// Render a react-router-compatible button
render() {
  return rrAdapter(EuiButton, { to: '/courses?sort=name' });
}

CC @zinckiwi @mattapperson @snide

@mattapperson
Copy link
Contributor

mattapperson commented Mar 16, 2018

Might I recommend this be done as a component? So

render() {
  return <RRCompat component={EuiButton}  to={ '/courses?sort=name' } />;
}

Might just be cleaner in apps using it :)

@snide
Copy link
Contributor

snide commented Apr 30, 2018

@pickypg was mentioning a need for this today as well.

@snide snide mentioned this issue Apr 30, 2018
44 tasks
@cjcenizal cjcenizal self-assigned this May 3, 2018
@cjcenizal
Copy link
Contributor Author

cjcenizal commented May 9, 2018

Just spitballing a few other ideas for the interface.

// This util converts an idiomatic `to` string into a { href, onClick } object.

<EuiLink {...toHrefAndOnClick('/location')}>Link</EuiLink>
// This component would use cloneElement to apply the href an onClick props to the
// EuiLink element, but this would screw up propType checking for those props.
// EuiLink will also create a `button` if only onClick is provided, and this kind of
// interface doesn't accommodate that type of behavior.

<RrAdapter to="/location">
  <EuiLink>Link</EuiLink>
</RrAdapter>
// Like Matt's suggestion above, this component would use createElement to create
// the EuiLink with the correct props, but it's a bit of an unusual interface. All other
// props but `component` and `to` would be passed down to the instantiated EuiLink
// component. Same problem as above re creating a `button`, too.

<RrAdapter to="/location" component={EuiLink} color="danger" target="_blank" />;

All of these solutions except for the first one also won't be compatible with a component which consumes EuiLink, EuiButton, etc internally.

// Here's an idea using a render prop.

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

@chandlerprall
Copy link
Contributor

I'm in favor of either an toHrefAndOnClick function (CJ's first example) or the render prop approach (fourth example), whichever feels better / less cumbersome to the people using it.

@snide
Copy link
Contributor

snide commented May 9, 2018

From the design side of things the only concern @cchaos and I would have is whether the solution creates a wrapping element. Specifically things get tricky when we use flex level items in groups where they need to grow.

A primary example is EuiCard within a FlexGrid. The card itself has a bunch of styling concerns that would be broken if a wrapper element was added (to get in the way of the flex). We'd prefer if it was smart enough to apply an onClick to the card itself (if possible).

Changing the card to be more flexible with what it accepts for linking is totally an option too, I just thought I'd bring it up.

image

@chandlerprall
Copy link
Contributor

Good concern; none of CJ's proposals would add any additional elements to the DOM, wrapping or otherwise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants