ssleptsov / ninja-keys

Keyboard shortcuts interface for your website. Working with static HTML, Vanilla JS, Vue, React, Svelte.
https://ninja-keys-demo.vercel.app/
MIT License
1.65k stars 60 forks source link

Items listed after an item with children aren't displayed #26

Open jwoertink opened 2 years ago

jwoertink commented 2 years ago

I'm using this with Vue2, so not sure if it's related to that.... It seems which ever items has children: [], any item below that never gets rendered, but is still searchable.

data() {
    return {
      hotkeys: [
        {id: 'Home', title: 'Home'},
        {id: 'Publish', title: 'Publish'},
        {id: 'Settings', title: 'Settings'},
        {id: 'Theme', title: 'Theme', children: [{id: 'Light', title: 'Light Theme'}]}
     ]
  }
}

Screenshot from 2022-06-15 11-30-05

data() {
    return {
      hotkeys: [
        {id: 'Home', title: 'Home', children: [{id: 'Notifications', title: 'Notifications'}]},
        {id: 'Publish', title: 'Publish'},
        {id: 'Settings', title: 'Settings'},
        {id: 'Theme', title: 'Theme'}
     ]
  }
}

Screenshot from 2022-06-15 11-35-57 Screenshot from 2022-06-15 11-36-21

This may just be a configuration setup issue, but I'm not sure what I'm missing.

Using version 1.2.1

oddcelot commented 2 years ago

Isn't this like the desired behaviour? 😅 I mean if you are in "non-search-mode" you would navigate to the parent, confirm your selection and get the child routes/items displayed. If you enter search mode this tree-like structure gets flattened and you are presented with every possible match

I may also not get the point… so yeah 😄

The double Home entry for the breadcrumb was a bit of a head-scratcher, but makes sense now as the root level is also called Home (check this prop).

jwoertink commented 2 years ago

Isn't this like the desired behaviour?

I don't think so, but it may not be clear here. I suck at explaining stuff :sweat_smile:

The issue is the first path is Home which should be a parent path. Selecting Home and confirming would take you to a sub page with Notifications.... This works as expected :+1: Then below Home should be Publish which is another top-level path (without sub-children). The Publish doesn't render at all because the path about it (Home in this case) has children. If I swap the order of those two, then it would render Publish first, Home second, then nothing after that.

So basically, you can only have 1 top-level item with children. If you wanted multiple top-level items with children, they wouldn't render. Hopefully that makes more sense?

oddcelot commented 2 years ago

Ahhhh, i see 😬 Yip, this should work. I was wondering why there were no issues on my side, when using this. I opted for the flat structure. So this seems to work out fine:

[
  {
    id: "Home",
    title: "Home",
    children: ["Notifications"],
  },
  {
    id: "Notifications",
    title: "Notifications",
    parent: "Home",
  },
  {
    id: "Publish",
    title: "Publish",
  },
  {
    id: "Settings",
    title: "Settings",
  },
  {
    id: "Theme",
    title: "Theme",
  },
];

If you inspect the property ___flatData on the rendered <ninja-keys> element with the nested structure, you can kinda see what happens:

[
  {
    "id": "Home",
    "title": "Home",
    "children": ["Notifications"]
  },
  {
    "id": "Publish",
    "title": "Publish",
    "parent": "Home",
    "children": []
  },
  {
    "id": "Settings",
    "title": "Settings",
    "parent": "Home",
    "children": []
  },
  {
    "id": "Theme",
    "title": "Theme",
    "parent": "Home",
    "children": []
  },
  {
    "id": "Notifications",
    "title": "Notifications",
    "parent": "Home",
    "children": []
  }
]

My best guess would be something going sideways in flatten function… feels like the initial level isn't set right

jwoertink commented 2 years ago

Ah. I see. Sweet. Well, if it helps any, here's a bit more of how I'm actually using it.

let pages = [];

if (this.user.isSpecial) {
  pages.push({ id: "Publish", title: "Publish" })
}

So in this case, I'm not using a flat structure since each user could have a different set of pages based on their access level. I think I might be able to try the flat structure though. Thanks for the update!

minzdrav commented 2 years ago

Hi @oddcelot Thanks for your efforts to fix the problem! Will your pull request be in the master soon?

nxu commented 1 year ago

@oddcelot Thanks a lot for finding and solving the issue! Since there has been no activity for almost a year, and I found your fix very useful (or rather, necessary), I forked and published it temporarily as @nxu/ninja-keys.

For people finding this issue in the future: A quick workaround until the author merges the related PR is to replace ninja-keys with @nxu/ninja-keys in your package.json and update your dependencies.


@ssleptsov I hope everything's alright! Please ping or DM me if/when you merge the PR and release a hotfix so I can mark my fork as deprecated and advise people to return back to your original package.