elastic / eui

Elastic UI Framework πŸ™Œ
https://eui.elastic.co/
Other
54 stars 841 forks source link

Updates to navigation components in support of Kibana nav redesign #6759

Closed ryankeairns closed 1 year ago

ryankeairns commented 1 year ago

As part of the upcoming initiatives, we were tasked with redesigning Kibana's main navigation to accommodate new workstream-driven info architectures (IA). This redesigned navigation aims to unify the existing main and solution-level navigations into a singular navigation. Here are links to our latest designs:

The Platform UX team's proposal largely utilizes functionality from the EuiCollapsibleNav and EuiSideNav components, but it does deviate in a number of key ways as outlined below.

Implementation considerations

Before getting into the design details, there is an open question as to how we proceed with the implementation. Some options that come to mind are:

  1. Make no changes to the EUI components, and rely on the shared UX team to override styles and functionality and/or create custom components as needed.
  2. Make changes to the EUI components on main.
  3. Current thinking πŸ‘‰ Utilize a feature branch and ship changes as a separate package on npm.
  4. Add props with options that accommodate for the differences.
  5. Create an entirely new EUI component.

Depending on how we split the work in this issue, we may find that the visual design changes, for example, are safe to push to main. That said, it would be helpful to see them in the flesh and try things out in Kibana before committing to main. With this bit of uncertainty, option 3 seems a good path for trying things on in a low-risk manner.

Currently, there are two nav versions in Kibana. One for the current design and a second that is the beginning of this new design (emphasis on chrome service/API not UI design changes). It may be that Shared UX (i.e. Kibana Eng) want to apply this new/alternative package to only the latter for initial launch.

Suggestion

  1. Start with the Emotion conversion for these components
  2. Tackle the visual design changes together
  3. Proceed to functional enhancements

For item 3, the right-aligned icon is likely to be straightforward and relatively small. The new collapsible design will be the largest of all items in this effort and needs further discussion. Given that, I will mark it as hold until we get final alignment from our Product and UX peers.

Design changes

The sections below provide a component-level comparison of key deviations between the existing EUI components and the newly proposed navigation redesign. At a high level, the tasks can be summarized as follows:

### Tasks - Visual design changes
- [ ] `EuiCollapsibleNav` - remove borders
- [ ] `EuiCollapsibleNav` - change button styles
- [ ] `EuiCollapsibleNav` - change accordion arrow icons
- [ ] `EuiSideNav`- change item styles
- [ ] `EuiSideNav` - change accordion arrow icons
- [ ] `EuiSideNav` - change borders on children
- [ ] `EuiSideNav` - change text color on children
- [ ] `EuiSideNav` - change bottom padding on parent group
#### Tasks - Functional enhancements
- [ ] `EuiSideNav` - Display (optional) right-aligned icon on items
- [ ] ⏸️  **HOLD** `EuiCollapsibleNav` - Add new collapsed, icon-only design that uses `EuiContextMenu` (for this item, first estimate size, then let's discuss if/how to proceed)

EuiCollapsibleNav Differences

Functional enhancements

Visual changes

EuiSideNav Differences

Functional enhancements

Visual changes


EuiBreadcrumbs Differences

➑️ Moved breadcrumb work to https://github.com/elastic/eui/issues/7015

Functional enhancements

CCing @tsullivan in case there's anything we missed.

tsullivan commented 1 year ago

Are there changes that need to go into EUI components as part of the new header for serverless projects?

Specifically, creating the nav show/hide button looks like it is to be done with EuiHeaderSectionItem, but will have a box-like border. Is it best to implement that in the Kibana side with custom code, that uses the correct theme color for the border, or should the EUI provide a way to make that box-like header section item?

cee-chen commented 1 year ago

Is it best to implement that in the Kibana side with custom code, that uses the correct theme color for the border, or should the EUI provide a way to make that box-like header section item?

EUI should be handling this on our end.

cee-chen commented 1 year ago

Current thinking πŸ‘‰ Utilize a feature branch and ship changes as a separate package on npm. @ryankeairns My strong vote would be to create a new internal (not a public top-level exported) component called EuiCollapsibleNavBeta that merges to main and can be imported in Kibana by diving into the EUI library specifically, e.g.

import { EuiCollapsibleNavBeta } from '@elastic/eui/lib/components/collapsible_nav/collapsible_nav_beta';

Using this approach means significantly less headache with npm exports and staying up to date on main (especially since we have several tech debt items such as node/React versions occurring) while still allowing us to remain flexible and tweak the component as-needed if specific props aren't working as expected.

ryankeairns commented 1 year ago

Roger that regarding an internal component.

On the topic of top header, I suspect we need a separate issue to capture the collapse button design. I don't know that there are other new changes (perhaps breadcrumbs πŸ€” ), but I will suss those out.

cc:/ @MichaelMarcialis

MichaelMarcialis commented 1 year ago

I don't know that there are other new changes (perhaps breadcrumbs πŸ€” ), but I will suss those out.

@ryankeairns: Aside from what is mentioned in the original comment above (re: inclusion of deployment/project and space as part of breadcrumbs), I don't believe the current designs suggest any changes to breadcrumbs other than to use the type="page" prop to give it that more subdued styling.

The header remains largely the same for both expanded and collapsed navigation modes in larger viewport sizes. The only difference in the two is the icon being used to represent the collapse/expand action.

Where the header (and navigation) does start to exhibit some more significant change is at smaller viewport sizes (see Figma examples).

cee-chen commented 1 year ago

Michael, you're a mind-reader! I was just about to ask for mobile views :)

cee-chen commented 1 year ago

So I ended up going with option 5 (a brand new component called EuiCollapsibleNavItem that's an amalgamation of EuiCollapsibleNavGroup and EuiSideNavItem). I have an MVP of the new design that you can check out locally here in Storybook: https://github.com/elastic/eui/pull/6904 (see QA steps).

There are a couple UX questions I wanted to iron out before writing more tests and un-drafting the PR, such as:

  1. Whether items can be both a link to a page and an accordion toggle - I assumed yes and based most of my work around that given that's how Kibana currently operates
  2. What level of nesting we're expecting (I see Observability has a 3rd level in the Figma mocks but not a mock for what's inside there)
  3. Whether subitems should show isSelected state
  4. Whether EUI should attempt to implement the noted behavior (only 1 accordion open at a time) by default or leave that up to Kibana

@tsullivan, in particular I'd love to chat at next week's meeting about the new API and confirm that it'll work for your use case / doesn't have any missing props / won't be too annoying to use or migrate to.

Thanks y'all!

ryankeairns commented 1 year ago

Looking forward to the review. Looking really good!

MichaelMarcialis commented 1 year ago

Hey, @cee-chen. Here are some quick answers to your questions before our review today.

Whether items can be both a link to a page and an accordion toggle - I assumed yes and based most of my work around that given that's how Kibana currently operates

We spoke about this in Slack, but I'll paste my response here as well for visibility. My original intent for root level navigation items was that they could function as either an accordion or a link, but not both (to keep it simple and predictable for users). So in the case of the solutions (like Observability and Security), their root level item was intended to function only as an accordion toggle. That said, I know you mentioned having implemented it with the possibility for both, so we can evaluate further in our review.

What level of nesting we're expecting (I see Observability has a 3rd level in the Figma mocks but not a mock for what's inside there)

