Close nav drawer and flyout on nav item click

[ryankeairns]

Summary

This change will fix a Kibana issue elastic/kibana#30331 where the expanded nav drawer will remain open if you navigate to the app that you are already viewing. In other words, there is no refresh so the menu doesn't get reloaded to the collapsed state. I noticed this can also happen with the flyout - if it's open and you click on an app icon in the main menu, the flyout stays open. This PR addresses both cases.

Reviewer note

This change necessitates having both an href and an onClick on the nav list items. Previously, I had added a console warning in the case where both were provided, since the onClick was not being handled. With this change, onClick is now being handled so I have removed that warning and the related test.

Further, I have added a Prop note on the EuiListItem onClick that indicates a custom onClick will be applied when no href is provided. This reflects the current logic in EuiListItem.

Demo

Checklist

• This was checked in mobile
• This was checked in IE11
• This was checked in dark mode
• Any props added have proper autodocs
• Documentation examples were added
• A changelog entry exists and is marked appropriately
• This was checked for breaking changes and labeled appropriately
• Jest tests were updated or added to match the most common scenarios
• This was checked against keyboard-only and screenreader scenarios
• This required updates to Framer X components

[cchaos]

We've (@snide and I) had some discussions around this behavior lately (applying both href and onClick). We came to the conclusion that this is not a good practice. It can be unpredictable how a browser would handle this and skews the semantics of an anchor tag for accessibility.

We should discuss how to handle the edge cases. Whether it's to use a button if both are present and the onClick lastly calls window.goto or something else. But I'd like @chandlerprall and @thompsongl to weigh in here.

[thompsongl]

@cchaos Ryan and I talked yesterday, and I was ok with both in this case. I do think that allowing both is something we do not want to expose broadly, though.

My thought here was that there are very few cases where the the onClick actually happens; the href location change will happen first, which bypasses the onClick (the drawer resets to closed on remount). Only when clicking the link to the page/view the user is already on does the onClick happen.

I'd like to see a way to keep the thrown error in place for cases outside of this.

[ryankeairns]

Thank you both for your feedback.

@thompsongl I had an "override href warning" prop so that you are explicitly and knowingly making this decision. Does that fit what you are envisioning?

[cchaos]

I think my concern is that we're altering the EuiListGroupItem to allow this behavior for the EuiNavDrawer specifically. Which means that anyone consuming the list group can also apply this behavior. Is there any way to restrict this to just the EuiNavDrawer or only on the consuming (Kibana) side?

[thompsongl]

It may be that we want to address this case more holistically, because @cchaos point is valid. And I'm struggling to come up with an error solution that doesn't involve either an exposed prop or duplicating EuiListGroupItem as EuiNavGroupItem with that single difference

[chandlerprall]

My thought here was that there are very few cases where the the onClick actually happens; the href location change will happen first, which bypasses the onClick

onClick always happens first, which allows calling preventDefault on the event to prevent navigation (and URL update)

[ryankeairns]

Is there a way that the EuiNavDrawer or EuiNavDrawerGroup could detect a click on a child and then call the closeBoth() function?

Keeping in mind, we don't want this to happen on the footer item click or on items that have a flyout.

[ryankeairns]

Closed in favor of #1773