Skip to content

[next] active links: allow trailing slash in route matching (nested routes) #628

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.

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

Closed
jwahdatehagh opened this issue Aug 31, 2016 · 18 comments
Assignees
Labels

Comments

@jwahdatehagh
Copy link

I would like to have nested routes with a default component in the nested-root like so:

routes: [
  { 
    path: '/home', 
    name: 'home',
    component: Home,
    children: [
      {
        path: '',
        name: 'home.index',
        component: HomeIndex
      },
      {
        path: 'foo',
        name: 'home.foo',
        component: HomeFoo
      }
    ]
  }
]

For the active link highlights i'd like to scope the children separately. Can we serve the same content on both /home and /home/ and have the /home link active in both cases?

Vue.js & vue-router.js version

2.0.0-rc.4, 2.0.0-rc.4

Reproduction Link

https://jsfiddle.net/jwahdatehagh/44ed5xxf/1/

@LinusBorg
Copy link
Member

LinusBorg commented Aug 31, 2016

I think this should be working as you describe. Currently it doesnt' because with do a simple === match of the path strings here:

https://github.com/vuejs/vue-router/blob/next/src/util/route.js#L8

We could maybe solve this with:

return (
      a.path.replace(/\/$/,'') === b.path.replace(/\/$/,'') &&
      a.hash === b.hash &&
      isObjectEqual(a.query, b.query)
    )

@fnlctrl What are your thoughts?

@LinusBorg LinusBorg added the bug label Aug 31, 2016
@jwahdatehagh
Copy link
Author

Does the same problem not apply to a.hash === b.hash? Sorry, i'm not quite acquainted with the internals of the router.

@LinusBorg
Copy link
Member

LinusBorg commented Aug 31, 2016

In what way? hashes don't have this issue with optional trailing slashes. hashes come after the path and don't have any slashes:

http://domain.tld/some/path/#hash?query=params

@jwahdatehagh
Copy link
Author

jwahdatehagh commented Aug 31, 2016

I thought this check is for when the router uses the hash vs the history api e.g. domain.tld#/home/foo.

Then the same issue would occur for #/home vs #/home/.

@LinusBorg
Copy link
Member

LinusBorg commented Aug 31, 2016

The hash is removed before route matching:

https://github.com/vuejs/vue-router/blob/next/src/history/hash.js#L44

(the getHash() function does it...)

So when the code reaches the section discussed previously, any hash present is a "real" hash, not a "fake path" hash.

@jwahdatehagh
Copy link
Author

Ok, cool! Thanks for the explanation!

@fnlctrl
Copy link
Member

fnlctrl commented Sep 1, 2016

@LinusBorg Though unlikely, a.path.replace(/\/$/,'') === b.path.replace(/\/$/,'') will be true for '/foo/bar' and '/foobar'.

I don't quite get what OP wants... What's wrong with adding exact on roter-link?

@fnlctrl fnlctrl closed this as completed Sep 1, 2016
@fnlctrl fnlctrl reopened this Sep 1, 2016
@LinusBorg
Copy link
Member

LinusBorg commented Sep 1, 2016

@fnlctrl

  1. my Regex will only remove trailing slashes, so your example should not be possible.
  2. The problem is that a router-link with a path of /home and the exact attribute will not get the activeClass set if the current route's path is /home/ - even though both routes render the same components and should therefore be considered equal in my opinion.

@fnlctrl
Copy link
Member

fnlctrl commented Sep 1, 2016

@LinusBorg Oops, misread that regex...sorry.
Thanks for the explanation! I'll look into it.

@fnlctrl
Copy link
Member

fnlctrl commented Sep 1, 2016

@LinusBorg I agree with removing trailing slashes before comparison.

@jwahdatehagh
Copy link
Author

I don't quite get what OP wants... What's wrong with adding exact on roter-link?

@fnlctrl i want my <router-link to="/home" exact>/home</router-link> to be active for both /home and /home/ which doesn't work atm. This would be solved by @LinusBorg's proposal...

@fnlctrl
Copy link
Member

fnlctrl commented Sep 1, 2016

@jwahdatehagh Understood. I'll submit a PR shortly.

@fnlctrl
Copy link
Member

fnlctrl commented Sep 1, 2016

@LinusBorg @jwahdatehagh
The PR is submitted: https://github.com/vuejs/vue-router/pull/632/files

Had some troubles running flow on windows, and found that vue-router uses 0.29 while 0.31 is required for windows support. Tried running tests on 0.31 and everything passed, should probably submit a PR to update flow later. Totally forgot that I was on next-doc branch, which wasn't updated. Though it appears tests didn't break.

@LinusBorg
Copy link
Member

We should add a test for this case as well.

@fnlctrl
Copy link
Member

fnlctrl commented Sep 1, 2016

Yep.. adding one now.

@jwahdatehagh
Copy link
Author

Nice, thank you so much!

yyx990803 pushed a commit that referenced this issue Sep 8, 2016
* Allow trailing slashes in path comparison (fix #628)

* Modify existing test for isSameRoute
@fnlctrl
Copy link
Member

fnlctrl commented Sep 9, 2016

@jwahdatehagh
The PR's merged, should land in the next rc soon, so I'm closing this. 😄

@fnlctrl fnlctrl closed this as completed Sep 9, 2016
@jwahdatehagh
Copy link
Author

Thanks a log @fnlctrl

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants