Account for presence of top banner when EuiDataGrid and Canvas are full screen #111038

MichaelMarcialis · 2021-09-02T20:25:01Z

Summary

This PR makes style changes to correctly account for the presence of a Kibana top banner when either the EuiDataGrid component or the Canvas app are in full screen mode. Previously, the contents of these full screen items would fall behind and become hidden under the top banner. Some notes regarding these two changes:

Use of .euiDataGrid--fullScreen selector: I know using EUI classes in Kibana stylesheets is frowned upon, but I didn't know of any other way that we could apply these top banner offset styles globally to the component. If there is a better way to do it, please let me know. CCing @cchaos.
Canvas conditional styles: Canvas appears to have a fairly robust set of custom styles, and I didn't want to go too far down the rabbit hole of suggesting too many changes. As such, I've made a small handful of conditional changes to the scaling and positioning styles when in full screen mode and a top banner is present. If what I've done feels a little too hacky, please let me know and I'd be happy to tweak as necessary. CCing @elastic/kibana-presentation.

EuiDataGrid full screen before:

EuiDataGrid full screen after:

Canvas full screen before:

Canvas full screen after:

Closes #109554

Checklist

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)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

…nner-full-screen-fixes

elasticmachine · 2021-09-02T20:25:03Z

Pinging @elastic/kibana-core (Team:Core)

elasticmachine · 2021-09-02T20:25:03Z

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

src/core/public/rendering/_base.scss

pgayvallet · 2021-09-02T21:14:37Z

x-pack/plugins/canvas/public/components/workpad/workpad.js

+              const hasHeaderBanner = document.getElementsByClassName('kbnBody--hasHeaderBanner')
+                .length;
+
+              const headerBannerOffset = hasHeaderBanner ? HEADER_BANNER_HEIGHT : 0;


Sorry, when you asked me how to check if a banner was present in javascript, I thought it was for testing, not for actually using it in a plugin's code.

The proper way with Kibana API would be to add a new hasHeaderBanner API, and have the plugin leverage this info.

kibana/src/core/public/chrome/chrome_service.tsx

Lines 272 to 274 in e88d77b

setHeaderBanner: (headerBanner?: ChromeUserBanner) => {

headerBanner$.next(headerBanner);

},

Something like

hasHeaderBanner$: () => { return headerBanner$.pipe(map((banner) => banner !== undefined)) },

How urgent is this? I could have a PR ready tomorrow to add the API to coreStart.chrome. Not sure how hard to would be to cascade this info from canvas mountpoint to this component?

Thanks for the help! Not super urgent in my opinion. Just let me know what I'd need to do on my end when you're ready.

I created #111248, I'll ping you once it's merged.

#111248 has been merged and backported to 7.16 and 7.15. You should now be able to use the coreStart.chrome.hasHeaderBanner$ API.

Now, about how to retrieve this API down in the Workpad component. I took a quick look, but given the multiple layers of composition of the component (e.g x-pack/plugins/canvas/public/components/workpad/index.js) and the fact that it's still plain javascript, I'm not comfortable performing the change myself in your PR, and think that maybe the owner team should be the one to do it.

I see the canvas app is wrapping their root tree with the Kibana context provider

kibana/x-pack/plugins/canvas/public/application.tsx

Lines 73 to 74 in 303526f

