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

Optional Link component wrap #137

Closed
dhenson02 opened this issue Aug 25, 2018 · 9 comments
Closed

Optional Link component wrap #137

dhenson02 opened this issue Aug 25, 2018 · 9 comments
Labels
enhancement New feature or request

Comments

@dhenson02
Copy link

dhenson02 commented Aug 25, 2018

It would be useful to support componentWrap and getWrapProps props on a <Link />.

When passed, they would be supplied with a React component (that wraps the returned <a>) and function (same usage as getProps, spread over the wrapper props instead of <a>), respectively.

Example:

const Wrapper = ({ children, ...props }) => (
  <li {...props}>
    {children}
  </li>
);

const getWrapProps = ({ isCurrent }) => ({ 
  'className': isCurrent 
    ? 'is-active' 
    : '' 
});

// ...

<Link
  to="/abc"
  componentWrap={Wrapper}
  getWrapProps={getWrapProps}>
  Go somewhere
</Link>

I've already linted/tested/committed this to my own fork, but given the outcome of #63 I'll hold off on PR unless requested.

@elifitch
Copy link

elifitch commented Sep 4, 2018

Sounds like this would help a use case I have! I'd also like to be able to style a component that wraps my Link depending on whether that link is active. Here's a little image to illustrate.

use-case

At present I'm calling setState from within my Link's getProps function, which leads to ye olde Cannot update during an existing state transition warning.

@criscola
Copy link

criscola commented Oct 4, 2018

Can we have a follow up on this please?

@elifitch
Copy link

elifitch commented Oct 4, 2018

I was able to accomplish the effect I wanted with existing tools in reach router, and a render prop that allows you to return a link element with whatever arbitrary wrapper you choose. This example is a little crude, but hopefully is enough to get the gist of what's going on and implement something that's nearer and dearer to y'all's specific use case.

// @flow
import React from 'react';
import { Location, Link } from '@reach/router';
import { BASE_PATH } from '../../constants/router_constants';

type Props = {
  to: string,
  children: React$Node,
  className?: string,
  render?: ({ isCurrent: boolean, Link: React$Node }) => React$Node
};

class WrappedLink extends React.Component<Props> {
  renderLink = ({ location }: { location: Object }): React$Node => {
    const { to, render, children, className } = this.props;

    const isCurrent = location.pathname === `/${to}`;

    if (render) {
      return render({
        isCurrent,
        Link: React.createElement(Link, { className, to }, children)
      });
    }

    return (
      <Link className={className} to={linkTarget}>
        {children}
      </Link>
    );
  };

  render() {
    return <Location>{this.renderLink}</Location>;
  }
}

export { WrappedLink };

/* Example use */

<WrappedLink
  to="my-url"
  className="my-style"
  render={({isCurrent, Link}) => {
    const wrapperStyle = isCurrent ? 'current' : 'not-current';
    return (
      <div className={wrapperStyle}>
        {Link}
      </div>
    )
  }}
/>

If y'all feel like this accomplishes what you want, I'd personally be cool seeing this issue closed because we get what we need and reach router doesn't have to accept any additional complexity.

cc @criscola @dhenson02

@dhenson02
Copy link
Author

@elifitch what you have here would basically override the built-in functionality that decides the isCurrent value. The idea behind my patch was to add the ability to wrap an element around the link but allow it to behave as it currently does, supporting any future changes to props that may be passed to the <Link /> component.

But as I said before, the previous/similar issue #63 resulted in a rejection so if @ryanflorence feels this is adding too much complexity and/or going against the original goal of this project, he will certainly reject this as well. I just wanted to offer it up since I had to do this for my project already and someone else may find it useful.

@MunifTanjim
Copy link

MunifTanjim commented Nov 30, 2018

This is what I came up with, works for my usecase:

const WrapperLi = ({ to, children }) => (
  <Match path={`${to}/*`}>
    {props => (
      <li className={props.match ? 'is-active' : ''}>
        <Link to={to}>{children}</Link>
      </li>
    )}
  </Match>
)

<WrapperLi to="hello">Hello</TabItem>

Edit: actually this is general enough to work for any usecases

@meandillar
Copy link

meandillar commented Jul 11, 2019

I'm using Rebass which relies on passing style props to the component. The solution from @MunifTanjim is working well for me so far. Would love to see a @dhenson02 kind of implementation

@joshuaalpuerto
Copy link

@MunifTanjim you're a lifesaver man!

@jsphstls
Copy link

jsphstls commented Sep 3, 2019

Similar solution. I use the aria-current attribute of the <Link /> to style the children and the <LinkWrapper/> does not import Link thanks to a render prop.

const LinkWrapper = ({
  children: renderChildrenProp,
  label
}) =>
  <span className='someClass'>
    {
      renderChildrenProp( // Renders an element that will receive the following elements as children
        <>
          <SomeIcon />
          {label}
        </>
      )
    }
  </span>    

// Example use
<LinkWrapper label='Some Label'>
  {
    contents =>
      <a href='#somewhere'>
        {contents}
      </a>
  }
</LinkWrapper>

// Output
<span className='someClass'>
  <a href='somewhere'>
    <SomeIcon />
    Some Label
  </a>
<span>
      
// Example CSS
.someClass {
  [aria-current] {
    color: red;
  }
}

@mtliendo mtliendo added the enhancement New feature or request label Sep 12, 2019
@mtliendo
Copy link

Great discussion and just to summarize a bit, Ryan mentioned in #63 some concerns in adding some additional features in but @MunifTanjim snippet would be the "reach router way" of accomplishing this task. Gonna go ahead and close but keep an eye on the future of reach router for everything in store!

Thanks for using reach router!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

8 participants