Extracted LinkMixin from Link

[mtscout6]

The goal here is to be able to allow for a react-bootstrap NavItem to utilize the power of Link. Virtually removing all the duplicated code in react-router-bootstrap in particular for the NavItemLink component.

[mtscout6]

I'm open to writing more tests for this, but I wanted to get feedback before going too much further.

[mjackson]

I don't understand why this is necessary. Why can't you just render a <Link>?

[mtscout6]

My goal here is to use the react-bootstap library to handle the orchestration of all our bootstrap classes onto DOM elements. They already have a NavItem component, but the ability to change the anchor tag that's rendered does not feel right. It muddles that code base more than anyone really cares to allow. (See: react-bootstrap/react-bootstrap#187). The existing Link component has some great logic around what do do when an achor tag is clicked, but using the Link component is not always easy to use. By extracting some of the non-render logic out to the mixin, it allows for other components to leverage that logic and render something else.

[mtscout6]

Note that the react-bootstrap guys are already applying classes in a nice way: https://github.com/react-bootstrap/react-bootstrap/blob/master/src/NavItem.jsx

And you guys are doing awesome stuff with Link that makes the handleClick function really nice: https://github.com/rackt/react-router/blob/master/modules/components/Link.js#L111-L118, which includes many of the supporting functions to make it work.

Now, I'd like to bring the two together as seamless as possible. When you guys make changes to how Link works I want my stuff to pick up that change. When the react-bootstrap guys make changes to their stuff I'd like to get those changes.

Hence my reasoning that extracting the non-view specific logic around link handling to a mixin will allow a simple and concise wrapper component that ties the two concepts together. My goal with my react-router-bootstrap package is to build up wrappers for each of the react-bootstrap components that do link type stuff.

Could this mixin reside in my repo, sure. But I would still have to copy any of your changes you may or may not make over time, as I've already done.

[mtscout6]

Hit the wrong button

[mtscout6]

I found a second use case for this, as I'm sure more will follow. When using the react-bootstrap Button component as an anchor, again I'll need to make a wrapper to coordinate both libraries and using this mixin would be nice.

[ryanflorence]

<Link className="btn"/> doesn't work? That ends up being <a class="btn"/> in the dom.

[mtscout6]

All that does is put a simple class on the anchor, but what if that button is nested within a Nav? I agree that does get a class on there, but then you are loosing all the nice work done by react-bootstrap. I feel like you are not even looking at the react-bootstrap source, or even considering how to make this interact well with other libraries. I'd like something like <Button to='home' bsStyle='primary' /> since all the other components we are working with are using the same paradigm from react-bootstrap. I don't want to re-write all the logic that the react-bootstrap guys have done, and I don't want to re-write all the logic you guys have done. I'm just trying to get the two to play well together. Your recommendation does not integrate the two.

If you look at the react-router-bootstrap ButtonLink You'll see how clean it becomes to integrate them together. It has all the benefits of the react-router and the logic for what to do with react-bootstrap's Button let alone all the other components they have written that work with anchor tags.

I have added this LinkMixin in my react-router-bootstrap repo, but it is not where I feel it belongs. It is entirely copied/extracted from your Link component. I feel that adding this will allow existing UI frameworks like react-bootstrap to easily integrate without becoming tightly coupled together.

[ryanflorence]

Just to help me understand, you could also be asking the react-bootstrap devs to make NavItemMixin instead of just NavItem, yeah?

I admit I am hesitant, when people start using it on divs and spans and buttons there will be accessibility issues that come back to us as though they are our fault.

[ryanflorence]

Seems to me that whenever react-bootstrap renders an anchor tag, they ought to have built their API in a way to allow you drop in <Link/> instead.

I don't mean to frustrate you, I just need some more convincing :\

[ryanflorence]

https://github.com/react-bootstrap/react-bootstrap/blob/master/src/Button.jsx#L46

Can't you do <Button componentClass={Link} to="about"/> ?

[pieterv]

