From cb31040d5f889c5b193fee6fcf5e14ada2893f80 Mon Sep 17 00:00:00 2001 From: Muhtasim Ahsan Date: Fri, 30 Oct 2015 14:46:00 -0400 Subject: [PATCH 1/3] Add test for index links on dynamic routes --- modules/__tests__/DynamicRoute-test.js | 100 +++++++++++++++++++++++++ 1 file changed, 100 insertions(+) create mode 100644 modules/__tests__/DynamicRoute-test.js diff --git a/modules/__tests__/DynamicRoute-test.js b/modules/__tests__/DynamicRoute-test.js new file mode 100644 index 0000000000..9db0217c2f --- /dev/null +++ b/modules/__tests__/DynamicRoute-test.js @@ -0,0 +1,100 @@ +/*eslint-env mocha */ +import expect from 'expect' +import React, { Component } from 'react' +import { render, unmountComponentAtNode } from 'react-dom' +import createHistory from 'history/lib/createMemoryHistory' +import IndexLink from '../IndexLink' +import execSteps from './execSteps' +import Router from '../Router' +import Link from '../Link' + +describe('Dynamic Routes', function () { + + class App extends Component { + render() { + return ( +
+ + {this.props.children} +
+ ) + } + } + + class Website extends Component { + render() { + return
website wrapper {this.props.children}
+ } + } + + class ContactPage extends Component { + render() { + return
contact page
+ } + } + + class IndexPage extends Component { + render() { + return
index page
+ } + } + + const routes = { + childRoutes: [ { + path: '/', + component: App, + childRoutes: [ + { + path: 'website', + component: Website, + childRoutes: [ + { path: 'contact', component: ContactPage } + ], + getIndexRoute(location, callback) { + setTimeout(function () { + callback(null, { component: IndexPage } ) + }) + } + } + ] + } ] + } + + let node + beforeEach(function () { + node = document.createElement('div') + }) + + afterEach(function () { + unmountComponentAtNode(node) + }) + + describe('when linking to an index link', function () { + it('is active and non-index routes are not', function (done) { + let overviewLink, contactLink + const steps = [ + function () { + overviewLink = node.querySelector('#overviewLink') + contactLink = node.querySelector('#contactLink') + expect(overviewLink.className).toEqual('') + expect(contactLink.className).toEqual('active') + this.history.pushState(null, '/website') + }, + function () { + expect(overviewLink.className).toEqual('active') + expect(contactLink.className).toEqual('') + } + ] + + const execNextStep = execSteps(steps, done) + + render(( + + ), node, execNextStep) + }) + }) + +}) From 8dd8cebb50c008d2f7ed950714c1a0bd446697e3 Mon Sep 17 00:00:00 2001 From: Jimmy Jia Date: Fri, 30 Oct 2015 15:16:29 -0400 Subject: [PATCH 2/3] [fixed] Mark dynamic index routes as active --- modules/__tests__/DynamicRoute-test.js | 100 ------------------------- modules/__tests__/isActive-test.js | 41 ++++++++++ modules/isActive.js | 23 +++++- 3 files changed, 61 insertions(+), 103 deletions(-) delete mode 100644 modules/__tests__/DynamicRoute-test.js diff --git a/modules/__tests__/DynamicRoute-test.js b/modules/__tests__/DynamicRoute-test.js deleted file mode 100644 index 9db0217c2f..0000000000 --- a/modules/__tests__/DynamicRoute-test.js +++ /dev/null @@ -1,100 +0,0 @@ -/*eslint-env mocha */ -import expect from 'expect' -import React, { Component } from 'react' -import { render, unmountComponentAtNode } from 'react-dom' -import createHistory from 'history/lib/createMemoryHistory' -import IndexLink from '../IndexLink' -import execSteps from './execSteps' -import Router from '../Router' -import Link from '../Link' - -describe('Dynamic Routes', function () { - - class App extends Component { - render() { - return ( -
-
    -
  • overview
    • -
  • contact
    • -
- {this.props.children} -
- ) - } - } - - class Website extends Component { - render() { - return
website wrapper {this.props.children}
- } - } - - class ContactPage extends Component { - render() { - return
contact page
- } - } - - class IndexPage extends Component { - render() { - return
index page
- } - } - - const routes = { - childRoutes: [ { - path: '/', - component: App, - childRoutes: [ - { - path: 'website', - component: Website, - childRoutes: [ - { path: 'contact', component: ContactPage } - ], - getIndexRoute(location, callback) { - setTimeout(function () { - callback(null, { component: IndexPage } ) - }) - } - } - ] - } ] - } - - let node - beforeEach(function () { - node = document.createElement('div') - }) - - afterEach(function () { - unmountComponentAtNode(node) - }) - - describe('when linking to an index link', function () { - it('is active and non-index routes are not', function (done) { - let overviewLink, contactLink - const steps = [ - function () { - overviewLink = node.querySelector('#overviewLink') - contactLink = node.querySelector('#contactLink') - expect(overviewLink.className).toEqual('') - expect(contactLink.className).toEqual('active') - this.history.pushState(null, '/website') - }, - function () { - expect(overviewLink.className).toEqual('active') - expect(contactLink.className).toEqual('') - } - ] - - const execNextStep = execSteps(steps, done) - - render(( - - ), node, execNextStep) - }) - }) - -}) diff --git a/modules/__tests__/isActive-test.js b/modules/__tests__/isActive-test.js index db85f2b974..e3675a42a6 100644 --- a/modules/__tests__/isActive-test.js +++ b/modules/__tests__/isActive-test.js @@ -297,4 +297,45 @@ describe('isActive', function () { }) }) }) + + describe('dynamic routes', function () { + const routes = { + path: '/', + childRoutes: [ + { path: 'foo' } + ], + getIndexRoute(location, callback) { + setTimeout(() => callback(null, {})) + } + } + + describe('when not on index route', function () { + it('does not show index as active', function (done) { + render(( + + ), node, function () { + expect(this.history.isActive('/')).toBe(true) + expect(this.history.isActive('/', null, true)).toBe(false) + expect(this.history.isActive('/foo')).toBe(true) + done() + }) + }) + }) + + describe('when on index route', function () { + it('shows index as active', function (done) { + render(( + + ), node, function () { + // Need to wait for async match to complete. + setTimeout(() => { + expect(this.history.isActive('/')).toBe(true) + expect(this.history.isActive('/', null, true)).toBe(true) + expect(this.history.isActive('/foo')).toBe(false) + done() + }) + }) + }) + }) + }) }) diff --git a/modules/isActive.js b/modules/isActive.js index 35f85536f2..4d572d099c 100644 --- a/modules/isActive.js +++ b/modules/isActive.js @@ -69,11 +69,28 @@ function getMatchingRoute(pathname, activeRoutes, activeParams) { function routeIsActive(pathname, activeRoutes, activeParams, indexOnly) { let route = getMatchingRoute(pathname, activeRoutes, activeParams) - if (route == null) + if (route == null) { return false + } + + if (indexOnly) { + if (activeRoutes.length < 2) { + return false + } - if (indexOnly) - return activeRoutes.length > 1 && activeRoutes[activeRoutes.length - 1] === route.indexRoute + const lastRoute = activeRoutes[activeRoutes.length - 1] + if (route.indexRoute) { + return lastRoute === route.indexRoute + } + + // TODO: Should we also return true if lastRoute is route? + + // In case the index route was configured via getIndexRoute. + return ( + activeRoutes[activeRoutes.length - 2] === route && + !lastRoute.path // Pathless route must have been the index route. + ) + } return true } From 31b1a79eaec6d32e4977a919e5847dff55c493fd Mon Sep 17 00:00:00 2001 From: Jimmy Jia Date: Fri, 30 Oct 2015 16:38:32 -0400 Subject: [PATCH 3/3] [SQUASH] Actually, it's a lot easier than that to match index route --- modules/isActive.js | 29 ++++------------------------- 1 file changed, 4 insertions(+), 25 deletions(-) diff --git a/modules/isActive.js b/modules/isActive.js index 4d572d099c..9788689129 100644 --- a/modules/isActive.js +++ b/modules/isActive.js @@ -66,33 +66,12 @@ function getMatchingRoute(pathname, activeRoutes, activeParams) { * Returns true if the given pathname matches the active routes * and params. */ -function routeIsActive(pathname, activeRoutes, activeParams, indexOnly) { - let route = getMatchingRoute(pathname, activeRoutes, activeParams) - - if (route == null) { - return false - } - +function routeIsActive(pathname, location, routes, params, indexOnly) { if (indexOnly) { - if (activeRoutes.length < 2) { - return false - } - - const lastRoute = activeRoutes[activeRoutes.length - 1] - if (route.indexRoute) { - return lastRoute === route.indexRoute - } - - // TODO: Should we also return true if lastRoute is route? - - // In case the index route was configured via getIndexRoute. - return ( - activeRoutes[activeRoutes.length - 2] === route && - !lastRoute.path // Pathless route must have been the index route. - ) + return location.pathname.replace(/\/*$/) === pathname.replace(/\/*$/) } - return true + return getMatchingRoute(pathname, routes, params) != null } /** @@ -117,7 +96,7 @@ function isActive(pathname, query, indexOnly, location, routes, params) { if (location == null) return false - if (!routeIsActive(pathname, routes, params, indexOnly)) + if (!routeIsActive(pathname, location, routes, params, indexOnly)) return false return queryIsActive(query, location.query)