[Dashboard Usability] Moves scrollbar to panel section #145628
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,8 +33,6 @@ | |
| */ | ||
| .dshLayout-isMaximizedPanel { | ||
| height: 100% !important; /* 1. */ | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,10 +10,11 @@ import './_dashboard_container.scss'; | |
|
|
||
| import { v4 as uuidv4 } from 'uuid'; | ||
| import classNames from 'classnames'; | ||
| import React, { useEffect, useMemo, useRef, useState } from 'react'; | ||
| import useObservable from 'react-use/lib/useObservable'; | ||
|
|
||
| import { EuiLoadingElastic, EuiLoadingSpinner, useEuiOverflowScroll } from '@elastic/eui'; | ||
| import { css } from '@emotion/react'; | ||
| import { useReduxEmbeddableContext } from '@kbn/presentation-util-plugin/public'; | ||
|
|
||
| import { | ||
|
|
@@ -110,13 +111,20 @@ export const DashboardContainerRenderer = ({ | |
| { 'dashboardViewport--screenshotMode': isScreenshotMode() }, | ||
| { 'dashboardViewport--loading': loading } | ||
| ); | ||
|
|
||
| const viewportStyles = css` | ||
| ${useEuiOverflowScroll('y', true)} | ||
| `; | ||
|
There was a problem hiding this comment. Nice use of Nit. The overflow creates a blurry gradient on the viewport that maybe shouldn't appear when the viewport is at the top of the page. Would increasing the padding on the viewport fix this?
There was a problem hiding this comment. Is adding padding the best solution here? Ideally we wouldn't want to change the look of the dashboard like that. Plus, the gutter created by the tops of the Controls is roughly the same as the gutter between the elements of the Unified Search bar. @andreadelrio, thoughts on this? Maybe there's some way to remove the fade There was a problem hiding this comment. I can just remove the blur by not passing I think adding 4px top padding to the controls is sufficient to offset the blur, but if we don't want the additional height added, I can remove the blur. Here are some screenshots to compare: Extra top paddingwithout adding top padding
with 4px top padding
Blur vs no blurwith blur, scroll midway down the dashboard
no blur, scroll midway down the dashboard
There was a problem hiding this comment. Crazy idea, but could we add a scroll listener that removes the blur only when at the top? Similar to the scroll listener that was added for options list suggestions. Might be overkill though 🤷 There was a problem hiding this comment.
Padding the top and bottom is recommended by EUI. But we could also just remove the mask as Catherine suggested. I don't feel strongly either way. There was a problem hiding this comment. I think that the border doesn't work too well when the page is at the top. In view mode, the border when the page is at the top makes the Controls look very separate from the filter context, which I suppose is somewhat true. In edit mode, having two borders like that in quick succession is also strange, maybe we leave the border in place, but we don't put the border under the unified search bar in edit mode, and only show the one under the editor? There was a problem hiding this comment. @ThomThomson you mean like this with only the bottom bar? @MichaelMarcialis What do you think? only bottom border
borders between every bar
There was a problem hiding this comment. Yeah I would say that looks better. The whole pinned section gets one border at the bottom! What do you think? There was a problem hiding this comment. Yeah the extra lines do look pretty busy, but without the border between the two bars, the spacing looks a little tall compared to the other padding on the page, so I think I'll also reduce some padding like this:
There was a problem hiding this comment. 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! |
||
|
|
||
| const loadingSpinner = showPlainSpinner ? ( | ||
| <EuiLoadingSpinner size="xxl" /> | ||
| ) : ( | ||
| <EuiLoadingElastic size="xxl" /> | ||
| ); | ||
| return ( | ||
| <div className={viewportClasses} css={viewportStyles}> | ||
|
There was a problem hiding this comment. Not sure if related to this change. But the open context menu on panels does not scroll with the dashboard anymore. Adding There was a problem hiding this comment. Oh that's a handy prop! I added the prop to the popover in the maps embeddable tooltip too, and it fixed the scrolling issue there as well.
|
||
| {loading ? loadingSpinner : <div ref={dashboardRoot} />} | ||
| </div> | ||
| ); | ||
| }; | ||
|
|
||
|
|
||
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.
Excluding the
kibanaFullBodyHeightmixin from thesbreakpoint appears to be causing dueling scrollbars to appear at small viewport sizes. Fixed in f1ad633.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.
Oh nice catch! I added a
xstoo because I was still seeing the dual scrollbar when I shrank the viewport width even more.