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

Implement system option for theme:darkMode uiSetting #173044

Merged
merged 33 commits into from
Feb 20, 2024
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
243d36c
implement POC
pgayvallet Dec 11, 2023
6af5e73
cleanup the POC
pgayvallet Dec 11, 2023
03e61fc
[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…
kibanamachine Dec 11, 2023
b0a0017
update euiThemeVars dynamically
pgayvallet Dec 11, 2023
aa16ab2
update __kbnThemeTag__ too
pgayvallet Dec 11, 2023
f65a833
start adapting tests
pgayvallet Dec 11, 2023
c8cead5
[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…
kibanamachine Dec 11, 2023
0d580a6
improve the thing
pgayvallet Dec 12, 2023
600c942
fix theme for loading screen and legacy static css
pgayvallet Dec 12, 2023
87fb6db
remove unused import
pgayvallet Dec 12, 2023
f46a24a
fix overview component unit tests
pgayvallet Dec 12, 2023
9f21ff0
fix more tests and violations
pgayvallet Dec 12, 2023
3ea35d3
I think we're pretty good now?
pgayvallet Dec 12, 2023
292510a
fix user profile FTR tests
pgayvallet Dec 12, 2023
aeaabcb
theme service tests
pgayvallet Dec 12, 2023
57f52f2
rendering service tests
pgayvallet Dec 12, 2023
dfb1645
start adding tests, extract utilities
pgayvallet Dec 12, 2023
370796f
moar tests
pgayvallet Dec 12, 2023
56c58b5
more tests and cleanup
pgayvallet Dec 12, 2023
ff22176
Merge remote-tracking branch 'upstream/main' into kbn-89340-system-da…
pgayvallet Dec 12, 2023
1b5cb27
disable dynamic reloading
pgayvallet Dec 12, 2023
2b75535
Merge remote-tracking branch 'upstream/main' into kbn-89340-system-da…
pgayvallet Dec 13, 2023
a684d24
Merge remote-tracking branch 'upstream/main' into kbn-89340-system-da…
pgayvallet Dec 18, 2023
cc9fde6
update doc
pgayvallet Dec 18, 2023
8e3b644
Merge remote-tracking branch 'upstream/main' into kbn-89340-system-da…
pgayvallet Feb 20, 2024
102c6f8
Adding missing changes
pgayvallet Feb 20, 2024
344e1c0
fix tests
pgayvallet Feb 20, 2024
ee143d2
adapt cloud_links darkMode usage
pgayvallet Feb 20, 2024
1572925
adapt remaining direct accessors of theme:darkMode
pgayvallet Feb 20, 2024
b8163ae
adapt remaining direct accessors of theme:darkMode
pgayvallet Feb 20, 2024
e4822ee
Merge remote-tracking branch 'upstream/main' into kbn-89340-system-da…
pgayvallet Feb 20, 2024
7d0eefc
fixing snapshot
pgayvallet Feb 20, 2024
26f02dc
update doc
pgayvallet Feb 20, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions docs/management/advanced-options.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -184,8 +184,10 @@ this setting stores part of the URL in your browser session to keep the URL
short.

[[theme-darkmode]]`theme:darkMode`::
Set to `true` to enable a dark mode for the {kib} UI. You must refresh the page
to apply the setting.
The UI theme that the {kib} UI should use.
Set to `true` or `false` to enable or disable the dark theme.
Set to `system` to have the {kib} UI theme follow the system theme.
You must refresh the page to apply the setting.

[[theme-version]]`theme:version`::
Kibana only ships with the v8 theme now, so this setting can no longer be edited.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
*, *:before, *:after {
box-sizing: border-box;
}

html, body, div, span, svg {
margin: 0;
Comment on lines +1 to +6
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I extracted the static inline styles (from packages/core/rendering/core-rendering-server-internal/src/views/styles.tsx), and added them in a css file instead of having them inlined in the template. Better isolation of concern, and now it's cache-able.

padding: 0;
border: none;
vertical-align: baseline;
}

body, html {
width: 100%;
height: 100%;
margin: 0;
display: block;
}

.kbnWelcomeView {
line-height: 1.5;
height: 100%;
display: -webkit-box;
display: -webkit-flex;
display: -ms-flexbox;
display: flex;
-webkit-box-flex: 1;
-webkit-flex: 1 0 auto;
-ms-flex: 1 0 auto;
flex: 1 0 auto;
-webkit-box-orient: vertical;
-webkit-box-direction: normal;
-webkit-flex-direction: column;
-ms-flex-direction: column;
flex-direction: column;
-webkit-box-align: center;
-webkit-align-items: center;
-ms-flex-align: center;
align-items: center;
-webkit-box-pack: center;
-webkit-justify-content: center;
-ms-flex-pack: center;
justify-content: center;
}

.kbnWelcomeTitle {
color: #000;
font-size: 20px;
font-family: sans-serif;
margin: 16px 0;
animation: fadeIn 1s ease-in-out;
animation-fill-mode: forwards;
opacity: 0;
animation-delay: 1.0s;
}

.kbnWelcomeText {
display: block;
font-size: 14px;
font-family: sans-serif;
line-height: 40px !important;
height: 40px !important;
color: #98A2B3;
}

.kbnLoaderWrap {
text-align: center;
line-height: 1;
font-family: sans-serif;
letter-spacing: -.005em;
-webkit-text-size-adjust: 100%;
-ms-text-size-adjust: 100%;
font-kerning: normal;
font-weight: 400;
}

.kbnLoaderWrap svg {
width: 64px;
height: 64px;
margin: auto;
line-height: 1;
}

.kbnLoader path {
stroke: #FFFFFF;
}

.kbnProgress {
display: inline-block;
position: relative;
width: 32px;
height: 4px;
overflow: hidden;
line-height: 1;
}

.kbnProgress:before {
position: absolute;
content: '';
height: 4px;
width: 100%;
top: 0;
bottom: 0;
left: 0;
transform: scaleX(0) translateX(0%);
animation: kbnProgress 1s cubic-bezier(.694, .0482, .335, 1) infinite;
}

@keyframes kbnProgress {
0% {
transform: scaleX(1) translateX(-100%);
}

100% {
transform: scaleX(1) translateX(100%);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

/* eslint-disable no-var */

function systemIsDark() {
try {
return window.matchMedia('(prefers-color-scheme: dark)').matches;
} catch (e) {
return false;
}
}

function createInlineStyles(content) {
var head = document.getElementsByTagName('head')[0];
var style = document.createElement('style');
style.textContent = content;
head.appendChild(style);
}

// must be kept in sync with
// packages/core/rendering/core-rendering-server-internal/src/views/styles.tsx

var lightStyles = [
'html { background-color: #F8FAFD; }',
'.kbnWelcomeText { color: #69707D; }',
'.kbnProgress { background-color: #F5F7FA; }',
'.kbnProgress:before { background-color: #006DE4; }',
].join('\n');

var darkStyles = [
'html { background-color: #141519; }',
'.kbnWelcomeText { color: #98A2B3; }',
'.kbnProgress { background-color: #25262E; }',
'.kbnProgress:before { background-color: #1BA9F5; }',
].join('\n');

if (systemIsDark()) {
createInlineStyles(darkStyles);
} else {
createInlineStyles(lightStyles);
}
Comment on lines +43 to +47
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this was required to work around point 4. Static inline styles from the issue's description.

  • we need those theme-dependent styles as soon as the loading page
  • Core isn't fully up yet, and doing that logic in the bootstrap script is also too late
  • Because of the CSP config, I can't inline a script in the template
  • Meaning that I can't know, from this script, what the value of the darkMode setting is.

So the only solution I found was:

  • if darkMode is true or false
    • then keep the inline styles in the template
    • do not inject that script
  • if darkMode is system
    • do not inject the line styles in the template
    • inject that script

I would have loved to have a single place to do that, but until we properly implement an nonce- mechanism (#93785) to allow executing inline script from the page template, this is the only viable approach

Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,12 @@
* Side Public License, v 1.
*/

import { ThemeVersion } from '@kbn/ui-shared-deps-npm';
import {
InjectedMetadata,
InjectedMetadataClusterInfo,
InjectedMetadataExternalUrlPolicy,
InjectedMetadataPlugin,
InjectedMetadataTheme,
} from '@kbn/core-injected-metadata-common-internal';
import type { CustomBranding } from '@kbn/core-custom-branding-common';

Expand Down Expand Up @@ -39,10 +39,7 @@ export interface InternalInjectedMetadataSetup {
getExternalUrlConfig: () => {
policy: InjectedMetadataExternalUrlPolicy[];
};
getTheme: () => {
darkMode: boolean;
version: ThemeVersion;
};
getTheme: () => InjectedMetadataTheme;
getElasticsearchInfo: () => InjectedMetadataClusterInfo;
/**
* An array of frontend plugins in topological order.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
],
"kbn_references": [
"@kbn/std",
"@kbn/ui-shared-deps-npm",
"@kbn/core-base-common",
"@kbn/core-injected-metadata-common-internal",
"@kbn/core-custom-branding-common",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,14 @@ const createSetupContractMock = () => {
},
} as any);
setupContract.getPlugins.mockReturnValue([]);
setupContract.getTheme.mockReturnValue({ darkMode: false, version: 'v8' });
setupContract.getTheme.mockReturnValue({
darkMode: false,
version: 'v8',
stylesheetPaths: {
default: ['light-1.css'],
dark: ['dark-1.css'],
},
});
return setupContract;
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,6 @@ export type {
InjectedMetadata,
InjectedMetadataClusterInfo,
InjectedMetadataExternalUrlPolicy,
InjectedMetadataTheme,
InjectedMetadataPlugin,
} from './src/types';
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import type { PluginName, DiscoveredPlugin } from '@kbn/core-base-common';
import type { ThemeVersion } from '@kbn/ui-shared-deps-npm';
import type { EnvironmentMode, PackageInfo } from '@kbn/config';
import type { CustomBranding } from '@kbn/core-custom-branding-common';
import type { DarkModeValue } from '@kbn/core-ui-settings-common';

/** @internal */
export interface InjectedMetadataClusterInfo {
Expand All @@ -35,6 +36,16 @@ export interface InjectedMetadataExternalUrlPolicy {
protocol?: string;
}

/** @internal */
export interface InjectedMetadataTheme {
darkMode: DarkModeValue;
version: ThemeVersion;
stylesheetPaths: {
default: string[];
dark: string[];
};
}

/** @internal */
export interface InjectedMetadata {
version: string;
Expand All @@ -53,10 +64,7 @@ export interface InjectedMetadata {
i18n: {
translationsUrl: string;
};
theme: {
darkMode: boolean;
version: ThemeVersion;
};
theme: InjectedMetadataTheme;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess InjectedMetadata will stay with us for a while longer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but fwiw this PR isn't introducing anything new that wasn't already in the list of things we need to figure out to get rid of this injection system.

csp: {
warnLegacyBrowsers: boolean;
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@
"@kbn/config",
"@kbn/ui-shared-deps-npm",
"@kbn/core-base-common",
"@kbn/core-custom-branding-common"
"@kbn/core-custom-branding-common",
"@kbn/core-ui-settings-common"
],
"exclude": [
"target/**/*",
Expand Down
Loading