Allow locking of EuiNavDrawer in the expanded state #2247

cchaos · 2019-08-22T13:43:19Z

Based on these mocks by @ryankeairns

The EuiNavDrawer now includes a locking icon in the expand button to allow the user to keep the nav drawer open, pushing the body contents instead of overlaying.

A couple of notes:

The flyout (second drawer) will still always overlay
We restrict the locking ability to wider screens where there's enough room to accommodate both the nav drawer and the body content

Questions: @ryankeairns

Currently clicking on the "Collapse" portion of the toggle will also unlock the drawer. Is this expected?
Do you think the collapse toggle should look any different when in the locked state? If a user has this locked open for like 90% of the time, that button seems a bit prominent.

Checklist

Checked in dark mode
Checked in mobile
Checked in IE11 and Firefox
Props have proper autodocs
~~[ ] Added documentation examples~~
[ ] Added or updated jest tests
~~[ ] Checked for breaking changes and labeled appropriately~~
Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately

and hide extra action button on smaller screens

ryankeairns · 2019-08-22T14:18:51Z

src/components/nav_drawer/nav_drawer.js

ryankeairns · 2019-08-22T14:49:02Z

@cchaos to answer your two questions...

(both of these answers are influenced by the hope we can push this along quickly for 7.4)

That was my expectation, yes. I think that if you are deliberately clicking 'Collapse' then it is a safe assumption/expectation that the lock would be undone.
I agree it looks a little heavy visually but having just tried this out - and presuming it will often be used on larger screens - it seems to be far enough out of sight that it didn't make my eye wander down into the corner. Being fixed, the shadow feels essential for scrolling/stuff going under the button, but some other small short-term tweaks could be to lighten up that shadow perhaps? Transparency or some form of hiding feel not great on the surface... My personal opinion is to leave it as-is, live with it for a while, and see how it feels. If it becomes a distraction then let's revisit it as a follow-up task and explore alternative visual and interaction designs.

Seem reasonable? This keeps us moving forward and gives us some time to try it on for size.

myasonik · 2019-08-22T15:08:01Z

@cchaos I'm not able to tab to that bottom section... Do you know what's going on?

ryankeairns

👍 LGTM. The visual and interaction design look and work as expected across all breakpoints!

As noted in my previous comment, we could explore some additional tweaks to the Collapse button, but I think those are good follow-up tasks which take into account new feedback. As it stands, the change introduced here feels very minimal to the user (basically the addition of an action button) yet provides an impactful enhancement. This buys us some time for exploration which would be beneficial in this situation as the changes may involve more robust interactions based upon state.

ryankeairns · 2019-08-22T15:12:00Z

@myasonik It looks like the tab order is placing the footer button after the user profile button (in the header) / before the first link in the drawer.

myasonik · 2019-08-22T15:12:48Z

Also noticed a console error:

Warning: Failed prop type: The prop `onChange` is marked as required in `EuiSwitch`, but its value is `undefined`.
    in EuiSwitch

From context_menu_example.js:16

myasonik · 2019-08-22T15:16:51Z

Oh yup, good find @ryankeairns

Seems like a very weird tab order. Is that something we can address now @cchaos or should we log it for later?

More critically:
Also seems like it's missing a focus state? And after pressing it (via keyboard) focus is lost and reverts to the body. Can it stay on the button?

ryankeairns · 2019-08-22T15:22:53Z

Mentioning this issue for context; it's from when we initially released the Nav Drawer. There are some known issues with focus trap that were captured here: #1712

cchaos · 2019-08-22T16:19:41Z

Regarding the tab-order of the expand button, visually, it doesn't make sense, agreed. However, I had intentionally put the expand button first so that users who are using the keyboard to navigate, can get to it quicker and expand the drawer before they need to tab through all the items, then expand to see the options.

thompsongl

I can't leave a comment on the line, so, on 198ish isCollapsed and isLocked need to be unpacked from ...rest so they don't get placed on the div (causes a warning).

I could be mistaken, but Dave mentioned something about localStorage when using this in Kibana. Is the intent to keep locked preference in localStorage? If so, we'll need to provide a callback so that external state can be updated on local state changes.

thompsongl · 2019-08-22T16:12:36Z

src/components/nav_drawer/nav_drawer.js

@@ -36,6 +70,16 @@ export class EuiNavDrawer extends Component {
    }, 150);
  };

+  collapseButtonClick = () => {


Need to remove the resize event listener in this function, also. Otherwise window resizing continues

cchaos · 2019-08-22T17:06:48Z

@ryankeairns

but some other small short-term tweaks could be to lighten up that shadow perhaps?

Yes, I think that would be really helpful. Even just at all states. I think it's a bit heavy for it's location compared to the EuiHeader.

cchaos · 2019-08-22T17:08:53Z

@myasonik

Also noticed a console error: ... from context_menu_example.js:16

Yep, but that wasn't changed in this PR.

cchaos · 2019-08-22T17:15:28Z

@thompsongl

If so, we'll need to provide a callback so that external state can be updated on local state changes.

Best way to do that?

Unpack isCollapsed, isLocked Remove resize listener on collapse

thompsongl · 2019-08-22T17:20:43Z

Add a prop onIsLockedUpdate that accepts a function (signature like, (isLocked: boolean) => void). And then call that function whenever setState updates isLocked

cchaos · 2019-08-22T17:32:00Z

Added callback:

thompsongl · 2019-08-22T17:39:38Z

src/components/nav_drawer/nav_drawer.js

+      window.addEventListener('resize', this.functionToCallOnWindowResize);
+    }
+
+    this.props.onIsLockedUpdate(!this.state.isLocked);


It's possible for this.props.onIsLockedUpdate to be undefined, so check before calling it (and when it's called in collapseButtonClick)

thompsongl

Interaction looks good 👍

cjcenizal · 2019-08-22T20:01:47Z

Tested this locally and it's fantastic! Thank you for making this change. I experienced a couple friction points in the UX I wanted to raise for your consideration.

Revealing the lock button on hover

In general, I've noticed our components and UIs try not to reveal content on-hover, and I think we've made that decision consciously in the past. Any reason not to keep the lock button visible? It seems like hiding it is an accessibility issue for people using the keyboard to navigate because there is no clear destination to navigate to until you're already there. What do you think @myasonik ?

Lock icon meaning

The lock is used elsewhere in Kibana's UI to represent a "reserved", "protected", or otherwise uneditable state. I'm worried that we might be overloading it with meaning if we use it here in this way. How about a pin icon instead?

Forgotten expanded state

I love the way the nav collapses on narrower screens. Nice detail which improves the UX. But can we make it expand again once the user makes their window wider? Put another way, can the nav "remember" if I clicked the lock button and re-apply that state when the screen width can support it?

ryankeairns · 2019-08-22T20:38:23Z

@cjcenizal regarding the icon: we have plans to use the pin in the nav for users to 'pin' apps to the top of the list, likewise with stars, thus the use of the lock in this case.

Regarding the forgotten expanded state: Could this be handled on the Kibana/consuming app side? Something like if localStorage.navIsLocked is 'on' then add the navIsLocked class ?

cjcenizal · 2019-08-22T21:24:29Z

we have plans to use the pin in the nav for users to 'pin' apps to the top of the list, likewise with stars, thus the use of the lock in this case.

Thanks for the explanation! It does sound to me like the meaning of the pin in this case is a lot closer to the expand/collapse lock button than the lock icon in the screenshot I shared (the user is "pinning" items in place and "pinning" the drawer in place), but I'll let you make the final call on that.

Re forgotten state, I would expect the responsive behavior to be either wholly controlled by the component or to be wholly controlled by the owner. Currently, it behaves as if it only controls responsive behavior if the window is shrinking but it doesn't control it if the window is expanding. It might be simplest if the owner is responsible for implementing the responsive behavior entirely. This way state only comes from one place, so it's easier to understand and define the behavior we want.

cchaos · 2019-08-22T21:58:26Z

@cjcenizal We had discussed the responsive handling and had previously decided that it should be "remembered" by the consumer not the component. However, I'm now seeing the limitation of that in which the consumer would then also need to continually pass the locally stored state on resize. I think we can it to remember on resize, however, if it seems like it's more complicated, we may have to punt to second PR so that we can at least get this inital functionality in for FF.

As for the hovering icon, we're using this pattern for all of the nav list items. This isn't a new convention as we also use it in tables for actions.

cchaos · 2019-08-22T22:13:00Z

There are a lot of edge cases to remembering that locked state the way we handle re-use of the collapse functions no matter the width of the browser. When we had talked about this functionality, we considered the fact that most users aren't changing their browser size so much to make locking the nav again a nuisance. Therefore I'm inclined to punt that functionality for now.

snide

Punt is OK. Let's merge so we're safe for feature freeze. Edge case is likely only to be noticed by people developing Kibana (who resize into small sizes for testing). Agree it's nice to have, so we should fit it in later.

Want to make sure we're not holding up the line with this one.

* Added a locked state of EuiNavDrawer * Auto-collapses at smaller window sizes * Collapse all when collapse button is clicked and hide extra action button on smaller screens * Fix passing classname to extra action * Allow passing `isLocked` as prop * Added is `onIsLockedUpdate` callback

cchaos added 6 commits August 21, 2019 19:14

Added a locked state of EuiNavDrawer

a22e96e

Auto-collapses at smaller window sizes

8c0b566

Collapse all when collapse button is clicked

d1a3855

and hide extra action button on smaller screens

Fix passing classname to extra action

c129119

Allow passing isLocked as prop

f267226

cleanup

a4b44d3

cchaos requested review from ryankeairns and thompsongl August 22, 2019 13:43

cchaos added 2 commits August 22, 2019 09:45

Remove default

80c5457

Better labels

b14bbe9

Fix a11y

797e931

cchaos commented Aug 22, 2019

View reviewed changes

src/components/nav_drawer/nav_drawer.js Show resolved Hide resolved

ryankeairns approved these changes Aug 22, 2019

View reviewed changes

thompsongl suggested changes Aug 22, 2019

View reviewed changes

PR fixes

6dcd158

Unpack isCollapsed, isLocked Remove resize listener on collapse

Added is onIsLockedUpdate callback

7d0c5ab

cl

37ec5c1

thompsongl suggested changes Aug 22, 2019

View reviewed changes

cchaos added 2 commits August 22, 2019 14:12

Check if callback extists first

819e51d

Role doesn’t work on the expand button itself

12fd2dc

cchaos requested a review from thompsongl August 22, 2019 18:19

thompsongl approved these changes Aug 22, 2019

View reviewed changes

cchaos and others added 2 commits August 22, 2019 17:59

Reverse icon and fix aria-label/title

806ff39

Merge branch 'master' into pin-nav-drawer

b17886a

snide approved these changes Aug 22, 2019

View reviewed changes

cchaos merged commit 59fc1a1 into elastic:master Aug 22, 2019

cchaos deleted the pin-nav-drawer branch August 22, 2019 22:54

thompsongl mentioned this pull request Aug 23, 2019

Upgrade EUI to 13.6.0 elastic/kibana#43916

Merged

nandorojo mentioned this pull request May 15, 2020

EuiNavDrawer isLocked prop does not shift main content to the right elastic/next-eui-starter#5

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow locking of EuiNavDrawer in the expanded state #2247

Allow locking of EuiNavDrawer in the expanded state #2247

cchaos commented Aug 22, 2019 •

edited

Loading

ryankeairns commented Aug 22, 2019

ryankeairns commented Aug 22, 2019 •

edited

Loading

myasonik commented Aug 22, 2019

ryankeairns left a comment

ryankeairns commented Aug 22, 2019

myasonik commented Aug 22, 2019

myasonik commented Aug 22, 2019

ryankeairns commented Aug 22, 2019

cchaos commented Aug 22, 2019

thompsongl left a comment •

edited

Loading

thompsongl Aug 22, 2019

cchaos commented Aug 22, 2019

cchaos commented Aug 22, 2019

cchaos commented Aug 22, 2019

thompsongl commented Aug 22, 2019

cchaos commented Aug 22, 2019

thompsongl Aug 22, 2019

thompsongl left a comment

cjcenizal commented Aug 22, 2019 •

edited

Loading

ryankeairns commented Aug 22, 2019

cjcenizal commented Aug 22, 2019

cchaos commented Aug 22, 2019

cchaos commented Aug 22, 2019

snide left a comment

Allow locking of EuiNavDrawer in the expanded state #2247

Allow locking of EuiNavDrawer in the expanded state #2247

Conversation

cchaos commented Aug 22, 2019 • edited Loading

Based on these mocks by @ryankeairns

Checklist

ryankeairns commented Aug 22, 2019

ryankeairns commented Aug 22, 2019 • edited Loading

myasonik commented Aug 22, 2019

ryankeairns left a comment

Choose a reason for hiding this comment

ryankeairns commented Aug 22, 2019

myasonik commented Aug 22, 2019

myasonik commented Aug 22, 2019

ryankeairns commented Aug 22, 2019

cchaos commented Aug 22, 2019

thompsongl left a comment • edited Loading

Choose a reason for hiding this comment

thompsongl Aug 22, 2019

Choose a reason for hiding this comment

cchaos commented Aug 22, 2019

cchaos commented Aug 22, 2019

cchaos commented Aug 22, 2019

thompsongl commented Aug 22, 2019

cchaos commented Aug 22, 2019

thompsongl Aug 22, 2019

Choose a reason for hiding this comment

thompsongl left a comment

Choose a reason for hiding this comment

cjcenizal commented Aug 22, 2019 • edited Loading

Revealing the lock button on hover

Lock icon meaning

Forgotten expanded state

ryankeairns commented Aug 22, 2019

cjcenizal commented Aug 22, 2019

cchaos commented Aug 22, 2019

cchaos commented Aug 22, 2019

snide left a comment

Choose a reason for hiding this comment

cchaos commented Aug 22, 2019 •

edited

Loading

ryankeairns commented Aug 22, 2019 •

edited

Loading

thompsongl left a comment •

edited

Loading

cjcenizal commented Aug 22, 2019 •

edited

Loading