Skip to content
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

[Dashboard Usability] Use hover actions for dashboard panels #151233

Closed
Tracked by #149155
cqliu1 opened this issue Feb 14, 2023 · 3 comments · Fixed by #182535
Closed
Tracked by #149155

[Dashboard Usability] Use hover actions for dashboard panels #151233

cqliu1 opened this issue Feb 14, 2023 · 3 comments · Fixed by #182535
Assignees
Labels
Feature:Dashboard Dashboard related features impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:large Large Level of Effort Project:Dashboard Usability Related to the Dashboard Usability initiative Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas

Comments

@cqliu1
Copy link
Contributor

cqliu1 commented Feb 14, 2023

Overview

We should move the current actions menu out of the title bar of a panel and use hover actions similar to the one on controls. This will allow us to:

  • Take up less space with the panel header, especially in edit mode.
  • Remove one or more clicks from many common actions: delete panel, clone, edit, edit panel settings etc.

Design (WIP)

unnamed

Open Questions

  • What should we do about the panel time range notification, the library notification, the local filters notification etc. Should these be removed, turned into individual panel actions etc?
  • Is there a way to avoid showing the title bar when there is no title in edit mode? If we hid the title bar, where would the user drag the panel from?
@cqliu1 cqliu1 changed the title Use hover actions for dashboard panels 🎨 Needs design ⚠️ Needs Issue Use hover actions for dashboard panels Feb 14, 2023
@botelastic botelastic bot added the needs-team Issues missing a team label label Feb 14, 2023
@cqliu1 cqliu1 added Feature:Dashboard Dashboard related features Project:Dashboard Usability Related to the Dashboard Usability initiative labels Feb 14, 2023
@cqliu1 cqliu1 added loe:large Large Level of Effort impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. and removed needs-team Issues missing a team label labels Feb 14, 2023
@botelastic botelastic bot added the needs-team Issues missing a team label label Feb 14, 2023
@kibanamachine kibanamachine added the Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas label Feb 14, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@botelastic botelastic bot removed the needs-team Issues missing a team label label Feb 14, 2023
@cqliu1 cqliu1 changed the title Use hover actions for dashboard panels [Dashboard Usability] Use hover actions for dashboard panels Feb 14, 2023
@andreadelrio andreadelrio self-assigned this Feb 14, 2023
Heenawter added a commit that referenced this issue Mar 6, 2023
…2155)

Closes #135458
Unblocks #151233

## Summary

This PR adds the floating actions back into the regular HTML tree by
removing the `EuiPortal` that surrounded them, thus making them keyboard
accessible via some added CSS.

