Skip to content

[Task](ui): fix SideNavigation problems #1573

@franzheidl

Description

@franzheidl

Our SideNavigation has some problems that we should fix. These problems span across SideNavigation, SideNavigationGroup, and SideNavigationItem, but they are related and/or interdependent, so we collect them here:

SideNavigation

Disabling SideNavigation

SideNavigation passes disabled down to the internal Navigation component to the following effects:

  • aria-disabledis set to true on the <ul> element (which is correct and desired)
  • a juno-sidenavigation-disabled class is set (also correct)
  • Publishes navigationDisabled: true into NavigationContext. This is correct, however SideNavigationItem never consumes this context. As a result, setting disabled: true on the SideNavigation component will be completely unconsequential for any child items.

Set activeItem on SideNavigation

Currently, we allow setting an activeItem prop on SideNavigation. This has problems:

  1. It will never have any effect, since we pass this to the internal Navigation component, which is never consumed by SideNavigationItem, so it effectively is a dead end currently.
  2. Typing is inconsistent / too wide: We allow reactNode as a type, but coerce into string as soon as the prop hits our internal Navigation component. Passing a jsx object would result in "[object Object]", which is useless even if we were to consume the context properly.

Suggestion: Remove the activeItem prop altogether from SideNavigation and handle selecting item exclusively by setting selected on an individual item directly, thus making the compnent truly "logic-less" in terms of managing the active item.
This would handle developer expectations by not implying any "magic" when passing an active item, and spare us any typing shenanigans.

However we would need to define a mechanism to reliably return a useful vaule with on ActiveItemChange regardless whether we use Navigation to handle it or bypass it by handling the handler in SideNavigation directly. We could provide a function in our context an item can use to pass whatever piece of data a developer user would want to have returned from onActiveItemChange.

Stories

There are some minor problems with the stories for SideNavigation in storybook:

  • Home button in default story and „Dashboard“ in „Interactive Navigation“ story point to „/„, thus causing 404 when clicked
  • Story name „Interactive Navigation“ does not communicate the story's intent: Navigations are always interactive?

SideNavigationGroup

Expand/Collapse Toggle Must Be a Native <button>

The expand/collapse toggle is currently a <span role="button" tabIndex={0}> with no onKeyDown handler — keyboard users can tab to it but cannot activate it with Enter or Space. Replace the <span> with a native <button>, which handles keyboard interaction, focus management, and disabled semantics without any extra code.

disabled Context Is Not Created

SideNavigationGroup accepts a disabled prop, but handles it only partially/inconsistently. As with SideNavigation, setting disabled: true currently does not have any effect on the items contained within the group.

When a group is set to disabled, the child items will never know, as it does not create any context that child elements could consume. It would be possible to render an open but disabled group with fully functional children inside. The only consequence disabled: truecurrently has is that we apply some disabled styles, and, when clicking the expand chevron, we return handleToggleOpen early.

Setting a group to disabled should disable its child items by publishing a context the children can consume.

Set Class to Mirror open And disabled States

SideNavigationGroup should set juno-sidenavigation-group-open and juno-sidenavigation-group-disabled classes depending on state (not raw props!)

SideNavigationItem

Expand/Collapse Toggle Must Be a Native <button>

Same issue as with SideNavigationGroup: the expand/collapse chevron uses <span role="button" tabIndex={0}> with only an onClick handler. Replace with a native <button>. Note: this toggle is distinct from the item's primary navigation element — the primary element is correctly an <a> when href is passed, but the expand chevron is always an independent interactive control and should always be a <button>.

Use Prefixed Classes To Mirror open And selected States

Currently, we add a plain selected class to selected items, and no class to open items (if there are children). We should use juno-sidenavigation-item-selected and juno-sidenavigation-item-open respectively.

SideNavigationItem Should Consume disabled Contexts

SideNavigationItem never consumes any context provided by the parent SideNavigation or SideNavigationGroup components. As a result, individual item muest be set diasabled: true in order to visually and functionally disable them.

SideNavigationItem should reliably be disabled when any parent SideNavigation or SideNavigationGroup item or the SideNavigaitonItem itself is set disabled: true.

Disabling SideNavigationItem Is Inconsistent / Flawed

Currently, we do not consistently disable SideNavigationItem:

<a> vs <button> Are Handled Inconsistently When disabled

For <button> we completely remove onClick when disabled, for <a> we leave it intact, but return early, while completely removing href. This is inconsistent, and our handling of <a> implies click handling intent. We should remove any onClickhandler completely – if they were ever passed.

<button> Doesn't Use The Native disabled Attribute

Using onClick={undefined} instead of disabled={true} on a <button> means:

  • No aria-disabled semantics — screen readers won't announce the item as disabled
  • The element stays in the tab order
  • No browser-native visual/behavioral treatment

The <a> case at least can't use disabled (it's not a valid HTML attribute for anchors), but for the button branch the native attribute should still be used.

Once <button> is set to disabled however, the applied tailwind disabledStyles from the component and the .juno-sidenavigation-item:disabled styles from the external ccs file will conflict / do the same thing, rendering one of them dead. We should remove one of them.

Inconsistent Cursor Classes

Currently we run against raw props to decide what cursor class to add ot an element, instead of using disabled state:

${onClick || href || children ? "jn:cursor-pointer" : "jn:cursor-default"}

So if disabled=true and onClick is also passed, the class list contains both jn:cursor-not-allowed (from disabledStyles) and jn:cursor-pointer. In this case it purely depends on the sequence the classes appear in our generated css, makign it unpredictable which cursor will be shown.

Expand Icon's Tab Stop Is Never Removed

<span onClick={handleToggleOpen} role="button" tabIndex={0}>

tabIndex={0} and role="button" are hardcoded — they're never cleared when disabled=true. A keyboard user can still tab to the expand chevron and press Enter. The click is silently swallowed by if (disabled) return, but the element is still reachable and appears interactive. The disabledStyles are applied to the <Icon> inside the span, not to the span itself.

SideNavigationItem Renders Empty aria-label by Default

Currently, the ariaLabel prop on SideNavigationItem renders an empty aria-label attribute since it default to "" (empty string). When there is no ariaLabel, the aria-label attribute should not render at all.

Define and Implement Consistent Disabling Strategy

The disabling mechanism should always follow the same, consistent disabling strategy:

  • Remove/void the respective handler completely, OR return early – and then stick to this strategy across all affected components / elements.
  • set disabled explicitly when available for native element
  • Alway set aria-disabled to true.
  • For anchor elements remove href
  • set not-allowed cursor consistently
  • clean up redundant or conflicting style rules from component styles and css files

We should consider introducing, documenting and enforcing a universal disabling startegy across all our components (out of scope of this ticket).

Sub-tasks

  • SideNavigation: Discuss: decide, implement: remove activeItem prop
  • SideNavigation: Discuss: decide , implement: how to handle onActiveItemChange
  • SideNavigation: fix story propblems (some link targets, story name/title)
  • SideNavigationGroup: replace <span role="button"> expand/collapse toggle with a native <button>
  • SideNavigationGroup: publish disabledcontext
  • SideNavigationGroup: set properly prefixed classes to mirror disabled and open states
  • SideNavigationItem: replace <span role="button"> expand/collapse toggle with a native <button>
  • SideNavigationItem: set properly prefixed classes to mirror selected, disabled and open states
  • SideNavigationItem should consume parent disabled context provided by SideNavigation and / or SideNavigationGroup
  • SideNavigationItem: make sure no empty aria-label is rendered (set default to undefined)
  • SideNavigationItem implement a consistent disabling strategy

Related Issues

#1460 The ticket expresses reporter intent, albeit our (non-working) implied mechanism to set an active item on the parent does not appear to have been used. Relevant context nonetheless, tells us we should document and communicate capabilities more clearly in the future. Should be closed with reference to this ticket when done.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingdocumentationImprovements or additions to documentationui-componentsAll tasks related to juno-ui-components library

    Type

    Projects

    Status

    New

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions