[EuiCollapsibleNavBeta] Various API updates and cleanups

[cee-chen]

Summary

This PR does multiple different things:

Removes isGroupTitle logic in favor of a wildcard renderItem (c12fb27)

Old prop API:

<EuiCollapsibleNavItem items={[
  { isGroupTitle: true, title: 'something' },
}] />

New prop API:

<EuiCollapsibleNavItem items={[
  { renderItem: () => <EuiTitle size="xxxs"><div>something</EuiTitle></div> },
  { renderItem: () => <EuiSpacer size="m" /> },
  { renderItem: WhateverOtherCustomComponentYouWant },
}] />

I'm opting for this API over the old one because 1. it's simpler code and type-wise, and 2. it places the onus of consistent title and spacing styling directly on Kibana and not on EUI, and 3. it's a potential escape hatch for future customization (e.g. a button that toggles another flyout).

This new API should address elastic/kibana#167326

Adds EuiCollapsibleNav.[SubComponent] exports (3291371)

Old API:

<EuiCollapsibleNavBeta>
  <EuiFlyoutBody>
    <EuiCollapsibleNavItem />
  </EuiFlyoutBody>
  <EuiFlyoutFooter>
    <EuiCollapsibleNavItem />
  </EuiFlyoutFooter>
</EuiCollapsibleNavBeta>

New API:

<EuiCollapsibleNavBeta>
  <EuiCollapsibleNavBeta.Body>
    <EuiCollapsibleNavBeta.Item />
  </EuiCollapsibleNavBeta.Body>
  <EuiCollapsibleNavBeta.Footer>
    <EuiCollapsibleNavBeta.Item />
  </EuiCollapsibleNavBeta.Footer>
</EuiCollapsibleNavBeta>
// Note: For ease of migration, `EuiCollapsibleNavItem` is still available as a standalone export

The goal of this architecture is to 1. reduce separate imports, 2. more clearly scope subcomponents only intended to be used within the new EuiCollapsibleNavBeta component, and 3. match existing component architecture for template-level components, e.g. EuiPageTemplate.Section.

