[added] wraptag property for Link

[krawaller]

Since it is a very common use case to want the active class of a Link to be applied to the parent tag for Bootstrap compatibility, this PR adds a new optional wraptag prop to Link. If supplied, the link will be wrapped in the given tag, and the class names applied to the wrapper instead of the anchor tag.

That means I can now do this:

<Link to="foo" wraptag="li">Foo</Link>

instead of having to make my own wrapper component, looking something like this:

var Navlink = React.createClass({
  mixins: [ State ],
  render: function() {
    var p = this.props;
    var className = (p.linkClasses || '')+(this.isActive(p.to, p.params, p.query) ? ' active' : '');
    return <li className={className}>{Link(p)}</li>;
  }
});

[mtscout6]

Have you considered using react-router-bootstrap with react-bootstrap instead?

[gaearon]

My personal opinion is that Link should stay as simple as possible. As you said yourself, it's easy to create a wrapper component, which is the model encouraged by React. Rather than make existing components do many things, wrap them and add functionality using composition.

[mjackson]

@krawaller I agree with @gaearon. I'd like to keep <Link> as simple as possible and the wrapper + mixin approach is the one currently encouraged by React.

[krawaller]

Yeah, I guess you guys are right. It just feels a bit... inelegant to have Link do the active class work, only for me to do the same thing in a wrapping component to cover the bootstrap use case.

My reasoning was that since using a property is cognitively much cheaper than creating a component + importing a mixin + making the isActive call + adding the classes, the difference in cost warranted the API bloat.

But I might be overestimating how common the wrapping need actually is, and either way, you are probably right to demand an even higher price for increasing API complexity. Well done, you passed the test! ;)

[gaearon]

Personally I don't see adding a component in the consuming app as a cost. I wrap most of the components from other libraries, whether it's a Link, a DocumentTitle or YoutubePlayer, because more often than not I need some customization.

This also gives you greater flexibility when changing tools. For example, I had a custom Link before migrating to React Router, when I still used Backbone. I also had UserLink, ArticleLink and other app-specific links that accepted my models. When I moved from Backbone to React Router, I didn't need to modify the consuming components that used UserLink or ArticleLink—only these helper components themselves.

[krawaller]

I meant it as a cognitive cost, i.e. how hard it is to figure out. But that was a really silly reasoning to begin with, as the ability to create components is definitely something you can assume in a user of this library. :P

[MarkMurphy]

What about allowing the link component itself to be customized like Ember does it:

<Link component="li" to="..." />

This would allow us to do whatever we want.

[taion]

This is deliberately a WONTFIX here, because those are going to have very poor a11y. If you need compatibility with e.g. React-Bootstrap, use React-Router-Bootstrap.

[MarkMurphy]

ok, sure. I'm not looking for compatibility with React-Bootstrap but I do need the same functionality as krawaller was asking for.

It seems like I have to create my own wrapper component using a mixin which is fine however, that's a tough thing for a new comer to wrap their head around. Especially without knowing there's a state mixin which can help with that.

I think adding an example to the repo showing how to accomplish this would help a lot of people, myself included.

[taion]

You can quite possibly just use the React-Bootstrap wrapper. It's fairly generic.

[MarkMurphy]

You can quite possibly just use the React-Bootstrap wrapper. It's fairly generic.

I'm not interested in using that library, I just want to keep things simple. After browsing through the react-router code base it appears that the State mixin isn't a thing anymore.

Anyone willing to provide a gist on how to create a simple wrapper for Link which applies an active class as discussed in this thread? I'm using ES2015 (ES6)

[taion]

React-Router-Bootstrap is literally two wrapper components and has no dependency on React-Bootstrap at all.

You can reinvent as much or as little as you'd like. I'm not interested in continuing this conversation.

[MarkMurphy]

@taion Thanks for the help. Maybe you didn't realize that your second last comment referred me to react-bootstrap, not react-router-bootstrap. I missed that in one of the initial comments as well.

No need to respond to this one...