Hey @rpflorence, the solution you suggested above is what was originally proposed, have a read of this comment i posted in response and see if you agree with its points: react-bootstrap/react-bootstrap#187 (comment)

[ryanflorence]

Again, to maybe help you understand why we're hesitant. When we change things in LinkMixin we have to consider a lot of use-cases. If we just keep Link, then we only have to support one. We have provided ActiveState, which is the only router specific bit of Link, everything else Link does is public API from the router and React.

Public API that brings flexibility brings with it liability.

[ryanflorence]

The mixin has a lot of code specifically for handling a click on an anchor tag.

If you use the mixin on a button element you will get undesirable behavior when the event isModified. Meaning, a modifier key is also being held down. For an anchor tag, you will get the link opened in a new window, for a button, you just get a cancelled event :(

Then we'll get requests to not prevent the actions if its a button v. an anchor. Then somebody will want to use it on an option element, which has all sorts of different issues.

I hope this makes sense and I'm not giving you the grumps. I also hope you understand the liability this kind of mixin brings. So ...

I would like to:

• promote makeHref to public API so you can use it and not feel bad.
• encourage you to use ActiveState and makeHref to do whatever you need to do

[mtscout6]

I think the problem is that by proposing the Link and ActiveState alone is not enough for extensibility. The ActiveState is fine for telling a boolean value only. By itself it is not too useful, since you can't drive actions with it. The Link component alone for action is imposing a strong opinion of low-level UI, which I don't believe to be react-router's concern, although it is great for a very basic use case. However, the logic to route with to, query, and xyzparam props is most definitely what react-router is great at. Being able to extract the props from various component types opens the doors for more complex UI components. I understand your concern about accessibility and screen readers. Although, this is a routing library, is that really the concern of the this framework? This is why I think that a Mixin that exposes these routing capabilities is incredibly powerful to UI libraries. Would it help if we diverged from the name LinkMixin say RoutableMixin, or RoutePropsMixin?

I think that asking for react-bootstrap to expose more extensibility is a much more difficult undertaking, and it would make its codebase far more complex than it needs to be. I know that the Button component has that componentClass prop, cause I put it in there. However, it is there because of a use case where an html form submit input should be rendered just like a Button, so their Input component defers to Button for that functionality. Doing that same thing everywhere that an anchor is used though gets exhausting. We'd also have to do a lot of transferPropsTo in places that just don't make sense to do so. That was the problem we were seeing with react-bootstrap/react-bootstrap#187. Which is why, we determined that a wrapper component was the cleanest way to address those concerns.

In the vain of talking about this, it makes me wonder if I should remove the ActiveState mixin from the LinkMixin, since they are two different concerns. Namely in that I should remove the getInitialState, componentWillReceiveProps, updateActiveState methods. And the handleClick method also probably shouldn't be in there. Then rename the mixin to RouteProps, or RouteTo.

I can see how the handleClick method would then feel wrong in that mixin. Especially, since most of it is mouse event filtering.

[ryanflorence]

Although, this is a routing library, [are UI controls] really the concern of the this framework?

Exactly. They are not.

We provide one view component for the majority use-case of moving around an app, and then

• ActiveState to know if a route is active
• makeHref for you to generate hrefs wherever you need them

I don't want a mixin that requires so much implementation hand-holding.

note: I hope you saw the message two above this one.

[mtscout6]

Then how do you feel about a mixin that just does the prop extraction? Not, the handle click and active state.

[mjackson]

Agree with @rpflorence on this. I'd rather expose the router-specific bits of functionality than start concerning ourselves with UI components (and/or mixins for them). ActiveState and makeHref should be all you need to create your own components that work well with the router.

[pieterv]

This seems like a good solution, i still think there is value in a LinkMixin to ease integration but it doesn't need to be (or rather shouldn't be) provided by this project.

[ryanflorence]

Yeah, as soon as we ship a LinkMixin we are in view library land. I'd rather expose what is needed, rather than provide view mixins.

[mtscout6]

I updated my integration library to have two mixins one is a RouteToMixin which is what I was thinking to trim down the LinkMixin. Anyone can take from that if they want.