googlearchive / more-routing

144 stars 30 forks source link

Bug: more-route-selector don't work this dynamic core-item made with template repeat #17

Open jmorille opened 9 years ago

jmorille commented 9 years ago

on the "more-routing": "Polymore/more-routing#~0.5.0"

Replace this working static menu

 <more-route-selector>
      <core-menu>
        <core-item  icon="home"  label="Home" route="home">
          <a href="{{urlFor('home')}}"></a>
        </core-item>
        <core-item icon="supervisor-account"  label="User" route="user">
          <a href="{{urlFor('user')}}"></a>
        </core-item>
        <core-item icon="perm-media" label="Portfolio" route="portfolio">
          <a href="{{urlFor('portfolio')}}"></a>
        </core-item>
        <core-item icon="perm-contact-cal" label="Contact" route="contact">
          <a href="{{urlFor('contact')}}"></a>
        </core-item>
      </core-menu>
    </more-route-selector>

with the equivalent dynamic one

    <more-route-selector>
      <core-menu>
        <template repeat="{{menu in menus}}">
          <core-item icon="{{menu.icon}}" label="{{menu.label}}" route="{{menu.route}}">
            <a href="{{urlFor(menu.route)}}"></a>
          </core-item>
        </template>
      </core-menu>
    </more-route-selector>
</template>
<script>
..
 menus: undefined,
      created: function () {
        this.menus = [
          {icon: 'home', label: 'Home', route: 'home'},
          {icon: 'supervisor-account', label: 'User', route: 'user'},
          {icon: 'perm-media', label: 'Portfolio', route: 'portfolio'},
          {icon: 'perm-contact-cal', label: 'Contact', route: 'contact'}
        ];
      }

and no selection was possible on the menu item with the template one.

and a message is writed in javascript console

Child of  <more-route-selector> is missing route attribute "route":   <template repeat="{{menu in menus}}"></template>            more-route-selector.html:140
jmorille commented 9 years ago

In the more-route-selector.html:90 this.$.selection.routes have 5 routes

 this.$.selection.routes: Array[5]
0: null
1: Route
_paramObserver: ObjectObserver
 active: true 
children: Array[0]
compiled: Array[0]
depth: 0
driver: HashDriver
fullPath: "/"
numParams: 0
params: Object
parent: null
parts: Array[1]
path: "/"
__proto__: Route
2: Route
_paramObserver: ObjectObserver
 active: true 
children: Array[0]
compiled: Array[1]
depth: 1
driver: HashDriver
fullPath: "/user"
numParams: 0
params: Object
parent: null
parts: Array[1]
path: "/user"
__proto__: Route
3: Route
_paramObserver: ObjectObserver
active: false
children: Array[0]
compiled: Array[1]
depth: 1
driver: HashDriver
fullPath: "/portfolio"
numParams: 0
params: Object
parent: null
parts: Array[1]
path: "/portfolio"
__proto__: Route
4: Route
_paramObserver: ObjectObserver
active: false
children: Array[0]
compiled: Array[1]
depth: 1
driver: HashDriver
fullPath: "/contact"
numParams: 0
params: Object
parent: null
parts: Array[1]
path: "/contact"
__proto__: Route
length: 5
__proto__: Array[0]

I don't know where the active item is computed. And in the visual it was the portfolio that was selected..

jmorille commented 9 years ago

The selected menu is the next one that should be selected. It the selection is based on the index of the this.$.selection.routes array, the idx 0, generated by the template could explicit the wrong menu selected ??

jmorille commented 9 years ago

If you call the route http://localhost:9000/#!/user the code below is call 2 times, one with selectedIndex = 2 and next with selectedIndex = 1

 selectedIndexChanged: function() {
        if (!this.routingIsReady || !this._managedSelector) return;
        this._managedSelector.selected = this.selectedIndex; 
      },
nevir commented 9 years ago

Aha! So the null route is coming from the template element (template repeats stay in the final DOM, and all their generated elements follow them) - that's also where you're getting the warning. Might be worth having <more-route-selector> ignore <template> elements.

On the two routes being active, that's correct and has been a difficult behavior of more routing for me to document :( Here's a quick try:

If your URL is /foo/bar, the route /foo/bar is active, but the route /foo would also be active, as is the route /. (They're a prefix match). This allows for better composition where elements/contexts can create sub routes.

<more-route-selector> will choose the most specific route (e.g. route w/ the most path parts). So if / and /foo/ are active, it'll choose /foo over /. The bulk of that logic is in https://github.com/Polymore/more-routing/blob/master/more-route-selection.html#L141-171 when it sorts the routes by specificity

nevir commented 9 years ago

There's definitely some issues here, though I probably won't be able to dig into them for a week or two :( (patches welcome, though, if you do manage to root it out!)

I suspect the null is confusing things, and that double call to selectedIndexChanged also seems suspect...

jmorille commented 9 years ago

My feeling for this bug, is that caused by a difference of selectedIndex value of the 2 components and the . The