Correct. Based on the current designs there could be a maximum of three nested accordion levels. However, the navigation items and their location/depth is changing on a daily basis at the moment while the solutions are ironing things out. While I can't guarantee it will be more or less than that, my hope is that we never or rarely go beyond a third level, unless absolutely necessary.

Whether subitems should show isSelected state

Yes. I've added a quick example of the selected state in various levels and accordion states below. The thinking is that each individual item should be able to support a selected state. If the currently selected item has had its parent accordion collapsed by the user, that parent should be given the selected state while collapsed.

image

Whether EUI should attempt to implement the noted behavior (only 1 accordion open at a time) by default or leave that up to Kibana

Good question. If there are style implications involved when determining if the navigation should allow only one or all accordions to be opened simultaneously (as I suspect there may be, namely the flexbox styles), it may make sense for this option to be a prop on the EUI side. If I'm wrong though, and there are little to no style implications, then I suppose there's no harm in having the Shared UX team implement that restriction.

cee-chen commented 1 year ago

@ryankeairns Quick reminder to move out the requested EuiBreadcrumbs behavior out of this issue and into a separate one - as of the next EuiCollapsibleNavBeta PR, my work on the collapsible nav itself will be complete and I'll be closing this issue.

cee-chen commented 1 year ago

This issue will close once https://github.com/elastic/eui/pull/7034 merges, as a wholly usable MVP will be available for Kibana at that point. As a heads up, here are several things we skipped implementing as a conscious choice:

  1. Width animations on desktop: When in overlay/mobile mode, EuiFlyouts have a nice & performant transform animation that makes them transition out from the left or right. When in push/desktop mode, EuiFlyout (and therefore EuiCollapsibleNavBeta) does not have this animation. While it may be worth investigating trying to find a way to do this in the future, it will need to be heavily performance-tested as adding an animation to the push flyout will cause the entire page and its content to repaint with multiple layout flow changes.

  2. Docked popover sub-sub-items:

    Currently, nested items render as accordions. There is an argument to be made here for using EuiContextMenu instead; however this would require a significant amount of dev work to transmute EuiCollapsibleNavItem's data structure to EuiContextMenu's data structure. For now, we've decided not to take this approach, although this can certainly be revisited in the future if necessary.