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

Fix discover, tsvb and Lens chart theming issues #69695

Merged
merged 16 commits into from
Jul 2, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@
"@babel/plugin-transform-modules-commonjs": "^7.10.1",
"@babel/register": "^7.10.1",
"@elastic/apm-rum": "^5.2.0",
"@elastic/charts": "19.5.2",
"@elastic/charts": "19.6.3",
"@elastic/datemath": "5.0.3",
"@elastic/ems-client": "7.9.3",
"@elastic/eui": "24.1.0",
Expand Down
2 changes: 1 addition & 1 deletion packages/kbn-ui-shared-deps/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
"kbn:watch": "node scripts/build --dev --watch"
},
"dependencies": {
"@elastic/charts": "19.5.2",
"@elastic/charts": "19.6.3",
"@elastic/eui": "24.1.0",
"@elastic/numeral": "^2.5.0",
"@kbn/i18n": "1.0.0",
Expand Down
72 changes: 2 additions & 70 deletions src/plugins/charts/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,80 +18,12 @@ Color mappings in `value`/`text` form

### `getHeatmapColors`

Funciton to retrive heatmap related colors based on `value` and `colorSchemaName`
Function to retrieve heatmap related colors based on `value` and `colorSchemaName`

### `truncatedColorSchemas`

Truncated color mappings in `value`/`text` form

Copy link
Contributor

@stratoula stratoula Jun 24, 2020

Choose a reason for hiding this comment

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

It's irrelevant with this PR but I saw on L21 that there is a typo Funciton instead of Function and retrive instead of retrieve. Do you want to also correct it in this PR ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in d82c6b8

## Theme

the `theme` service offers utilities to interact with theme of kibana. EUI provides a light and dark theme object to work with Elastic-Charts. However, every instance of a Chart would need to pass down this the correctly EUI theme depending on Kibana's light or dark mode. There are several ways you can use the `theme` service to get the correct theme.

> The current theme (light or dark) of Kibana is typically taken into account for the functions below.

### `useChartsTheme`

The simple fetching of the correct EUI theme; a **React hook**.

```js
import { npStart } from 'ui/new_platform';
import { Chart, Settings } from '@elastic/charts';

export const YourComponent = () => (
<Chart>
<Settings theme={npStart.plugins.charts.theme.useChartsTheme()} />
</Chart>
);
```

### `chartsTheme$`

An **observable** of the current charts theme. Use this implementation for more flexible updates to the chart theme without full page refreshes.

```tsx
import { npStart } from 'ui/new_platform';
import { EuiChartThemeType } from '@elastic/eui/src/themes/charts/themes';
import { Subscription } from 'rxjs';
import { Chart, Settings } from '@elastic/charts';

interface YourComponentProps {};

interface YourComponentState {
chartsTheme: EuiChartThemeType['theme'];
}

export class YourComponent extends Component<YourComponentProps, YourComponentState> {
private subscription?: Subscription;
public state = {
chartsTheme: npStart.plugins.charts.theme.chartsDefaultTheme,
};

componentDidMount() {
this.subscription = npStart.plugins.charts.theme
.chartsTheme$
.subscribe(chartsTheme => this.setState({ chartsTheme }));
}

componentWillUnmount() {
if (this.subscription) {
this.subscription.unsubscribe();
this.subscription = undefined;
}
}

public render() {
const { chartsTheme } = this.state;

return (
<Chart>
<Settings theme={chartsTheme} />
</Chart>
);
}
}
```

### `chartsDefaultTheme`

Returns default charts theme (i.e. light).
See Theme service [docs](public/services/theme/README.md)
92 changes: 92 additions & 0 deletions src/plugins/charts/public/services/theme/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
# Theme Service

The `theme` service offers utilities to interact with the kibana theme. EUI provides a light and dark theme object to supplement the Elastic-Charts `baseTheme`. However, every instance of a Chart would need to pass down the correct EUI theme depending on Kibana's light or dark mode. There are several ways you can use the `theme` service to get the correct shared `theme` and `baseTheme`.

> The current theme (light or dark) of Kibana is typically taken into account for the functions below.

## `chartsDefaultBaseTheme`

Default `baseTheme` from `@elastic/charts` (i.e. light).

## `chartsDefaultTheme`

Default `theme` from `@elastic/eui` (i.e. light).

## `useChartsTheme` and `useChartsBaseTheme`

A **React hook** for simple fetching of the correct EUI `theme` and `baseTheme`.

```js
import { npStart } from 'ui/new_platform';
import { Chart, Settings } from '@elastic/charts';

export const YourComponent = () => (
<Chart>
<Settings
theme={npStart.plugins.charts.theme.useChartsTheme()}
baseTheme={npStart.plugins.charts.theme.useChartsBaseTheme()}
/>
{/* ... */}
</Chart>
);
```

## `chartsTheme$` and `chartsBaseTheme$`

An **`Observable`** of the current charts `theme` and `baseTheme`. Use this implementation for more flexible updates to the chart theme without full page refreshes.

```tsx
import { npStart } from 'ui/new_platform';
import { EuiChartThemeType } from '@elastic/eui/src/themes/charts/themes';
import { Subscription, combineLatest } from 'rxjs';
import { Chart, Settings, Theme } from '@elastic/charts';

interface YourComponentProps {};

interface YourComponentState {
chartsTheme: EuiChartThemeType['theme'];
chartsBaseTheme: Theme;
}

export class YourComponent extends Component<YourComponentProps, YourComponentState> {
private subscriptions: Subscription[] = [];

public state = {
chartsTheme: npStart.plugins.charts.theme.chartsDefaultTheme,
chartsBaseTheme: npStart.plugins.charts.theme.chartsDefaultBaseTheme,
};

componentDidMount() {
this.subscription = combineLatest(
npStart.plugins.charts.theme.chartsTheme$,
npStart.plugins.charts.theme.chartsBaseTheme$
).subscribe(([chartsTheme, chartsBaseTheme]) =>
this.setState({ chartsTheme, chartsBaseTheme })
);
}

componentWillUnmount() {
if (this.subscription) {
this.subscription.unsubscribe();
}
}

public render() {
const { chartsBaseTheme, chartsTheme } = this.state;

return (
<Chart>
<Settings
theme={chartsTheme}
baseTheme={chartsBaseTheme}
/>
{/* ... */}
</Chart>
);
}
}
```

## Why have `theme` and `baseTheme`?

The `theme` prop is a recursive partial `Theme` that overrides properties from the `baseTheme`. This allows changes to the `Theme` TS type in `@elastic/charts` without having to update the `@elastic/eui` themes for every `<Chart />`.
14 changes: 11 additions & 3 deletions src/plugins/charts/public/services/theme/mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,17 @@ import { EUI_CHARTS_THEME_LIGHT } from '@elastic/eui/dist/eui_charts_theme';
import { ThemeService } from './theme';

export const themeServiceMock: ThemeService = {
chartsDefaultTheme: EUI_CHARTS_THEME_LIGHT.theme,
chartsTheme$: jest.fn(() => ({
subsribe: jest.fn(),
subscribe: jest.fn(),
})),
chartsDefaultTheme: EUI_CHARTS_THEME_LIGHT.theme,
useChartsTheme: jest.fn(),
chartsBaseTheme$: jest.fn(() => ({
subscribe: jest.fn(),
})),
darkModeEnabled$: jest.fn(() => ({
subscribe: jest.fn(),
})),
useDarkMode: jest.fn().mockReturnValue(false),
useChartsTheme: jest.fn().mockReturnValue({}),
useChartsBaseTheme: jest.fn().mockReturnValue({}),
} as any;
64 changes: 62 additions & 2 deletions src/plugins/charts/public/services/theme/theme.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,35 @@ import { EUI_CHARTS_THEME_DARK, EUI_CHARTS_THEME_LIGHT } from '@elastic/eui/dist

import { ThemeService } from './theme';
import { coreMock } from '../../../../../core/public/mocks';
import { LIGHT_THEME, DARK_THEME } from '@elastic/charts';

const { uiSettings: setupMockUiSettings } = coreMock.createSetup();

describe('ThemeService', () => {
describe('chartsTheme$', () => {
describe('darkModeEnabled$', () => {
it('should throw error if service has not been initialized', () => {
const themeService = new ThemeService();
expect(() => themeService.chartsTheme$).toThrowError();
expect(() => themeService.darkModeEnabled$).toThrowError();
});

it('returns the false when not in dark mode', async () => {
setupMockUiSettings.get$.mockReturnValue(new BehaviorSubject(false));
const themeService = new ThemeService();
themeService.init(setupMockUiSettings);

expect(await themeService.darkModeEnabled$.pipe(take(1)).toPromise()).toBe(false);
});

it('returns the true when in dark mode', async () => {
setupMockUiSettings.get$.mockReturnValue(new BehaviorSubject(true));
const themeService = new ThemeService();
themeService.init(setupMockUiSettings);

expect(await themeService.darkModeEnabled$.pipe(take(1)).toPromise()).toBe(true);
});
});

describe('chartsTheme$', () => {
it('returns the light theme when not in dark mode', async () => {
setupMockUiSettings.get$.mockReturnValue(new BehaviorSubject(false));
const themeService = new ThemeService();
Expand All @@ -58,6 +78,28 @@ describe('ThemeService', () => {
});
});

describe('chartsBaseTheme$', () => {
it('returns the light theme when not in dark mode', async () => {
setupMockUiSettings.get$.mockReturnValue(new BehaviorSubject(false));
const themeService = new ThemeService();
themeService.init(setupMockUiSettings);

expect(await themeService.chartsBaseTheme$.pipe(take(1)).toPromise()).toEqual(LIGHT_THEME);
});

describe('in dark mode', () => {
it(`returns the dark theme`, async () => {
// Fake dark theme turned returning true
setupMockUiSettings.get$.mockReturnValue(new BehaviorSubject(true));
const themeService = new ThemeService();
themeService.init(setupMockUiSettings);
const result = await themeService.chartsBaseTheme$.pipe(take(1)).toPromise();

expect(result).toEqual(DARK_THEME);
});
});
});

describe('useChartsTheme', () => {
it('updates when the uiSettings change', () => {
const darkMode$ = new BehaviorSubject(false);
Expand All @@ -75,4 +117,22 @@ describe('ThemeService', () => {
expect(result.current).toBe(EUI_CHARTS_THEME_LIGHT.theme);
});
});

describe('useBaseChartTheme', () => {
it('updates when the uiSettings change', () => {
const darkMode$ = new BehaviorSubject(false);
setupMockUiSettings.get$.mockReturnValue(darkMode$);
const themeService = new ThemeService();
themeService.init(setupMockUiSettings);
const { useChartsBaseTheme } = themeService;

const { result } = renderHook(() => useChartsBaseTheme());
expect(result.current).toBe(LIGHT_THEME);

act(() => darkMode$.next(true));
expect(result.current).toBe(DARK_THEME);
act(() => darkMode$.next(false));
expect(result.current).toBe(LIGHT_THEME);
});
});
});
Loading