[EuiCollapsibleNavBeta] Remove ability for accordion/group titles to contain links

[cee-chen]

Summary

closes elastic/kibana#168484

Hover/mouse states:

Focus/keyboard states:

QA

• https://eui.elastic.co/pr_7337/storybook/?path=/story/euicollapsiblenavbeta--kibana-example
• Confirm the accordion titles toggle on the entire button/text, not just on the arrow
• Confirm that when interacting with the accordion titles with the mouse, no outlines appear, but outlines do appear on keyboard tab
• Go to https://eui.elastic.co/pr_7337/storybook/?path=/story/euicollapsiblenavitem--edge-case-testing
• Confirm all nav items behave as expected (all accordion titles trigger open/close, no group titles have links)

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
• Designer checklist
  - [ ] Updated the Figma library counterpart

[cee-chen]

@MichaelMarcialis Could use your thoughts here again really quick. I added a "group" sub-component recently because Kibana is doing a mildly hacky thing where the arrow is getting hidden for "always open" accordions, which aren't really accordions at all (they're just groups).

With that in mind, should group titles be able to be links since accordion titles can't be links? Technically, they can be since there isn't the same accessibility limitation on them, but I'm not sure how confusing that would be from either a developer experience or user experience POV.

[MichaelMarcialis]

@MichaelMarcialis Could use your thoughts here again really quick. I added a "group" sub-component recently because Kibana is doing a mildly hacky thing where the arrow is getting hidden for "always open" accordions, which aren't really accordions at all (they're just groups).

With that in mind, should group titles be able to be links since accordion titles can't be links? Technically, they can be since there isn't the same accessibility limitation on them, but I'm not sure how confusing that would be from either a developer experience or user experience POV.

Is this "always open group" what is being show in the below screenshot?

If so, I suppose it wouldn't hurt to allow the group's parent to be linkable, as it doesn't carry the burden of also having to toggle an accordion. That said, I am curious about where this pattern is being used and why it would be necessary. For example, wouldn't using a navigation subheader with links below it generally achieve the same hierarchical presentation without stepping on the styles of the navigation accordions? CCing @ryankeairns as well, in case he has any thoughts on the matter.

[cee-chen]

That said, I am curious about where this pattern is being used and why it would be necessary.

I could be wrong but I think the reason why Kibana is doing it is the link in the accordion is in the current open page - essentially they want the link to always be visible in the sidenav(?). Please correct me if I'm wrong @tsullivan @sebelga

[ryankeairns]

I don’t foresee a need, in the this new nav, for having titles as links.
Let's take this more opinionated approach and make things more flexible, later, if the need arises. As opposed to allowing this flexibility and later having to remove/change it.

[cee-chen]

Thanks y'all - components have been updated to enforce either a href (i.e. it's a link) or items (it's an accordion or group). They can no longer be both. @MichaelMarcialis Do you want to do one final design/QA pass of https://eui.elastic.co/pr_7337/storybook/?path=/story/euicollapsiblenavbeta before moving forward?

[sebelga]

I could be wrong but I think the reason why Kibana is doing it is the link in the accordion is in the current open page - essentially they want the link to always be visible in the sidenav(?).

The reason we do it is for the top level item (at level 0). With the current design (where there is only 1 item at level 0 the main side nav) we don't want to make it collapsible

[cee-chen]

@sebelga I probably should have surfaced this more visibly, but I added a specific component for that top-level non-collapsible group. It's usable via <EuiCollapsibleNavBeta.Group>

Storybook: https://eui.elastic.co/storybook/?path=/story/euicollapsiblenavbeta-group--playground

I thought I saw in a screenshare with you at one point that there are sub-items that also are non-collapsible accordions however. Was I off on that?

[sebelga]

I thought I saw in a screenshare with you at one point that there are sub-items that also are non-collapsible accordions however. Was I off on that?

That was a temporary state until we fixed the controlled expand/collapse state of accordion (in elastic/kibana#169651).

With that said, it is still part of our API to allow an accordion to not be collapsible with the following props (but that's not used for now in any of the project navigations)

...
renderAs: 'accordion',
isCollapsible: false,

[cee-chen]

Gotcha, sounds good. So basically it shouldn't affect production IA, which is perfect / means this PR is good to go in terms of Kibana impact.

Also FYI that the items EUI API now supports isCollapsible out of the box - I'll see if I can't try to help clean up Kibana code with these new additions here sometime soon.

[MichaelMarcialis]

Thanks y'all - components have been updated to enforce either a href (i.e. it's a link) or items (it's an accordion or group). They ca no longer be both. @MichaelMarcialis Do you want to do one final design/QA pass of https://eui.elastic.co/pr_7337/storybook/?path=/story/euicollapsiblenavbeta before moving forward?

Thanks, @cee-chen. These changes look great. I only noticed two small things in my testing:

• There is a small gap between the text and accordion arrow doesn't open/close the accordion on click. The reason for this is that there are two buttons that trigger the accordion, with a small margin in between. This margin is where the click doesn't trigger the accordion. Would it be better to have a single button (containing both text and arrow) to correct this issue?

  

• This is a tangent issue, but I noticed that if the height of the nav footer contents grows larger than its container, navigation items can get cut off with no means to access them (as scrolling isn't supported). Should we allow vertical scrolling?

  

[cee-chen]

Would it be better to have a single button (containing both text and arrow) to correct this issue?

This is something that would need to be resolved at the underlying EuiAccordion level, as that is how the component behaves. I wasn't around for its creation so I'm not totally sure what the decision was behind separating the title into two buttons, but I presume there was an accessibility-related reason, at least at the time.

For now I've adding a workaround to this issue (specific to this component) in fa15cec, which basically removes the margins and increases the arrow size so there isn't any area where a click doesn't toggle the accordion.

Should we allow vertical scrolling?

Done! Thanks for catching these UI/UX improvements! ❤️

[cee-chen]

@1Copenut do you mind QA/reviewing this PR for merge sometime this week? 🙏

[kibanamachine]

Preview staging links for this PR:

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

[elasticmachine]

💚 Build Succeeded

• Buildkite Build
• Commit: 99e58fb

History

• 💚 Build #993 succeeded 882611b
• 💚 Build #975 succeeded 95182c7

[1Copenut]

This is a good refactor @cee-chen. The work here will help reduce errant links where groups shouldn't have them. I tested in the big three screen readers and ran through the manual QA you recommended.

I have one thought that I'm 99% sure should be a separate issue + PR, so I'll mention it here and open a feature request. When we have an accordion that toggles a group open and closed, we actually have two buttons (text + icon) with one ignored for keyboard navigation. Screen readers still recognize that icon button, so there's two buttons with the same name in SR form control menus. This is true with our accordion too, so I feel it'd be appropriate to make this change a separate PR.

[cee-chen]

Great call Trevor!