-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[Workspace Chrome] Visual Improvements #245898
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
Changes from all commits
895297a
0bfa71b
83e6eeb
b8e6a29
e4e0614
22dc473
4c4cc79
f71e601
74e38bf
dafdfab
87a5eb5
5b7efdc
1a6020a
f5ed9bc
969feca
73bf384
c74a4ef
2c21bd5
dd95750
2c74db9
2ddcd30
bebe613
0e35efe
422da88
a7b1161
9e5eef9
e3f9afe
de57b6a
9318d59
1f72d79
deb9a03
710a7ac
bd5046a
3de7124
8f25428
5c00e7f
d50a999
2e8c2db
c860c47
089d33d
8c243a0
a8ff786
f43f7c9
43d2032
8da615e
6821f5f
0019278
9055049
597be7e
abad092
09df9e3
b2141d5
8dd0ccb
b2ad97c
00ec3c2
9575bf8
e195d2b
ce6808e
1414b3a
029a5a6
228b101
be8818f
17fcc22
ea90c88
d768791
72ff0ea
923aa9e
a0d6bcb
0425d6a
f06ac16
2730003
199bad8
b546d5f
b57f40e
42188ef
0da4edf
c939a8d
81174c6
1ba7cd5
fac3f73
f660e81
11aa4b0
3732fad
c0ea163
24bce89
2f87e08
a4fb4dc
9a5d3c0
507a986
9a6283e
5c61c0c
daccacc
ea557e3
0e2a0c8
8b4ac6d
7bd1954
7a8365a
27fb696
49a2000
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,21 +9,45 @@ | |
|
|
||
| import { css } from '@emotion/react'; | ||
| import { layoutVar, layoutLevels } from '@kbn/core-chrome-layout-constants'; | ||
| import { euiOverflowScroll, euiShadow } from '@elastic/eui'; | ||
| import { getHighContrastBorder } from '@kbn/core-chrome-layout-utils'; | ||
| import type { EmotionFn } from '../types'; | ||
|
|
||
| const root: EmotionFn = ({ euiTheme }) => | ||
| const root: EmotionFn = (useEuiTheme) => | ||
| css` | ||
| grid-area: application; | ||
| height: 100%; | ||
| position: relative; | ||
| width: 100%; | ||
|
|
||
| height: calc(100% - ${layoutVar('application.marginBottom')}); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the original CSS might have been redundant, and explicit height and width might not be needed here at all. I'll create an issue for myself to double check this if can be simplified |
||
| width: calc(100% - ${layoutVar('application.marginRight')}); | ||
| margin-bottom: ${layoutVar('application.marginBottom')}; | ||
| margin-right: ${layoutVar('application.marginRight')}; | ||
|
|
||
| z-index: ${layoutLevels.content}; | ||
|
|
||
| position: relative; | ||
| display: flex; | ||
| flex-direction: column; | ||
|
|
||
| background-color: ${useEuiTheme.euiTheme.colors.backgroundBasePlain}; | ||
| border-radius: ${useEuiTheme.euiTheme.border.radius.medium}; | ||
| border: ${getHighContrastBorder(useEuiTheme)}; | ||
| ${euiShadow(useEuiTheme, 'xs', { border: 'none' })}; | ||
|
|
||
| &:focus-visible { | ||
| border: 2px solid ${euiTheme.colors.textParagraph}; | ||
| border: 2px solid ${useEuiTheme.euiTheme.colors.textParagraph}; | ||
| } | ||
|
|
||
| // only restrict overflow scroll on screen (not print) to allow for full page printing | ||
| @media screen { | ||
| ${euiOverflowScroll(useEuiTheme, { direction: 'y' })}; | ||
| // reset the height back to respect the margin bottom | ||
| height: calc(100% - ${layoutVar('application.marginBottom')}); | ||
|
|
||
| // Hide scrollbar | ||
| scrollbar-width: none; /* Firefox */ | ||
| &::-webkit-scrollbar { | ||
| display: none; /* Chrome, Safari, Edge */ | ||
| } | ||
| } | ||
| `; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,6 +26,8 @@ export const LayoutGlobalCSS = () => { | |
| sidebarWidth, | ||
| applicationTopBarHeight, | ||
| applicationBottomBarHeight, | ||
| applicationMarginBottom, | ||
| applicationMarginRight, | ||
| } = useLayoutState(); | ||
|
|
||
| const banner = css` | ||
|
|
@@ -80,10 +82,17 @@ export const LayoutGlobalCSS = () => { | |
| `; | ||
|
|
||
| const application = css` | ||
| ${layoutVarName('application.marginBottom')}: ${applicationMarginBottom}px; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: should we go ahead and add margin top and left as 0, for completeness and flexibility?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wouldn't add it until it's actually needed. |
||
| ${layoutVarName('application.marginRight')}: ${applicationMarginRight}px; | ||
|
|
||
| ${layoutVarName('application.top')}: ${bannerHeight + headerHeight}px; | ||
| ${layoutVarName('application.bottom')}: ${layoutVar('footer.height')}; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TODO for myself: we should refactor these variables to use static calculation as much as possible instead of dynamic |
||
| ${layoutVarName('application.bottom')}: calc(${layoutVar('footer.height')} + ${layoutVar( | ||
| 'application.marginBottom' | ||
| )}); | ||
| ${layoutVarName('application.left')}: ${navigationWidth}px; | ||
| ${layoutVarName('application.right')}: ${sidebarWidth}px; | ||
| ${layoutVarName('application.right')}: calc(${layoutVar( | ||
| 'application.marginRight' | ||
| )} + ${sidebarWidth}px); | ||
| ${layoutVarName('application.height')}: calc( | ||
| 100vh - ${layoutVar('application.top')} - ${layoutVar('application.bottom')} | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,5 +21,6 @@ | |
| ], | ||
| "kbn_references": [ | ||
| "@kbn/core-chrome-layout-constants", | ||
| "@kbn/core-chrome-layout-utils", | ||
| ] | ||
| } | ||
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.
is the change redundant?
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.
Without this change, the top bar would have a white background (light mode)