-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[K7] Side nav interaction adjustments #29978
Conversation
Pinging @elastic/kibana-design |
💔 Build Failed |
767cfd3
to
f4702cc
Compare
💔 Build Failed |
@legrego here is the K7 side nav PR where we're making some tweaks. There is no EUI build yet, so to run this you have to link EUI. Also, the 'pin closed' feature has not yet been hooked up... but you can follow along here :) |
f4702cc
to
bf9467d
Compare
💔 Build Failed |
@@ -254,6 +256,7 @@ class HeaderUI extends Component<Props, State> { | |||
'navDrawer', | |||
this.state.isCollapsed ? 'collapsed' : 'expanded' | |||
)} | |||
style={{ transitionDuration: '90ms', transitionDelay: '150ms' }} // TODO Change defaults in EUI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is ugly, I know, but it gets us out of an EUI dependency/build/update in order to get this in for FF.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we've been told we can no longer use style
tags directly on components for security reasons. Am I correct @chandlerprall ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll remove this if we squeeze in one more EUI build and Kibana update before FF.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious to hear that answer, regardless, but I'll be removing this from the PR as a change is coming on the EUI side.
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels better to me. TY!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was able to take this PR for a spin this afternoon. My thoughts and feedback can be found below:
- The delayed hover action definitely helps with mistakenly interacting with it. Most of the time, the menu will not expand if I've accidentally navigated over it. However, when I'm purposefully navigating to click on something, I'm surprised when the navigation expands out. It actually ends up disorienting me a bit and I forget where I'm clicking for a brief second. Sometimes, it will also expand just as I've clicked it.
- Removing the delay when navigating away from the nav works well. It's a much more seamless experience with the with the faster collapse.
After trying to do my normal workflows (using a mouse pad, not mouse) I ran into the problem mentioned in my first observation numerous times. I imagine our users will as well. Hovering is complex and I'm worried that it may be impossible to tweak the experience to get it "just right". If it's too fast, you accidentally expand the nav more often. If it's too slow, it can surprise you. Given that everyone navigates differently, I think it would be very hard to find that middle ground. I'm beginning to think this is why we moved away from hover in 5.x. That all being said, I know that there is an effort to introduce pinning capabilities for 7.0 (after FF). I'd like to pose the question as to whether or not this should be the default experience. I was hopeful that these changes would improve my experience, but am still a bit concerned around having hover be part of the day-to-day workflows (and inherently, initial experience for new users)
@alexfrancoeur I think we're feeling the same and @snide has expressed the same thought about moving away from the hover-to-expand. All that said, it seems as though the adjustments in this PR provide enough relief that we should go ahead with them. Following FF, we'll take a step back, evaluate the feedback, and come up with some alternatives. Does that sound good to you? In other words, keep iterating. |
The inline styles have been removed. If you pull this now then you won't see the quick collapse animation. I'll rebase and correct that once #30116 is merged. |
It sounds like we're all mostly aligned, I just wanted to echo @alexfrancoeur's impressions and add that we should probably be concerned about users possibly perceiving the removal of pinning as a regression, since the UI has offered this function since 5.0. Something to keep an eye on as we gather feedback during the beta period. |
💚 Build Succeeded |
I've opened a new issue to track ongoing work that addresses the feedback above. More improvements to come for the new side nav - #30121 |
📓 This PR previously contained some initial work on adding a 'pin collapsed' button, but due to time and underlying complexity (mostly on the design/interaction side), we need to push that off. That said, the seemingly minor adjustments made by this PR provide some immediate relief for users who have reported inadvertent hovers coupled with a slow-to-close nav.
Summary
This PR adjust the K7 side nav interaction in the following ways based upon internal feedback and personal observations:
This PR also adds a new footer link to the bottom of the side nav. This will provide users the to keep the side nav in a collapsed state.This has been deferred, see note above.Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.For maintainers