Adds new EuiCollapsibleNavBeta.Group top level component (#7208)

New API:

<EuiCollapsibleNavBeta>
  <EuiCollapsibleNavBeta.Body>
    <EuiCollapsibleNavBeta.Group
      title="Some solution"
      icon="logoElastic"
      items={[ ... ]}
    />
  </EuiCollapsibleNavBeta.Body>
</EuiCollapsibleNavBeta>

Please note that this new component DOES NOT address elastic/kibana#167323 and is not meant to. This new component, per Ryan's designs specifications and screencaps in #7208, is ONLY meant to be used as a top-level component (not as a subitem or in the footer), and has very specific styling and considerations for the desktop docked state.

I'd like to discuss the potential Kibana-side options for elastic/kibana#167323 more before implementing this at the EUI level. bc1e8a2 is one example of something Kibana could do with no intervention from EUI.

QA

• https://eui.elastic.co/pr_7228/storybook/?path=/story/euicollapsiblenavbeta-group--playground
• https://eui.elastic.co/pr_7228/storybook/?path=/story/euicollapsiblenavbeta--kibana-example

General checklist

• Browser QA
  • Checked in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
    - [ ] Checked for accessibility including keyboard-only and screenreader modes
• Docs site QA - N/A, beta component
• Code quality checklist
  • Added or updated jest and cypress tests
• Release checklist - N/A, beta component

[cee-chen]

@tsullivan @sebelga - I threw a quick sync on your calendars for tomorrow morning to quickly chat about the proposed API changes in the PR description and to make sure they actually meet your team's needs. Feel free to leave any comments/questions ahead of time for discussion.

[sebelga]

Thanks for putting this together!

Some thoughts:

// instead of this...
<EuiCollapsibleNavItem items={[
  { renderItem: () => <EuiTitle size="xxxs"><div>something</EuiTitle></div> },
  { renderItem: () => <EuiSpacer size="m" /> },
  { renderItem: WhateverOtherCustomComponentYouWant },
}] />

// Why not this? ...
<EuiCollapsibleNavItem>
  <EuiTitle size="xxxs"><div>something</EuiTitle>
  <EuiSpacer size="m" />
  <WhateverOtherCustomComponentYouWant />
<EuiCollapsibleNavItem>

I just love declarative syntax 😊

You should be able to get the items with

import { Children } from 'react';

...

const EuiCollapsibleNavItem = () => {
   ...
   const items = Children.toArray(children) as React.ReactElement[];
}

For the EuiCollapsibleNavBeta.Group the same would apply. Instead of having a items prop we would have

<EuiCollapsibleNavBeta.Group title="myGroup">
  <EuiCollapsibleNavBeta.Item>
    <div>Any content>
  </EuiCollapsibleNavBeta.Item>
  <EuiCollapsibleNavBeta.Item>
    <div>Any content>
  </EuiCollapsibleNavBeta.Item>
</EuiCollapsibleNavBeta.Group title="myGroup">

If need be you can filter and only get those EuiCollapsibleNavItem and discard the other children

const allChildren = Children.toArray(children) as React.ReactElement[];
const onlyItemChildren = allChildren.filter((child) => child.type === EuiCollapsibleNavItem);

And solving elastic/kibana#167323 would be one step away 😊

<EuiCollapsibleNavBeta.Group title="myGroup" wrapInAccordion={false}>
  <EuiCollapsibleNavBeta.Item>
    <div>Any content>
  </EuiCollapsibleNavBeta.Item>
  ...
</EuiCollapsibleNavBeta.Group title="myGroup">

[cee-chen]

Using children isn't efficient with the underlying logic/concerns that EuiCollapsibleNavItem handles. Under the hood, the component is essentially doing this:

• Am I a basic link, or am an accordion (do I contain child items)?
• If so, render the appropriate component (either a EuiCollapsibleNavLink or EuiCollapsibleNavAccordion, both of which contain their own separate complex internal logic)

Using children for that is incredibly fragile for multiple reasons:

• You need to analyze the name/type of the child component to see if it's the "correct" type, which fails if the consumer renames the component (e.g. adds another component wrapper around the component, or uses styled syntax, etc)
• You need to recursively dive into grandchildren in case the consumer does something like <div class="wrapper"><TheActualChildren /></div>, otherwise your logic fails (either checking type or count)

EUI has had to face this kind of API decision before (whether to use a specific prop for a limited set of child types, or to use children) and typically trying to use children, especially for stricter requirements, is a massive headache for everyone involved. I don't see the benefit here of allowing wildcard children in terms of developer time spent writing and testing the new architecture, when EUI has other higher priorities. That being said, if Kibana/Shared UX would like to take these beta components and move them to Kibana, that's certainly an option, as these components have not been publicly released on EUI and can be removed at any time

[kibanamachine]

Preview staging links for this PR:

• Docs site: https://eui.elastic.co/pr_7228/
• Storybook: https://eui.elastic.co/pr_7228/storybook

[elasticmachine]

💚 Build Succeeded

• Buildkite Build
• Commit: c8598ae

History

• 💚 Build #718 succeeded 4b31809
• 💔 Build #716 failed c762937
• 💔 Build #715 failed 3291371

[1Copenut]

👍 I have to say, Storybook feels like the piece that binds these big PRs together. Being able to review and edit data structures in real time is so helpful.

I like this sub-component approach a lot. Namespacing makes it clear what belongs to what.

[1Copenut]

After a couple of hours thinking about this, I agree with this thinking. The component is designed to house focusable links, so users should have no issue scrolling.

[cee-chen]

Thanks for sanity-checking this Trevor! FWIW it's overrideable if necessary as well, for edge cases, e.g. <EuiCollapsibleNavBeta.Body scrollableTabIndex={0}>

[1Copenut]

Fantastic. Sensible defaults and overrides where necessary.

[sebelga]

@cee-chen I made a mistake in my explanation but that's ok, your API aligns with other EUI APIs (e.g. Tabs) so it is consistent 😊

I still prefer a declarative API

<SomeGroupElement title="Accordion title">
  {/* Render custom content */}
  <SomeGroupElement.Item id="foo">
    <div>I can render anything I want</div>
  </SomeGroupElement.Item>

  {/* Use the default UI */}
  <SomeGroupElement.Item id="bar" title="I use the default UI" icon="someIcon" />
</SomeGroupElement>

than using props

<SomeGroupElement title="Accordion title"
  items={[{
    id: "foo",
    renderItem: () => <div>I can render anything I want</div>
  }, {
    id:"bar",
    title:"I use the default UI",
    icon: "someIcon"
  }]}
/>

The former feels more "react" and declarative to me, but that's just a preference.
Also, with the first implementation I should be able to do this and still render an accordion with all the children inside.

<SomeGroupElement title="Accordion title">
  {/* Render custom content */}
  <SomeGroupElement.Item id="foo">
    <div>I can render anything I want</div>
  </SomeGroupElement.Item>
  
  {/* I need to add all this in my accordion! :) */}
  <EuiSpacer size="l" />
  <img src="/why-not-an-image.jpg" />
  <div>Those JSX are just rendered as they appear, total freedom</div>

  {/* ...back to nav items... */}
  <SomeGroupElement.Item id="bar" title="I use the default UI" icon="someIcon" />
</SomeGroupElement>

I am not trying to convince you to change anything though! 😊 Just sharing what a declarative API allows vs using props.