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.

Sign up for GitHub

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

Add support for function to #3669

Merged
merged 1 commit into from
Jul 26, 2016
Merged

Add support for function to #3669

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 13 additions & 6 deletions modules/Link.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ function isEmptyObject(object) {
return true
}

function resolveToLocation(to, router) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would make this a class method so you don't have to pass it any args Bonus points: You can make it a getter (router.push(this.to)).

Also, and this is personal preference, but I'm generally not a fan of extracting methods from a class unless they are also being exported from the module.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't a class method because we need to strip out to from props in render anyway – we can't (shouldn't) pass it down because otherwise we get React 15.2.x "unknown props" warnings.

The other options would be to either destructure it out and leave it unused, or do an explicit delete, both of which are uglier than just using it as a function arg.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, we're still using createClass, so that's not even possible. I meant something like this:

get to() {
  const { to } = this.props
  return typeof to === 'function' ? to(this.context.router.location) : to
}

handleClick(event) {
  //...
  router.push(this.to)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this just ends up being the cleanest way to strip off the to prop before it gets to the underlying <a>.

return typeof to === 'function' ? to(router.location) : to
}

/**
* A <Link> is used to create an <a> element that links to a route.
* When that route is active, the link gets the value of its
Expand Down Expand Up @@ -49,7 +53,7 @@ const Link = React.createClass({
},

propTypes: {
to: oneOfType([ string, object ]).isRequired,
to: oneOfType([ string, object, func ]).isRequired,
query: object,
hash: string,
state: object,
Expand All @@ -74,8 +78,9 @@ const Link = React.createClass({
if (event.defaultPrevented)
return

const { router } = this.context
invariant(
this.context.router,
router,
'<Link>s rendered outside of a router context cannot navigate.'
)

Expand All @@ -89,19 +94,21 @@ const Link = React.createClass({

event.preventDefault()

this.context.router.push(this.props.to)
router.push(resolveToLocation(this.props.to, router))
},

render() {
const { to, activeClassName, activeStyle, onlyActiveOnIndex, ...props } = this.props
// Ignore if rendered outside the context of router, simplifies unit testing.

// Ignore if rendered outside the context of router to simplify unit testing.
const { router } = this.context

if (router) {
props.href = router.createHref(to)
const toLocation = resolveToLocation(to, router)
props.href = router.createHref(toLocation)

if (activeClassName || (activeStyle != null && !isEmptyObject(activeStyle))) {
if (router.isActive(to, onlyActiveOnIndex)) {
if (router.isActive(toLocation, onlyActiveOnIndex)) {
if (activeClassName) {
if (props.className) {
props.className += ` ${activeClassName}`
Expand Down
43 changes: 43 additions & 0 deletions modules/__tests__/Link-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,4 +473,47 @@ describe('A <Link>', () => {
})
})

describe('with function to', () => {
const LinkWrapper = () => (
<Link
to={location => ({ ...location, hash: '#hash' })}
activeClassName="active"
>
Link
</Link>
)

it('should have the correct href and active state', () => {
render((
<Router history={createHistory('/hello')}>
<Route path="/hello" component={LinkWrapper} />
</Router>
), node, () => {
const a = node.querySelector('a')
expect(a.getAttribute('href')).toEqual('/hello#hash')
expect(a.className.trim()).toEqual('active')
})
})

it('should transition correctly on click', done => {
const steps = [
() => {
click(node.querySelector('a'), { button: 0 })
},
({ location }) => {
expect(location.pathname).toEqual('/hello')
expect(location.hash).toEqual('#hash')
}
]

const execNextStep = execSteps(steps, done)

render((
<Router history={createHistory('/hello')} onUpdate={execNextStep}>
<Route path="/hello" component={LinkWrapper} />
</Router>
), node, execSteps)
})
})

})