[core] Fix docs segments and make navigation item segments optional

[apedroferreira]

Was fixing some segments in docs and maybe we can make them optional since they're already being checked in a few places in DashboardLayout and falling back to ''?

[apedroferreira]

This one can be omitted in this case, not sure if we want that.

[bharatkashyap]

Might be good to omit for ease of understanding

[Janpot]

Does it error when there are duplicates?

e.g.

• two page entries that have don't have segment defined
• What should be the meaning of

  [
    { kind: 'page', segment: 'hello', children: [ { kind: 'page' } ] }
  ]

Or more importantly. I was hoping we could build in the restriction of the navigation structure that for each possible path in the url there would exist at most one item in the whole hierarchy that corresponds to it. e.g. so that we can set the selected item correctly. Or so that we can match the breadcrumbs for page container. Is there any way we should guarantee this relationship? Or warn?

[apedroferreira]

Does it error when there are duplicates?

e.g.

• two page entries that have don't have segment defined
• What should be the meaning of

  [
    { kind: 'page', segment: 'hello', children: [ { kind: 'page' } ] }
  ]

Or more importantly. I was hoping we could build in the restriction of the navigation structure that for each possible path in the url there would exist at most one item in the whole hierarchy that corresponds to it. e.g. so that we can set the selected item correctly. Or so that we can match the breadcrumbs for page container. Is there any way we should guarantee this relationship? Or warn?

It works even with items that go to the same path, as for showing a selected item it just checks if their path matches the current pathname, and the item ids don't use the segment or titles, just indexes. So even if it's probably not ideal that a user sets duplicate paths, it's still possible and it works well.

I see that the PageContainer throws an error in these situations though, I guess you might have ran into some case that requires it. I could also throw a similar error in the same situation for these other components, probably, if we should make them consistent.

[
  { kind: 'page', segment: 'hello', children: [ { kind: 'page' } ] }
]

The item will lead to hello / which is probably what I would expect even if in practice it's a bit weird for the user to set up something like that. I guess we can make the logic omit the slash in those cases? Let me know if you'd prefer any alternative.

[Janpot]

It works even with items that go to the same path, as for showing a selected item it just checks if their path matches the current pathname, and the item ids don't use the segment or titles, just indexes. So even if it's probably not ideal that a user sets duplicate paths, it's still possible and it works well.

It feels wrong to me to show two active items. I think we should at least show a warning in the console (in dev mode) that this is not supported. And maybe show only 1 active item e.g. the first one it encounters.
Maybe PageContainer shouldn't error, but warn as well and use the same behavior as the navigation.

[apedroferreira]

It works even with items that go to the same path, as for showing a selected item it just checks if their path matches the current pathname, and the item ids don't use the segment or titles, just indexes. So even if it's probably not ideal that a user sets duplicate paths, it's still possible and it works well.

It feels wrong to me to show two active items. I think we should at least show a warning in the console (in dev mode) that this is not supported. And maybe show only 1 active item e.g. the first one it encounters. Maybe PageContainer shouldn't error, but warn as well and use the same behavior as the navigation.

Ok I think we can make it work like that, I'll try it out!

[apedroferreira]

I've addressed the latest feedback with commit 1388f27

• Add top-level check for any duplicated navigation paths as an effect in AppProvider, console warn for each duplicate found
• Remove errors checks/logic about duplicated paths from PageContainer component
• Only show the first item as selected in UI if 2 navigation items link to the same path

[Janpot]

To avoid an unnecessary effect, how about the following: During render, for each rendered navigation item, calculate the pathname, then look it up in a Set, if it already exists in the set, warn and don't mark it active. Add the pathname to the set.

[apedroferreira]

• Added validation during render but it required a bit of a hack using useMemo where useEffect would be more normal for ideal behavior... couldn't find a good cleaner solution so far.
• I think it's best to show multiple items as selected after all if the user sets up duplicated paths - users should fix it quickly anyway, and if we pick just the first one for example every method that checks selected items needs to be aware of the whole navigation (at all its levels) to know which the selected item is and be able to do comparisons and it seems like more trouble than it's worth, and we can't use the pathname as the source of truth for selected items.

[Janpot]

I don't think this can be safely done with concurrent mode. a render can be interrupted by a higher priority one and reset these in between rendering two navigation items. (Neither would it work correctly with the theoretical case of having this component twice in the tree).

Perhaps you can store the set in a ref and pass that as a prop to child components? then reset the ref in an effect. If this can't be done with idiomatic react and too much overhead, let's keep the effect then. Just make sure it has an early return in production, because there would be no sense in calculating the duplicate items if we're not going to warn on them.

[apedroferreira]

Refs work fine, I just wasn't sure about adding props just for this, but yeah global variables can fail in some rare cases so this is definitely better.

Changes here: c9269c6

[Janpot]

(Neither would it work correctly with the theoretical case of having this component twice in the tree).

Actually have to come back on this, it's not theoretical, we are rendering this component multiple times in a page on the docs 😄. In any case, you can never us a global variable like this for per-render behavior. It doesn't happen often, but I think this is a "never" that can be taken quite literally.

I just wasn't sure about adding props just for this

It's not a public component so it should be fine. In any case, we can always use a context as well if we want to avoid props

[apedroferreira]

I'll merge in a bit if this current approach with props seems good.