![Mar-02-2023
11-54-13](https://user-images.githubusercontent.com/8698078/222524586-8051b8e5-fe1e-48b2-bd83-30a90f9b3417.gif)


In order to do this, however, there were a few changes that had to be
made to the overall Dashboard HTML structure. Previously, as part of
[relocating the Dashboard
scrollbar](#145628), the
scrollable section of the app was moved to the Dashboard viewport, like
so:



https://user-images.githubusercontent.com/8698078/222511861-8707917c-9edc-4292-a182-58924bb00c8a.mov


<br>

While this had a lot of visual appeal, because of the structure of the
HTML tree, the floating actions had to be moved to an `EuiPortal` as
part of this change so that they would continue to float above the top
navigation bar rather than clipping behind it alongside the other
contents of the viewport - this made it impossible to add native
keyboard accessibility since they were removed from the natural HTML
structure.

Unfortunately, by removing the `EuiPortal` in order to allow for
keyboard accessibility, this meant that the scrollable section could
**no longer** be constrained to the viewport - this is because the
`z-index` of child of a given scrollable `div` is **always relative** to
its parent, which means that the floating actions would clip behind the
top nav bar regardless of how high you set their `z-index`:

<p align="center"><img
src="https://user-images.githubusercontent.com/8698078/222518354-80f1df75-69e5-4433-a256-d0b7dc57cd97.gif"/></p>



Therefore, in order to avoid this clipping, the scrollable section had
to remain at the top of the app, like so:


https://user-images.githubusercontent.com/8698078/222512203-60a88fc5-dd68-47ba-aeab-2425afc60a67.mov


<br>

In order to keep the benefit of the top query bar remaining in place,
the top nav bar was added to a **fixed position** `div` that floats
above the contents of the viewport as the user scrolls - this ensures
that the floating actions, which are now also positioned via a `fixed`
container, can still float above the nav bar while remaining part of the
natural order of the HTML tree.

As a follow up, we should also address
#152609.

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [x] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [x] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [x] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)


### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
bmorelli25 pushed a commit to bmorelli25/kibana that referenced this issue Mar 10, 2023
…stic#152155)

Closes elastic#135458
Unblocks elastic#151233

## Summary

This PR adds the floating actions back into the regular HTML tree by
removing the `EuiPortal` that surrounded them, thus making them keyboard
accessible via some added CSS.

![Mar-02-2023
11-54-13](https://user-images.githubusercontent.com/8698078/222524586-8051b8e5-fe1e-48b2-bd83-30a90f9b3417.gif)


In order to do this, however, there were a few changes that had to be
made to the overall Dashboard HTML structure. Previously, as part of
[relocating the Dashboard
scrollbar](elastic#145628), the
scrollable section of the app was moved to the Dashboard viewport, like
so:



https://user-images.githubusercontent.com/8698078/222511861-8707917c-9edc-4292-a182-58924bb00c8a.mov


<br>

While this had a lot of visual appeal, because of the structure of the
HTML tree, the floating actions had to be moved to an `EuiPortal` as
part of this change so that they would continue to float above the top
navigation bar rather than clipping behind it alongside the other
contents of the viewport - this made it impossible to add native
keyboard accessibility since they were removed from the natural HTML
structure.

Unfortunately, by removing the `EuiPortal` in order to allow for
keyboard accessibility, this meant that the scrollable section could
**no longer** be constrained to the viewport - this is because the
`z-index` of child of a given scrollable `div` is **always relative** to
its parent, which means that the floating actions would clip behind the
top nav bar regardless of how high you set their `z-index`:

<p align="center"><img
src="https://user-images.githubusercontent.com/8698078/222518354-80f1df75-69e5-4433-a256-d0b7dc57cd97.gif"/></p>



Therefore, in order to avoid this clipping, the scrollable section had
to remain at the top of the app, like so:


https://user-images.githubusercontent.com/8698078/222512203-60a88fc5-dd68-47ba-aeab-2425afc60a67.mov


<br>

In order to keep the benefit of the top query bar remaining in place,
the top nav bar was added to a **fixed position** `div` that floats
above the contents of the viewport as the user scrolls - this ensures
that the floating actions, which are now also positioned via a `fixed`
container, can still float above the nav bar while remaining part of the
natural order of the HTML tree.

As a follow up, we should also address
elastic#152609.

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [x] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [x] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [x] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)


### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
Heenawter added a commit that referenced this issue Mar 21, 2023
…3065)

Closes #143585
Closes #151767
Closes #152609

## Summary
This PR accomplishes three things, the first of which is moving the
edit/delete control hover actions to use the `uiActions` service - this
is the first step in moving existing panel actions (such as replacing
the panel, opening the panel settings flyout, etc.) to this hover
framework, which is outlined in
[this](#151233) issue.

While this was the primary goal of this PR, this also made the following
fixes possible:
1. Since I was refactoring the control editor flyout code as part of
this PR, I made it so that changes to the control's width/grow
properties are **only applied** when the changes are **saved** rather
than being automatically applied.

    | Before            | After |
    | ------------- | ------------- |
| ![Mar-14-2023
13-05-36](https://user-images.githubusercontent.com/8698078/225110954-d443f76b-4ac7-476c-b5b7-9af082d187fd.gif)
| ![Mar-14-2023
13-06-41](https://user-images.githubusercontent.com/8698078/225111172-ab9cce7e-7a70-45e4-ab06-5a87c053fb95.gif)
|
  


2. Since the edit control button is no longer a custom component, the
tooltip now responds to focus as expected.

    | Before            | After |
    | ------------- | ------------- |
| ![Mar-14-2023
13-05-36](https://user-images.githubusercontent.com/8698078/225113458-fe8f05fb-d56c-437a-b625-2a336bb4ba29.gif)
| ![Mar-14-2023
13-06-41](https://user-images.githubusercontent.com/8698078/225113313-d8cb7fcc-f611-48d0-83b4-f6fd147ce0ae.gif)
|


### Checklist

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [x] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [x] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)


### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: kibanamachine <[email protected]>
@thomasneirynck
Copy link
Contributor

++ on issue.

Any possibility of dealing with hiding the panel-header when there is no title.

e.g. The white No title header is redundant for metric-viz, which has its own title.
image

Maybe show header only on hover (?)

@ThomThomson
Copy link
Contributor

We could technically do this as a general change even before the hover actions are implemented. Honestly, we could remove the [No Title] placeholder entirely and use the same hover-ish behaviour from view mode in edit mode, with the gear icon instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Dashboard Dashboard related features impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:large Large Level of Effort Project:Dashboard Usability Related to the Dashboard Usability initiative Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants