Add NavLink option to use another element type for active state #69
Conversation
| <NavLink activeElement="span" routeName='foo' /> | ||
| </MockAppComponent> | ||
| ); | ||
| expect(link.getDOMNode().nodeName.toLowerCase()).to.equal('span'); |
There was a problem hiding this comment.
getDOMNode is deprecated
https://facebook.github.io/react/docs/top-level-api.html#react.finddomnode
There was a problem hiding this comment.
That would apply to all tests, not my pull request in particular. I like to be consistent. :)
|
@ebednarz I haven't heard of this use case before. Can you explain why it's bad UI to have a link for the current page? A lot of times I see this as a way of refreshing the current page. |
|
CLA is valid! |
|
@mridgway It often leads to errors and superfluous network requests. Inability to unlink the current resource is simply considered a technical shortcoming where I come from, not a design choice. |
|
@mridgway Any new insights on this topic? If there's anything I can do, I'd be happy to. |
|
@mridgway FYI, I merged master into the pull request. (I also created a fork for npm installation for the time being.) |
Add NavLink option to use another element type for active state
I was a bit disappointed that the NavLink component active state produces a href attribute with a same page reference. Besides potential unexpected activation side effects, that's fairly bad UX, especially when using the keyboard (it's possible to fix the wrong problem with "pointer-events: none" for pointing devices, but I don't see a way to do that to the tab index).
Would you be interested in adding an optional activeElement property? It might be preferable to remove the "href" attribute, but unfortunately that's not currently possible with React.
I tried my best to make the code change simple, not smart. :-)