ReactDOM.render(

<KibanaContextProvider services={{ ...startPlugins, ...coreStart }}>

So in theory the chrome core service should be accessible at any depth, using either the useKibana hook or the withKibana HOC.

So, it may be as easy as using the HOC in x-pack/plugins/canvas/public/components/workpad/workpad.js, however I'm not sure about the best practices, like maybe the team prefers to add this wrapper in the composition performed in x-pack/plugins/canvas/public/components/workpad/index.js, or even from elsewhere.

cc @elastic/kibana-presentation I think we need your help here.

…nner-full-screen-fixes

cchaos · 2021-09-08T18:45:59Z

x-pack/plugins/canvas/common/lib/constants.ts

@@ -45,3 +45,4 @@ export const CANVAS_EMBEDDABLE_CLASSNAME = `canvasEmbeddable`;
 export const CONTEXT_MENU_TOP_BORDER_CLASSNAME = 'canvasContextMenu--topBorder';
 export const API_ROUTE_FUNCTIONS = `${API_ROUTE}/fns`;
 export const ESSQL_SEARCH_STRATEGY = 'essql';
+export const HEADER_BANNER_HEIGHT = 32;


Suggestion: This is number can easily get out sync between the JS and Sass versions. I'd highly suggest moving it a constant that lives within the actual banner service and imported by Canvas. Then in both places (JS & CSS), add a comment that mentions where it's counterpart lives. Something like:

Suggested change

export const HEADER_BANNER_HEIGHT = 32;

export const HEADER_BANNER_HEIGHT = 32; // This value also declared in Sass file `.../path/to/_bases.css`

@pgayvallet: Thoughts on Caroline's suggestion above to move the banner height to live in the banner service? Sounds good to me, but would likely need your guidance on how best to implement.

It definitely makes sense, but I think this can be done by my team as a follow-up (if that's fine with you). I created #111688

Sounds good. In the mean time, I'll add those suggested comments in the current variable locations.

Use hasHeaderBanner$

…nner-full-screen-fixes

cchaos

Code LGTM, sorry I didn't pull down to test locally, but I trust your screenshots. Thanks for taking care of this one from EUI to here. 💯

poffdeluxe

Canvas changes LGTM

MichaelMarcialis · 2021-09-13T13:27:34Z

@elasticmachine merge upstream

kibanamachine · 2021-09-13T15:42:04Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`canvas`	1069	1071	+2

Async chunks

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

id	before	after	diff
`canvas`	1.6MB	1.6MB	+1.2KB

Page load bundle

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

id	before	after	diff
`canvas`	29.9KB	30.1KB	+229.0B
`core`	407.7KB	408.1KB	+376.0B
total			+605.0B

History

💚 Build #152318 succeeded 7b0e729
💔 Build #152043 failed 734c1f5
💔 Build #151967 failed 0efa8e4
💚 Build #150969 succeeded aa70475
💚 Build #150735 succeeded 710fdaf

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

…ll screen (elastic#111038) * account for banners when data grid is full screen * account for banner when canvas is full screen * change height per feedback * add withKibana * rm withKibana; move vars out of Fullscreen * Use hasHeaderBanner$ * add banner height var comments * fix ts error Co-authored-by: Catherine Liu <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

kibanamachine · 2021-09-13T16:01:19Z

💚 Backport successful

Status	Branch	Result
✅	7.15
✅	7.x

The backport PRs will be merged automatically after passing CI.

…ll screen (#111038) (#111981) * account for banners when data grid is full screen * account for banner when canvas is full screen * change height per feedback * add withKibana * rm withKibana; move vars out of Fullscreen * Use hasHeaderBanner$ * add banner height var comments * fix ts error Co-authored-by: Catherine Liu <[email protected]> Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Michael Marcialis <[email protected]> Co-authored-by: Catherine Liu <[email protected]>

…ll screen (#111038) (#111980) * account for banners when data grid is full screen * account for banner when canvas is full screen * change height per feedback * add withKibana * rm withKibana; move vars out of Fullscreen * Use hasHeaderBanner$ * add banner height var comments * fix ts error Co-authored-by: Catherine Liu <[email protected]> Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Michael Marcialis <[email protected]> Co-authored-by: Catherine Liu <[email protected]>

MichaelMarcialis added 3 commits September 2, 2021 14:31

account for banners when data grid is full screen

b56f13a

account for banner when canvas is full screen

b23599a

Merge branch 'master' of github.com:elastic/kibana into 109554/top-ba…

710fdaf

…nner-full-screen-fixes

MichaelMarcialis requested review from a team as code owners September 2, 2021 20:25

MichaelMarcialis added impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort labels Sep 2, 2021

cchaos reviewed Sep 2, 2021

View reviewed changes

src/core/public/rendering/_base.scss Outdated Show resolved Hide resolved

pgayvallet reviewed Sep 2, 2021

View reviewed changes

MichaelMarcialis added 2 commits September 3, 2021 09:49

change height per feedback

860ea58

Merge branch 'master' of github.com:elastic/kibana into 109554/top-ba…

aa70475

…nner-full-screen-fixes

pgayvallet mentioned this pull request Sep 6, 2021

add hasHeaderBanner$ API to chrome #111248

Merged

2 tasks

MichaelMarcialis added 3 commits September 8, 2021 10:54

add withKibana

c0b0206

Merge branch 'master' of github.com:elastic/kibana into 109554/top-ba…

63a7df2

…nner-full-screen-fixes

rm withKibana; move vars out of Fullscreen

0efa8e4

cchaos reviewed Sep 8, 2021

View reviewed changes

Use hasHeaderBanner$

554aafe

Merge pull request #2 from cqliu1/pr/111038

734c1f5

Use hasHeaderBanner$

pgayvallet mentioned this pull request Sep 9, 2021

Expose an API from chrome service to access the height of the top banner #111688

Open

MichaelMarcialis added 3 commits September 9, 2021 12:08

add banner height var comments

128872d

Merge branch 'master' of github.com:elastic/kibana into 109554/top-ba…

b7abcb1

…nner-full-screen-fixes

fix ts error

7b0e729

cchaos approved these changes Sep 9, 2021

View reviewed changes

poffdeluxe approved these changes Sep 10, 2021

View reviewed changes

Merge branch 'master' into 109554/top-banner-full-screen-fixes

5e21b25

MichaelMarcialis merged commit 4dc7214 into elastic:master Sep 13, 2021

MichaelMarcialis deleted the 109554/top-banner-full-screen-fixes branch September 13, 2021 15:58

kibanamachine mentioned this pull request Sep 13, 2021

[7.15] Account for presence of top banner when EuiDataGrid and Canvas are full screen (#111038) #111980

Merged

kibanamachine mentioned this pull request Sep 13, 2021

[7.x] Account for presence of top banner when EuiDataGrid and Canvas are full screen (#111038) #111981

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Account for presence of top banner when EuiDataGrid and Canvas are full screen #111038

Account for presence of top banner when EuiDataGrid and Canvas are full screen #111038

MichaelMarcialis commented Sep 2, 2021 •

edited

Loading

elasticmachine commented Sep 2, 2021

elasticmachine commented Sep 2, 2021

pgayvallet Sep 2, 2021

MichaelMarcialis Sep 3, 2021

pgayvallet Sep 6, 2021

pgayvallet Sep 8, 2021

cchaos Sep 8, 2021

MichaelMarcialis Sep 8, 2021

pgayvallet Sep 9, 2021 •

edited

Loading

MichaelMarcialis Sep 9, 2021

cchaos left a comment

poffdeluxe left a comment

MichaelMarcialis commented Sep 13, 2021

kibanamachine commented Sep 13, 2021

kibanamachine commented Sep 13, 2021

	setHeaderBanner: (headerBanner?: ChromeUserBanner) => {
	headerBanner$.next(headerBanner);
	},

	ReactDOM.render(
	<KibanaContextProvider services={{ ...startPlugins, ...coreStart }}>

	export const HEADER_BANNER_HEIGHT = 32;
	export const HEADER_BANNER_HEIGHT = 32; // This value also declared in Sass file `.../path/to/_bases.css`

Account for presence of top banner when EuiDataGrid and Canvas are full screen #111038

Account for presence of top banner when EuiDataGrid and Canvas are full screen #111038

Conversation

MichaelMarcialis commented Sep 2, 2021 • edited Loading

Summary

Checklist

elasticmachine commented Sep 2, 2021

elasticmachine commented Sep 2, 2021

pgayvallet Sep 2, 2021

Choose a reason for hiding this comment

MichaelMarcialis Sep 3, 2021

Choose a reason for hiding this comment

pgayvallet Sep 6, 2021

Choose a reason for hiding this comment

pgayvallet Sep 8, 2021

Choose a reason for hiding this comment

cchaos Sep 8, 2021

Choose a reason for hiding this comment

MichaelMarcialis Sep 8, 2021

Choose a reason for hiding this comment

pgayvallet Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

MichaelMarcialis Sep 9, 2021

Choose a reason for hiding this comment

cchaos left a comment

Choose a reason for hiding this comment

poffdeluxe left a comment

Choose a reason for hiding this comment

MichaelMarcialis commented Sep 13, 2021

kibanamachine commented Sep 13, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

History

kibanamachine commented Sep 13, 2021

💚 Backport successful

MichaelMarcialis commented Sep 2, 2021 •

edited

Loading

pgayvallet Sep 9, 2021 •

edited

Loading