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

Feat (core): Make theme settings user-specific and user-configurable #5652

Merged
merged 34 commits into from
Apr 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
e3d494f
init ui
joshuarrrr Oct 2, 2023
b138518
Mock out new user menu panels
joshuarrrr Oct 10, 2023
b0af131
hook up to uisettings
joshuarrrr Oct 13, 2023
90fa4a6
Merge branch 'main' into feat/add-user-chrome
joshuarrrr Oct 13, 2023
5586192
audit theming
joshuarrrr Oct 14, 2023
da426c2
Merge branch 'main' into feat/add-user-chrome
joshuarrrr Oct 17, 2023
f073016
Merge branch 'opensearch-project:main' into feat/add-user-chrome
joshuarrrr Dec 12, 2023
f95c482
Merge branch 'main' into feat/add-user-chrome
joshuarrrr Dec 18, 2023
db5b415
revert changes to EuiIconType imports
joshuarrrr Dec 18, 2023
681afca
feat (core): make theme UI settings client-side
joshuarrrr Jan 2, 2024
eb5b2ae
Merge branch 'main' into feat/add-user-chrome
joshuarrrr Jan 2, 2024
a824708
update tests
joshuarrrr Jan 2, 2024
ee7a04c
- fix string/bool handling
joshuarrrr Jan 3, 2024
3290e4a
- update theme doc
joshuarrrr Jan 4, 2024
0f57b88
improve client and storage implemenation
joshuarrrr Jan 5, 2024
0586ae0
remove menu item from chrome dir
joshuarrrr Jan 5, 2024
d9dc09a
split dep update to separate PR
joshuarrrr Jan 5, 2024
f49765d
split ui-shared-deps imports to separate PR
joshuarrrr Jan 5, 2024
1782c6a
- remove console and debugger statements
joshuarrrr Jan 5, 2024
077424c
- update branding assignment and validation to be darkmode agnostic
joshuarrrr Jan 5, 2024
21a8740
rename button registration file
joshuarrrr Jan 5, 2024
c1987a9
consolidate bootstrap templates
joshuarrrr Jan 5, 2024
c424a21
resolve todos in rendering service
joshuarrrr Jan 5, 2024
7377ca9
todos
joshuarrrr Jan 5, 2024
8283561
update snapshots
joshuarrrr Jan 5, 2024
f56628a
add brower-default mode support
joshuarrrr Jan 6, 2024
abd6fb9
Merge branch 'main' into feat/add-user-chrome
joshuarrrr Feb 8, 2024
f0032cf
Merge branch 'main' into feat/add-user-chrome
joshuarrrr Apr 23, 2024
bea86f8
feat (theme): make new user theme controls opt-out
joshuarrrr Apr 26, 2024
97876e3
Merge branch 'main' into feat/add-user-chrome
joshuarrrr Apr 26, 2024
ac16643
fix: fix linting error
joshuarrrr Apr 26, 2024
1510505
fix: clean a couple last bugs
joshuarrrr Apr 26, 2024
7b71d55
test: fix custom branding functional test
joshuarrrr Apr 27, 2024
ca2a00e
test: fix functional tests
joshuarrrr Apr 29, 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- [Discover] Added customizable pagination options based on Discover UI settings [#5610](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5610)
- [PM] Enhance single version requirements imposed during bootstrapping ([#5675](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5675))
- [Custom Branding] Relative URL should be allowed for logos ([#5572](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5572))
- [Theme] Make theme and dark mode settings user/device specific (in local storage), with opt-out ([#5652](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5652))
- Revert to legacy discover table and add toggle to new discover table ([#5789](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5789))
- [Discover] Add collapsible and resizeable sidebar ([#5789](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5789))
- [Discover] Enhanced the data source selector with added sorting functionality ([#5719](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5719))
Expand Down
131 changes: 131 additions & 0 deletions docs/theme.md
Copy link
Member

Choose a reason for hiding this comment

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

I dont totally understand the theme system even after reading this, but i think that has less to do with your writing and more to do with how complex and confusing it is. Cannot tell you how much i appreciate this doc though.

Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
# Theme System

## Basic concepts

### Theme definitions in OUI

Themes are defined in OUI via https://github.com/opensearch-project/oui/blob/main/src/themes/themes.ts. When Building OUI, there are several theming artifacts generated (beyond the react components) for each mode (light/dark) of each theme:

1. Theme compiled stylesheets (e.g. `@elastic/eui/dist/eui_theme_dark.css`). Consumed as entry files in [/packages/osd-ui-shared-deps/webpack.config.js](/packages/osd-ui-shared-deps/webpack.config.js) and republished by `osd-ui-shared-deps` (e.g. [UiSharedDeps.darkCssDistFilename](/packages/osd-ui-shared-deps/index.js)).
2. Theme compiled and minified stylesheets (e.g. `@elastic/eui/dist/eui_theme_dark.min.css`). These appear unused by OpenSearch Dashboards
3. Theme computed SASS variables as JSON (e.g. `@elastic/eui/dist/eui_theme_dark.json`). Consumed by [/packages/osd-ui-shared-deps/theme.ts](/packages/osd-ui-shared-deps/theme.ts) and made available to other components via the mode and theme aware `euiThemeVars`. In general, these should not be consumed by any other component directly.
4. Theme type definition file for SASS variables as JSON (e.g. `@elastic/eui/dist/eui_theme_dark.json.d.ts`)

Note that all of these artifacts should ideally only be imported or used directly in one place (by `osd-ui-shared-deps`).

In addition to these artifacts, OpenSearch Dashboards also makes heavy use of the theme SASS variables and mixins as defined in the source files (e.g. `@elastic/eui/src/theme_dark.scss`).

### Theme definitions in OpenSearch Dashboards

1. Theme tags are defined in [/packages/osd-optimizer/src/common/theme_tags.ts](/packages/osd-optimizer/src/common/theme_tags.ts) corresponding to each mode (light/dark) of each OUI theme.
AMoo-Miki marked this conversation as resolved.
Show resolved Hide resolved
2. These tags must correspond to entrypoint SCSS files in [/src/core/public/core_app/styles/](/src/core/public/core_app/styles/_globals_v8dark.scss), because they are imported by all SCSS files as part of the `sass-loader` in [/packages/osd-optimizer/src/worker/webpack.config.ts](/packages/osd-optimizer/src/worker/webpack.config.ts) and [/packages/osd-optimizer/src/worker/theme_loader.ts](/packages/osd-optimizer/src/worker/theme_loader.ts). Note that the optimizer webpack will compile a separate stylesheet for each unique mode and theme combination.
3. OUI SCSS source files are also imported by `osd-ui-framework`, which generates the legacy KUI stylesheets (e.g. [/packages/osd-ui-framework/src/kui_next_dark.scss](/packages/osd-ui-framework/src/kui_next_dark.scss)). KUI is a UI library that predates EUI/OUI, and should be deprecated and fully removed via [#1060](https://github.com/opensearch-project/OpenSearch-Dashboards/issues/1060). Because it's a legacy package it has its own build process that doesn't use webpack; it just [compiles the SCSS files with grunt](/packages/osd-ui-framework/Gruntfile.js). But similarly to 2., a separate stylesheet is generated for each mode and theme combination.

### Thmemed assets in OpenSearch Dasboards

In general, most themed assests can be found in [/src/core/server/core_app/assets](src/core/server/core_app/assets/fonts/readme.md) (it also includes non-themed assets such as `favicons`, which could easily be themed if desired in the future).

Most of the graphics/images are only dark/light mode-specific, not theme-specific:

1. `default_branding` marks
2. `logos`

This directory also includes legacy CSS files ([/src/core/server/core_app/assets/legacy_dark_theme.css](/src/core/server/core_app/assets/legacy_dark_theme.css) and [/src/core/server/core_app/assets/legacy_light_theme.css](/src/core/server/core_app/assets/legacy_light_theme.css)), which predate even KUI, and are still used by some plugins (notably `discover`). See [#4385](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4385) for an experiment in removing these. Unlike KUI, they don't rely on OUI themes at all.

Finally, font assets are a bit of a special case. Theme-specific fonts are defined by OUI, but it doesn't include the font definitions directly. Instead, the font assets are in [/src/core/server/core_app/assets/fonts](/src/core/server/core_app/assets/fonts/readme.md). The corresponding `@font-face` style definitions are generated at runtime via [/src/core/server/rendering/views/fonts.tsx](/src/core/server/rendering/views/fonts.tsx).

## Theme settings

## Theme loading

```mermaid
sequenceDiagram
autonumber
critical Setup
core/server->>core/server/rendering: setup rendering service
core/server/rendering->>core/server: provide render() method
core/server->>core/server: setup legacy service
core/server->>legacy: create legacy server
legacy->>legacy: start ui mixin to<br>handle special routes
core/server->>core/server/core_app: setup core app
core/server/core_app->>core/server/core_app: register default routes
core/server/core_app->>core/server/core_app: register static asset dir routes
end
Browser->>core/server: OSD page request (e.g. /app/home#/ )
Copy link
Contributor

Choose a reason for hiding this comment

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

The # causes rendering to get cut off

2024-01-17 14_43_35-OpenSearch-Dashboards_docs_theme md at f56628a7e24051b22308c52e6fb64d7af78002c4

Copy link
Member Author

Choose a reason for hiding this comment

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

wow - well spotted 👁️! I'll update on the next revision.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@joshuarrrr would you have time to address this?

Copy link
Member

Choose a reason for hiding this comment

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

@AMoo-Miki i have created an issue for non-blockers to which i added this comment tagged for 2.13: #5831

core/server->>core/server/core_app: request to default route<br>(via `http` service)
core/server/core_app->>core/server: call renderCoreApp()
core/server->>core/server/rendering: call render()
critical Initial page bootstrap
core/server/rendering->>core/server/rendering: get theme settings from config
core/server/rendering->>core/server/rendering: assign branding values \<br>(including dark mode)
core/server/rendering->>Browser: return static loading page template
Note over core/server/rendering,Browser: includes inlined font-face styles and static loading page styles
critical <head> (render blocking)
Browser->>Browser: define injection points
Browser->>Browser: load static loading page styles
Browser->>Browser: load font-face styles
Browser->>legacy: load startup.js special route
legacy->>legacy: build startup.js from template
Note over legacy: inject theme settings and font sources
legacy->>Browser: startup.js
critical startup.js
Browser->>Browser: get theme preferences from local storage
Browser->>Browser: set global theme tag
Browser->>Browser: inject theme-specific loading page styles
Browser->>Browser: inject theme-specific font css vars
end
end
Browser->>Browser: render loading/error page<br>(with loaders hidden)
Browser->>legacy: load bootstrap.js special route
legacy->>legacy: build bootstrap.js from template
legacy->>Browser: bootstrap.js
critical bootstrap.js
Browser->>Browser: toggle visibility of errors/loaders
Browser->>Browser: get theme preferences from local storage
Browser->>core/server/core_app: load js bundles
core/server/core_app->>Browser: (React application)
Browser->>core/server/core_app: load theme-specific stylesheets<br>(base, OUI, KUI, legacy)
core/server/core_app->>Browser: themed css
end
end
```

### Loading

`src/legacy/ui/ui_render/ui_render_mixin.js` via `src/legacy/ui/ui_render/bootstrap/template.js.hbs` and `src/legacy/ui/ui_render/bootstrap/app_bootstrap.js`. Aliased in `src/legacy/ui/ui_mixin.js`, called by `src/legacy/server/osd_server.js`. Called by `src/core/server/legacy/legacy_service.ts` via `src/core/server/server.ts`

### Injected style tags

1. `src/core/server/rendering/views/styles.tsx` - depends on dark/light mode and injects style tag in head
2. `src/core/server/rendering/views/fonts.tsx` - depends on theme version and injects font style tag in head
3. Monaco editor styles
4. Ace styles
5. Ace TM overrides
6. Ace error styles
6. Component styles

### Styleshsheets loaded

Each of the following are loaded in the browser by the [bootstrap script](/src/legacy/ui/ui_render/bootstrap/template.js.hbs) in this order. Currently, these are never unloaded.

1. Monaco editor styles (e.g. [/packages/osd-ui-shared-deps/target/osd-ui-shared-deps.css](/packages/osd-ui-shared-deps/target/osd-ui-shared-deps.css)), packaged by [/packages/osd-ui-shared-deps/webpack.config.js](/packages/osd-ui-shared-deps/webpack.config.js). In theory, this file could include styles from other shared dependencies, but currently `osd-monaco` is the only package that exports styles. Note that these are the default, un-themed styles; theming of monaco editors is handled by [/src/plugins/opensearch_dashboards_react/public/code_editor/editor_theme.ts](/src/plugins/opensearch_dashboards_react/public/code_editor/editor_theme.ts).
2. Theme and mode-specific OUI styles (e.g. [](), compiled by `packages/osd-ui-shared-deps/webpack.config.js`).
3. Theme and mode-specific KUI styles (e.g. `packages/osd-ui-framework/src/kui_next_dark.scss`, compiled by `packages/osd-ui-framework/Gruntfile.js`). Separate stylesheets for each theme version/dark mode combo (colors).
4. Mode-specific legacy styles (e.g. [/src/core/server/core_app/assets/legacy_dark_theme.css](/src/core/server/core_app/assets/legacy_dark_theme.css))

Component styles are not loaded as stylesheets.

## Current theme usage

### JSON/JS Vars

1. Defined by `packages/osd-ui-shared-deps/theme.ts`
1. Used by `src/plugins/charts/public/static/color_maps/color_maps.ts` to set vis colors
2. Used by `src/plugins/discover/public/application/components/chart/histogram/histogram.tsx` to define Discover histogram Elastic Chart styling
3. Used by `src/plugins/maps_legacy/public/map/opensearch_dashboards_map.js` and `src/plugins/region_map/public/choropleth_layer.js` for minor map UI styling (line color, empty shade)
4. Used by `src/plugins/vis_type_vega/public/data_model/vega_parser.ts` for Vega/Vega-Lite theming
2. Used by `src/plugins/vis_type_vislib/public/vislib/components/tooltip/tooltip.js` for tooltip spacing
3. Used by `src/plugins/expressions/public/react_expression_renderer.tsx` to define padding options.
4. Used by `src/core/server/rendering/views/theme.ts` to inject values into `src/core/server/rendering/views/styles.tsx`
5. Used (incorrectly) to style a badge color in `src/plugins/index_pattern_management/public/components/create_button/create_button.tsx`
6. Used by `src/plugins/opensearch_dashboards_react/public/code_editor/editor_theme.ts` to create Monaco theme styles
1 change: 1 addition & 0 deletions src/core/public/osd_bootstrap.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import { __osdBootstrap__ } from './';
describe('osd_bootstrap', () => {
beforeAll(() => {
const metadata = {
branding: { darkMode: 'true' },
i18n: { translationsUrl: 'http://localhost' },
vars: { apmConfig: null },
};
Expand Down
5 changes: 5 additions & 0 deletions src/core/public/osd_bootstrap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ export async function __osdBootstrap__() {
document.querySelector('osd-injected-metadata')!.getAttribute('data')!
);

const globals: any = typeof window === 'undefined' ? {} : window;
const themeTag: string = globals.__osdThemeTag__ || '';

injectedMetadata.branding.darkMode = themeTag.endsWith('dark');

let i18nError: Error | undefined;
const apmSystem = new ApmSystem(injectedMetadata.vars.apmConfig, injectedMetadata.basePath);

Expand Down
43 changes: 41 additions & 2 deletions src/core/public/ui_settings/ui_settings_client.ts
AMoo-Miki marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,13 @@
this.defaults = cloneDeep(params.defaults);
this.cache = defaultsDeep({}, this.defaults, cloneDeep(params.initialSettings));

if (
this.cache['theme:enableUserControl']?.userValue ??
this.cache['theme:enableUserControl']?.value
) {
this.cache = defaultsDeep(this.cache, this.getBrowserStoredSettings());

Check warning on line 65 in src/core/public/ui_settings/ui_settings_client.ts

View check run for this annotation

Codecov / codecov/patch

src/core/public/ui_settings/ui_settings_client.ts#L65

Added line #L65 was not covered by tests
}

params.done$.subscribe({
complete: () => {
this.update$.complete();
Expand Down Expand Up @@ -173,6 +180,28 @@
return this.updateErrors$.asObservable();
}

private getBrowserStoredSettings() {
const uiSettingsJSON = window.localStorage.getItem('uiSettings') || '{}';
try {
return JSON.parse(uiSettingsJSON);

Check warning on line 186 in src/core/public/ui_settings/ui_settings_client.ts

View check run for this annotation

Codecov / codecov/patch

src/core/public/ui_settings/ui_settings_client.ts#L185-L186

Added lines #L185 - L186 were not covered by tests
} catch (error) {
this.updateErrors$.next(error);

Check warning on line 188 in src/core/public/ui_settings/ui_settings_client.ts

View check run for this annotation

Codecov / codecov/patch

src/core/public/ui_settings/ui_settings_client.ts#L188

Added line #L188 was not covered by tests
}
return {};

Check warning on line 190 in src/core/public/ui_settings/ui_settings_client.ts

View check run for this annotation

Codecov / codecov/patch

src/core/public/ui_settings/ui_settings_client.ts#L190

Added line #L190 was not covered by tests
}

private setBrowserStoredSettings(key: string, newVal: any) {
const oldSettings = this.getBrowserStoredSettings();
const newSettings = cloneDeep(oldSettings);

Check warning on line 195 in src/core/public/ui_settings/ui_settings_client.ts

View check run for this annotation

Codecov / codecov/patch

src/core/public/ui_settings/ui_settings_client.ts#L194-L195

Added lines #L194 - L195 were not covered by tests
if (newVal === null) {
delete newSettings[key];

Check warning on line 197 in src/core/public/ui_settings/ui_settings_client.ts

View check run for this annotation

Codecov / codecov/patch

src/core/public/ui_settings/ui_settings_client.ts#L197

Added line #L197 was not covered by tests
} else {
newSettings[key] = { userValue: newVal };

Check warning on line 199 in src/core/public/ui_settings/ui_settings_client.ts

View check run for this annotation

Codecov / codecov/patch

src/core/public/ui_settings/ui_settings_client.ts#L199

Added line #L199 was not covered by tests
}
window.localStorage.setItem(`uiSettings`, JSON.stringify(newSettings));
return { settings: newSettings };

Check warning on line 202 in src/core/public/ui_settings/ui_settings_client.ts

View check run for this annotation

Codecov / codecov/patch

src/core/public/ui_settings/ui_settings_client.ts#L201-L202

Added lines #L201 - L202 were not covered by tests
BSFishy marked this conversation as resolved.
Show resolved Hide resolved
}

private assertUpdateAllowed(key: string) {
if (this.isOverridden(key)) {
throw new Error(
Expand All @@ -198,8 +227,18 @@
this.setLocally(key, newVal);

try {
const { settings } = (await this.api.batchSet(key, newVal)) || {};
this.cache = defaultsDeep({}, defaults, settings);
if (
this.cache['theme:enableUserControl']?.userValue ??
this.cache['theme:enableUserControl']?.value
) {
const { settings } = this.cache[key]?.preferBrowserSetting
? this.setBrowserStoredSettings(key, newVal)
: (await this.api.batchSet(key, newVal)) || {};
this.cache = defaultsDeep({}, defaults, this.getBrowserStoredSettings(), settings);

Check warning on line 237 in src/core/public/ui_settings/ui_settings_client.ts

View check run for this annotation

Codecov / codecov/patch

src/core/public/ui_settings/ui_settings_client.ts#L237

Added line #L237 was not covered by tests
} else {
const { settings } = (await this.api.batchSet(key, newVal)) || {};
this.cache = defaultsDeep({}, defaults, settings);
}
this.saved$.next({ key, newValue: newVal, oldValue: initialVal });
return true;
} catch (error) {
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading