-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Make Borealis the default theme in non-serverless #203840
Changes from all commits
4ce8481
46f458c
3029b62
3bfb2ab
a453893
2daee71
5e53377
2068665
22d2278
d12cbe3
a43718e
59bd030
1d6af69
8147e95
4aeb2f9
1cae9e6
e163a74
4558f92
23d2063
177831c
630c79f
088bb12
1f638bb
c0b02f7
bdbb321
8582a46
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 |
---|---|---|
|
@@ -10,7 +10,11 @@ | |
import { createHash } from 'crypto'; | ||
import { PackageInfo } from '@kbn/config'; | ||
import type { KibanaRequest, HttpAuth } from '@kbn/core-http-server'; | ||
import { type DarkModeValue, parseDarkModeValue } from '@kbn/core-ui-settings-common'; | ||
import { | ||
type DarkModeValue, | ||
DEFAULT_THEME_NAME, | ||
parseDarkModeValue, | ||
} from '@kbn/core-ui-settings-common'; | ||
import type { IUiSettingsClient } from '@kbn/core-ui-settings-server'; | ||
import type { UiPlugins } from '@kbn/core-plugins-base-server-internal'; | ||
import { InternalUserSettingsServiceSetup } from '@kbn/core-user-settings-server-internal'; | ||
|
@@ -58,7 +62,11 @@ export const bootstrapRendererFactory: BootstrapRendererFactory = ({ | |
|
||
return async function bootstrapRenderer({ uiSettingsClient, request, isAnonymousPage = false }) { | ||
let darkMode: DarkModeValue = false; | ||
let themeName: string = 'amsterdam'; | ||
let themeName: string = DEFAULT_THEME_NAME; | ||
|
||
if (packageInfo.buildFlavor !== 'serverless') { | ||
themeName = 'borealis'; | ||
} | ||
Comment on lines
+67
to
+69
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 wonder if it is worth having this conditional also depend on some configuration setting... That way when the time comes we can rollout borealis incrementally to serverless. Not a blocker for this PR, but would be nice to hear your thoughts! 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 personally would want this to depend solely on a configuration setting. |
||
|
||
try { | ||
const authenticated = isAuthenticated(request); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,13 +16,14 @@ import { getThemeSettings } from './theme'; | |
import { getStateSettings } from './state'; | ||
import { getAnnouncementsSettings } from './announcements'; | ||
|
||
interface GetCoreSettingsOptions { | ||
isDist?: boolean; | ||
isThemeSwitcherEnabled?: boolean; | ||
export interface GetCoreSettingsOptions { | ||
isServerless: boolean; | ||
isDist: boolean; | ||
isThemeSwitcherEnabled: boolean | undefined; | ||
} | ||
|
||
export const getCoreSettings = ( | ||
options?: GetCoreSettingsOptions | ||
options: GetCoreSettingsOptions | ||
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. Is there a reason this was switched to required? 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. It has only one usage, and I don't see why it would be optional if it stores important data like |
||
): Record<string, UiSettingsParams> => { | ||
return { | ||
...getAccessibilitySettings(), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,32 +12,54 @@ import { i18n } from '@kbn/i18n'; | |
import type { ThemeVersion } from '@kbn/ui-shared-deps-npm'; | ||
import { | ||
type UiSettingsParams, | ||
type ThemeName, | ||
parseThemeTags, | ||
SUPPORTED_THEME_NAMES, | ||
DEFAULT_THEME_NAME, | ||
} from '@kbn/core-ui-settings-common'; | ||
|
||
function getThemeInfo(options: GetThemeSettingsOptions) { | ||
if (options?.isDist ?? true) { | ||
return { | ||
defaultDarkMode: false, | ||
}; | ||
} | ||
interface ThemeInfo { | ||
defaultDarkMode: boolean; | ||
defaultThemeName: ThemeName; | ||
} | ||
|
||
const getThemeInfo = ({ isDist, isServerless }: GetThemeSettingsOptions): ThemeInfo => { | ||
const themeTags = parseThemeTags(process.env.KBN_OPTIMIZER_THEMES); | ||
return { | ||
defaultDarkMode: themeTags[0].endsWith('dark'), | ||
|
||
const themeInfo: ThemeInfo = { | ||
defaultDarkMode: false, | ||
defaultThemeName: DEFAULT_THEME_NAME, | ||
}; | ||
} | ||
|
||
interface GetThemeSettingsOptions { | ||
isDist?: boolean; | ||
isThemeSwitcherEnabled?: boolean; | ||
if (!isDist) { | ||
// Allow environment-specific config when not building for distribution | ||
themeInfo.defaultDarkMode = themeTags[0]?.endsWith('dark') || false; | ||
} | ||
|
||
if (!isServerless) { | ||
// Default to Borealis theme in non-serverless | ||
themeInfo.defaultThemeName = 'borealis'; | ||
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. This feels brittle, depending on whether or not we're in serverless to default the theme. I feel like this shouldn't be determined here. |
||
} | ||
|
||
return themeInfo; | ||
}; | ||
|
||
export interface GetThemeSettingsOptions { | ||
isServerless: boolean; | ||
isDist: boolean; | ||
isThemeSwitcherEnabled: boolean | undefined; | ||
} | ||
|
||
export const getThemeSettings = ( | ||
options: GetThemeSettingsOptions = {} | ||
options: GetThemeSettingsOptions | ||
): Record<string, UiSettingsParams> => { | ||
const { defaultDarkMode } = getThemeInfo(options); | ||
const { defaultDarkMode, defaultThemeName } = getThemeInfo(options); | ||
|
||
// Make `theme:name` readonly in serverless unless the theme switcher is enabled | ||
let isThemeNameReadonly = options.isServerless; | ||
if (options.isThemeSwitcherEnabled !== undefined) { | ||
isThemeNameReadonly = !options.isThemeSwitcherEnabled; | ||
} | ||
|
||
return { | ||
'theme:darkMode': { | ||
|
@@ -109,10 +131,8 @@ export const getThemeSettings = ( | |
defaultMessage: 'Borealis', | ||
}), | ||
}, | ||
value: 'amsterdam', | ||
readonly: Object.hasOwn(options, 'isThemeSwitcherEnabled') | ||
? !options.isThemeSwitcherEnabled | ||
: true, | ||
value: defaultThemeName, | ||
readonly: isThemeNameReadonly, | ||
requiresPageReload: true, | ||
schema: schema.oneOf([ | ||
schema.literal('amsterdam'), | ||
|
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.
nit: why not
v9light
...?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.
@JasonStoltz We didn't use
Amsterdam
for v8, perhaps we should usev9
here...?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.
In the past, themes were pretty much directly related to Kibana major versions. Moving forward, we want themes to be more dynamic than that, meaning a theme may change over the lifespan of a single major, and there might be more than one theme available.