[Dashboard Usability] Moves scrollbar to panel section #145628

cqliu1 · 2022-11-17T20:58:07Z

Summary

Cloud deployment for testing: https://kibana-pr-145628.kb.us-west2.gcp.elastic-cloud.com:9243
User: elastic
Password: zuIno5Tuy4lVmhMwbt2C6NyY

This moves the scrollbar from the entire app to only the panel section of the dashboard app. The search/filter bar and editor toolbar will remain at the top while controls and panels scroll. The controls floating actions were extracted out into their own component for future use for panel actions.

Before

After

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

…rd/move-scrollbar

MichaelMarcialis

Nice enhancement @cqliu1! Leaving you a few comments below for your initial review.

On a side note, I found some oddities with the page template markup being generated. This may be an issue for the @elastic/appex-sharedux team, but mentioning it here for the sake of visibility/discussion. It looks like the div.kbnAppWrapper element is being rendered twice, and the div.app-fixed-viewport element is empty and unused. Not sure if this is intentional or not, but I couldn't immediately think of a good reason for us to do this. Is this intentional? Or should an issue be opened to fix this?

MichaelMarcialis · 2023-02-08T16:28:25Z

src/plugins/dashboard/public/dashboard_app/_dashboard_app.scss

+  @include euiBreakpoint('m', 'l', 'xl') {
+    @include kibanaFullBodyHeight();
+  }


Excluding the kibanaFullBodyHeight mixin from the s breakpoint appears to be causing dueling scrollbars to appear at small viewport sizes. Fixed in f1ad633.

Oh nice catch! I added a xs too because I was still seeing the dual scrollbar when I shrank the viewport width even more.

MichaelMarcialis · 2023-02-08T16:36:04Z

src/plugins/dashboard/public/dashboard_container/dashboard_container_renderer.tsx

+
+  const viewportStyles = css`
+    ${useEuiOverflowScroll('y', true)}
+  `;


Adding my two cents here. Beyond the aesthetics, I'm not a fan of using the gradient/fade/shadow effect between the unified search and page content because it's not consistent with what we do in other applications in Kibana (ex. Lens, Maps, Graph, etc.). The Lens approach may be best here, with a simple border in between the unified search and content (which will likely remove the concern with how dashboards look midway when scrolling). Thoughts?

cqliu1 · 2023-02-08T20:06:13Z

On a side note, I found some oddities with the page template markup being generated. This may be an issue for the @elastic/appex-sharedux team, but mentioning it here for the sake of visibility/discussion. It looks like the div.kbnAppWrapper element is being rendered twice, and the div.app-fixed-viewport element is empty and unused. Not sure if this is intentional or not, but I couldn't immediately think of a good reason for us to do this. Is this intentional? Or should an issue be opened to fix this?

@MichaelMarcialis It looks like the same duplicate kbnAppWrapper and empty app-fixed-viewport exists in the other apps too. I'm seeing it on the home page as well.

…kibana into dashboard/move-scrollbar

…rd/move-scrollbar

MichaelMarcialis · 2023-02-09T14:17:32Z

@MichaelMarcialis It looks like the same duplicate kbnAppWrapper and empty app-fixed-viewport exists in the other apps too. I'm seeing it on the home page as well.

@cqliu1: Yeah, I noticed that as well after I posted that comment. Would you happen to know if it is intentional or a bug that should be reported to @elastic/appex-sharedux? Or a perhaps a member from that team could comment here.

MichaelMarcialis

Thanks for making those changes, @cqliu1! This looks great. I left one final suggestion below for your re-review, but nothing worth holding you up any further for. Approving now.

MichaelMarcialis · 2023-02-09T14:27:23Z

src/plugins/dashboard/public/dashboard_app/_dashboard_app.scss

+  @include euiBreakpoint('xs', 's', 'm', 'l', 'xl') {
+    @include kibanaFullBodyHeight();
+  }


Since this is now using all available breakpoints, we should be able to remove the media query altogether.

Suggested change

@include euiBreakpoint('xs', 's', 'm', 'l', 'xl') {

@include kibanaFullBodyHeight();

}

@include kibanaFullBodyHeight();

MichaelMarcialis · 2023-02-09T14:33:08Z

src/plugins/dashboard/public/dashboard_container/dashboard_container_renderer.tsx

+
+  const viewportStyles = css`
+    ${useEuiOverflowScroll('y', true)}
+  `;


I like @ThomThomson's suggestion here for the single border in edit mode. Looks like you've already implemented it, so thumbs up from me!

…rd/move-scrollbar

kibana-ci · 2023-02-09T17:29:48Z

💚 Build Succeeded

Buildkite Build
Commit: 1219623

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`presentationUtil`	178	179	+1

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`presentationUtil`	161	163	+2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`controls`	175.2KB	174.6KB	-615.0B
`dashboard`	365.0KB	366.0KB	+1.1KB
`maps`	2.7MB	2.7MB	+22.0B
total			+494.0B

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`presentationUtil`	11	12	+1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`embeddable`	74.5KB	74.6KB	+22.0B
`presentationUtil`	40.4KB	41.5KB	+1.1KB
total			+1.1KB

Unknown metric groups

API count

id	before	after	diff
`presentationUtil`	217	219	+2

History

💔 Build #106966 failed 4d797ef
💔 Build #106695 failed 8e714c1
💔 Build #106632 failed ab51a19
💚 Build #106613 succeeded 011e2fe
💚 Build #106319 succeeded 2b3414f
💚 Build #106203 succeeded 5914895

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

* main: (115 commits) [Custom branding] Add custom logo to space selector (elastic#150284) [api-docs] 2023-02-10 Daily api_docs build (elastic#150831) [ci] build next docs in PRs when relevant files change (elastic#149991) [codeowners] allow overrides to take higher precedence (elastic#150821) [docs] Remove kibDevDocsOpsPluginDiscovery (elastic#150788) [Fleet] Fix max 20 installed integrations returned from Fleet API (elastic#150780) [maps] fix Changing resolutions on Heat map layer throws error in console (elastic#150761) fixes Failing ES Promotion: X-Pack API Integration Tests x-pack/test/api_integration/apis/maps/get_grid_tile.js (elastic#150768) [Synthetics] adjust overview scrolling e2e (elastic#150774) [Security Solution] Fixes bulk close alerts from exception flyout type bug (elastic#150765) Upgrade EUI to v74.1.0 (elastic#150235) [skip ci] Fix labeling for Infrastructure UI (elastic#150571) [Enterprise Search] Move pipelines modal to flyout (elastic#150727) [Security Solution] fix flaky endpoint tests (elastic#150652) Fixes the space selector page layout (elastic#150503) [Dashboard] [Navigation] Fix mount point bug (elastic#150507) [Infrastructure UI] Track host cloud provider on table entry click (elastic#150685) [Dashboard Usability] Moves scrollbar to panel section (elastic#145628) [Maps] fixes Kibana maps shows MVT borders if the geometry border style is greater than 1 (elastic#150497) [Cloud Posture][Dashboard] dashboard re-design enhancements (elastic#150394) ...

…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 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`: <img src="https://user-images.githubusercontent.com/8698078/222518354-80f1df75-69e5-4433-a256-d0b7dc57cd97.gif"/> 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 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)

…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 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`: <img src="https://user-images.githubusercontent.com/8698078/222518354-80f1df75-69e5-4433-a256-d0b7dc57cd97.gif"/> 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 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)

cqliu1 force-pushed the dashboard/move-scrollbar branch from 9bc2580 to f1911f5 Compare December 20, 2022 08:36

cqliu1 added 6 commits January 8, 2023 23:02

Moved scroll from entire app page to only panel section

00fdc98

Fixed cut off floating actions

83fd7ad

Removed unused import

b96acc0

Delete unused file

d52a748

Enable shadow effect on top border of panel section

e6e12f5

Clean up styles

d9a6083

cqliu1 force-pushed the dashboard/move-scrollbar branch from 31de6e6 to d9a6083 Compare January 9, 2023 07:10

cqliu1 added 14 commits January 9, 2023 12:55

Extracted floating actions component

c766516

Merge branch 'main' of https://github.com/elastic/kibana into dashboa…

94fb0a8

…rd/move-scrollbar

Updated snapshots

a7be283

Fix ts errors

550c9e2

Merge branch 'main' of https://github.com/elastic/kibana into dashboa…

6db3151

…rd/move-scrollbar

Undo lazy loading

96878e9

Updated snapshots

8e52180

Fixed a11y tests

a29b956

Update snapshots

5ae15c7

Merge branch 'main' of https://github.com/elastic/kibana into dashboa…

0c56299

…rd/move-scrollbar

Merge branch 'main' of https://github.com/elastic/kibana into dashboa…

0c67558

…rd/move-scrollbar

Merge branch 'main' of https://github.com/elastic/kibana into dashboa…

7657bb4

…rd/move-scrollbar

Fix options list test

0c2fe97

Remove test.only

de92647

cqliu1 added the ci:cloud-deploy Create or update a Cloud deployment label Feb 7, 2023

Merge branch 'main' into dashboard/move-scrollbar

2b3414f

MichaelMarcialis reviewed Feb 8, 2023

View reviewed changes

ThomThomson added v8.8.0 and removed v8.7.0 labels Feb 8, 2023

cqliu1 added 6 commits February 8, 2023 12:15

Added horizontal rule separate panels from filter bars

2bcc0ce

Fixes dual scrollbars in small viewport

f1ad633

Merge branch 'dashboard/move-scrollbar' of https://github.com/cqliu1/…

011e2fe

…kibana into dashboard/move-scrollbar

Reduce padding top of dashboard editor toolbar

ab51a19

Update controls snapshots

06f05ee

Merge branch 'main' of https://github.com/elastic/kibana into dashboa…

8e714c1

…rd/move-scrollbar

MichaelMarcialis approved these changes Feb 9, 2023

View reviewed changes

Remove breakpoints

ee7ffa4

cqliu1 removed the ci:cloud-deploy Create or update a Cloud deployment label Feb 9, 2023

cqliu1 added 2 commits February 9, 2023 07:59

Merge branch 'main' of https://github.com/elastic/kibana into dashboa…

4d797ef

…rd/move-scrollbar

Fix lint error

1219623

cqliu1 merged commit 1f03570 into elastic:main Feb 9, 2023

cqliu1 deleted the dashboard/move-scrollbar branch February 9, 2023 18:11

Heenawter assigned cqliu1 Feb 9, 2023

cqliu1 mentioned this pull request Feb 10, 2023

[EuiPopover] Scrolling an open popover down stretches page height elastic/eui#6592

Closed

cqliu1 removed their assignment Feb 22, 2023

Heenawter mentioned this pull request Mar 2, 2023

[Dashboard] [Controls] Make floating actions keyboard accessible #152155

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Dashboard Usability] Moves scrollbar to panel section #145628

[Dashboard Usability] Moves scrollbar to panel section #145628

cqliu1 commented Nov 17, 2022 •

edited

Loading

MichaelMarcialis left a comment

MichaelMarcialis Feb 8, 2023 •

edited by cqliu1

Loading

cqliu1 Feb 8, 2023

MichaelMarcialis Feb 8, 2023

cqliu1 commented Feb 8, 2023 •

edited

Loading

MichaelMarcialis commented Feb 9, 2023 •

edited

Loading

MichaelMarcialis left a comment

MichaelMarcialis Feb 9, 2023

MichaelMarcialis Feb 9, 2023

kibana-ci commented Feb 9, 2023

API count

[Dashboard Usability] Moves scrollbar to panel section #145628

[Dashboard Usability] Moves scrollbar to panel section #145628

Conversation

cqliu1 commented Nov 17, 2022 • edited Loading

Summary

Before

After

Checklist

For maintainers

MichaelMarcialis left a comment

Choose a reason for hiding this comment

MichaelMarcialis Feb 8, 2023 • edited by cqliu1 Loading

Choose a reason for hiding this comment

cqliu1 Feb 8, 2023

Choose a reason for hiding this comment

MichaelMarcialis Feb 8, 2023

Choose a reason for hiding this comment

cqliu1 commented Feb 8, 2023 • edited Loading

MichaelMarcialis commented Feb 9, 2023 • edited Loading

MichaelMarcialis left a comment

Choose a reason for hiding this comment

MichaelMarcialis Feb 9, 2023

Choose a reason for hiding this comment

MichaelMarcialis Feb 9, 2023

Choose a reason for hiding this comment

kibana-ci commented Feb 9, 2023

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

History

cqliu1 commented Nov 17, 2022 •

edited

Loading

MichaelMarcialis Feb 8, 2023 •

edited by cqliu1

Loading

cqliu1 commented Feb 8, 2023 •

edited

Loading

MichaelMarcialis commented Feb 9, 2023 •

edited

Loading