patternfly / patternfly-design

Use this repo to file all new feature or design change requests for the PatternFly project
114 stars 104 forks source link

NavExpandable - Allow using a ReactNode for display in place of a `string` title #1267

Closed dvail closed 10 months ago

dvail commented 1 year ago

Extension of an existing component:

NavExpandable

Currently the NavExpandable component gets its content from a string prop called title. In contrast, the NavItem component can accept any React.ReactNode via the children prop. We would like to be able to pass similar content for display to both components.

The use case for this in ACS is that we would like to be able to display a label next to items in the navigation denoting that they are "New", "Deprecated", "Tech Preview", etc. This works well for bottom level navigation items, as we can pass the <Label> directly as child content, but does not work for expandable navigation items. See a rough desired state in the screenshot below.

image

Since the title is simply rendered as the content of a button element we are able to workaround this by using a type assertion, but this isn't ideal long term:

<NavExpandable
    title={(<span>Non-string content</span>) as unknown as string}
>

A better, easy fix would be to update the type of the prop in PatternFly to be more permissive, but there are some potential downsides:

  1. Nested content could not be interactive since it resides in <button>, as pointed out by @nicolethoen
  2. This would restrict the component API in the future, since changes would have to take into account React.ReactNodes instead of just a string
  3. ???
nicolethoen commented 1 year ago

This may need to be combined with this nav enhancement effort: https://github.com/patternfly/patternfly-design/issues/1243 And we likely need to provide design guidance for how to layout icons + text + labels (for example) in nav items

andrew-ronaldson commented 10 months ago

This was fixed in react