Expose child routes on currentRoute object; add property for a CSS class on inactive routes #1650

tmcdos · 2017-07-27T16:28:24Z

It is not easy to get access to child routes when you need to render nested menus with <router-link>
Publishing children on the $route.currentRoute solves a real problem. The parent property is required so that the route can see its siblings. The title property is optional - it is useful instead of name when you want to name a route having a default child. I personally use name in beforeRouteEnter/beforeRouteUpdate to update the document.title in browser - and I need it on all routes (even with default child), so I had to add title property.

This closes issue #1149

There are non-trivial situations where playing with CSS selectors becomes too inconvenient. Consider the following markup:

    <div>
      <div class="tabs_account">
        <div class="tab_spacer">&nbsp;</div>
        <template v-for="page in $router.currentRoute.parent.children" v-if="page.can_show">
          <router-link tag="div" active-class="tab_active" inactive-class="tab" v-bind:to="page.path">{{ page.title }}</router-link>
          <div class="tab_spacer">&nbsp;</div>
        </template>
        <div class="tab_fill tab_spacer">&nbsp;</div>
      </div>
      <router-view></router-view>
    </div>

You can achieve the same effect like this

<router-link :class="{'router-link-active': $route.fullPath ==='/' || $route.fullPath === '/a', 'router-link-inactive': $route.fullPath !=='/' && $route.fullPath !== '/a'}" to="/a">
Text
</router-link>

but it looks hacky (you have to keep the PATH strings in sync between HTML and JavaScript code) while the proposed solution feels more elegant.

…s (linkInactiveClass, linkExactInactiveClass) There are non-trivial situations where playing with CSS selectors becomes too inconvenient. Consider the following markup: <div> <div class="tabs_account"> <div class="tab_spacer"> </div> <template v-for="page in $router.currentRoute.parent.children" v-if="page.can_show"> <router-link tag="div" active-class="tab_active" inactive-class="tab" v-bind:to="page.path">{{ page.title }}</router-link> <div class="tab_spacer"> </div> </template> <div class="tab_fill tab_spacer"> </div> </div> <router-view></router-view> </div> You can achieve the same effect like this <router-link :class="{'router-link-active': $route.fullPath ==='/' || $route.fullPath === '/a'}" to="/a"></router-link> but it looks hacky (you have to keep the PATH in HTML in sync with the JavaScript code) while my proposed solution feels more elegant.

…, parent, title) It is not easy to get access to child routes when you need to render nested menus with <router-link> Publishing "children" on the $route.currentRoute solves a real problem. The "parent" property is required so that the route can see its siblings. The "title" property is optional - it is useful instead of "name" when you want to name a route having a default child. I personally use "name" in beforeRouteEnter/beforeRouteUpdate to update the "document.title" in browser - and I need it on all routes (even with default child), so I had to add "title" property. This closes issue #1149

posva · 2017-07-27T17:47:58Z

Hey thanks for the PR 🙂
If you want to add the feature, please, only add that feature, so don't add the title feature which is a personal thing. Also, please, don't add the // TMCDOS at the end of every line you added
Remember to add a test case for children

…rty for CSS class on inactive router links The $router.currentRoute object now exposes "children[]" and "parent" properties. The <router-link> component has now properties "inactive-class" and "exact-inactive-class" for specifying CSS class on inactive routes. Solves issue #1149

tmcdos · 2017-07-28T14:44:21Z

Okay, I followed your recommendations:

removed the // TMCDOS marks
removed the custom title property
added a unit test for parent and children[] properties
added a Selenium test for inactive-class and exact-inactive-class
fixed some settings to allow headless browser testing with ChromeDriver

Hopefully the PR is now acceptable.

posva

I thought you were going to remove the other extra feature as well 😅 . Can you remove the inactive class feature from this PR, please?
Also, could you add the typings to the flow/declarations.js file, please?

posva · 2017-07-31T09:27:04Z

package.json

@@ -28,7 +28,7 @@
    "lint": "eslint src examples",
    "test": "npm run lint && flow check && npm run test:unit && npm run test:e2e && npm run test:types",
    "test:unit": "jasmine JASMINE_CONFIG_PATH=test/unit/jasmine.json",
-    "test:e2e": "node test/e2e/runner.js",
+    "test:e2e": "xvfb-run node test/e2e/runner.js && rm -rf ./xvfb-run.* && rm -rf ./.com.google.* && rm -rf ./.org.chromium.*",


Please let it as it was

posva · 2017-07-31T09:28:46Z

test/e2e/nightwatch.config.js

@@ -33,7 +33,10 @@ module.exports = {
      'desiredCapabilities': {
        'browserName': 'chrome',
        'javascriptEnabled': true,
-        'acceptSslCerts': true
+        'acceptSslCerts': true,


Can you remove this as well please

posva · 2017-07-31T19:53:26Z

test/unit/specs/children.spec.js

+Vue.use(VueRouter)
+
+describe('currentRoute', () => {
+  describe('children[]', () => {


I don't think a describe is necessary here. a test with should have children and another with should have parent.children is enough

Frondor · 2017-08-10T10:00:51Z

I believe this is overkill, child routes should never be exposed in currentRoute object. A new method like this.$route.getChildren() should be better in my opinion.

tmcdos · 2017-08-11T06:23:15Z

Child routes are exposed in every route object - just like the parent property. Why do you think a method is better than a property ?

Frondor · 2017-08-11T12:53:15Z

Actually, no. They aren't.
The parent route is just one referenced object, and if you are exposing children for those route objects, you're wasting a lot of memory for something that most people don't need. That's why in my opinion it should be called/built on demand instead.

tmcdos · 2017-08-15T13:17:19Z

Forgive me my profanity - perhaps using the original JavaScript objects as defined when instantiating the router instead of partially cloning them and exposing these partial clones would solve the problem with memory waste ?

kanatkubash · 2017-11-22T16:46:53Z

Are not js objects just referenced not copied, it shouldnt be wasting memory.

posva · 2018-01-13T23:08:20Z

Closing in favour of #1991

tmcdos added 2 commits July 27, 2017 19:01

posva requested changes Jul 31, 2017

View reviewed changes

posva added discussion proposal labels Oct 8, 2017

ianwalter mentioned this pull request Dec 15, 2017

Adding children and parent to currentRoute #1941

Closed

posva closed this Jan 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose child routes on currentRoute object; add property for a CSS class on inactive routes #1650

Expose child routes on currentRoute object; add property for a CSS class on inactive routes #1650

tmcdos commented Jul 27, 2017 •

edited

Loading

posva commented Jul 27, 2017

tmcdos commented Jul 28, 2017

posva left a comment

posva Jul 31, 2017

posva Jul 31, 2017

posva Jul 31, 2017

Frondor commented Aug 10, 2017

tmcdos commented Aug 11, 2017

Frondor commented Aug 11, 2017

tmcdos commented Aug 15, 2017

kanatkubash commented Nov 22, 2017

posva commented Jan 13, 2018

Expose child routes on currentRoute object; add property for a CSS class on inactive routes #1650

Expose child routes on currentRoute object; add property for a CSS class on inactive routes #1650

Conversation

tmcdos commented Jul 27, 2017 • edited Loading

posva commented Jul 27, 2017

tmcdos commented Jul 28, 2017

posva left a comment

Choose a reason for hiding this comment

posva Jul 31, 2017

Choose a reason for hiding this comment

posva Jul 31, 2017

Choose a reason for hiding this comment

posva Jul 31, 2017

Choose a reason for hiding this comment

Frondor commented Aug 10, 2017

tmcdos commented Aug 11, 2017

Frondor commented Aug 11, 2017

tmcdos commented Aug 15, 2017

kanatkubash commented Nov 22, 2017

posva commented Jan 13, 2018

tmcdos commented Jul 27, 2017 •

edited

Loading