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 2 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
70 changes: 1 addition & 69 deletions src/plugins/charts/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,72 +26,4 @@ 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 suplement the Elastic-Charts `baseTheme`. 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 shared `theme` and `baseTheme`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The `theme` service offers utilities to interact with the kibana theme. EUI provides a light and dark theme object to suplement the Elastic-Charts `baseTheme`. 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 shared `theme` and `baseTheme`.
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`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gracias 189843c


> 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` type in `@elastic/charts` without having to update the `@elastic/eui` themes or every `<Chart />`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The `theme` prop is a recursive partial `Theme` that overrides properties from the `baseTheme`. This allows changes to the `Theme` type in `@elastic/charts` without having to update the `@elastic/eui` themes or every `<Chart />`.
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 />`.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean that every <Chart /> instance now must provide both the baseTheme and the EUI provided theme?

Copy link
Contributor Author

@nickofthyme nickofthyme Jun 23, 2020

Choose a reason for hiding this comment

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

It's not a must. But since we added the background color to the theme for the contrast ratio logic, the background color defaults to a non-transparent colors respective of the mode (i.e. light and dark). So in order for the chart to work in both dark and light modes without a baseTheme, one would need to set the theme.background.color to 'transparent'. This would avoid the contrast logic though.

I'm not sure if 'transparent' should just be the default for the background color or if you just want to add this to the EUI themes.

But on a more general note, I think it's helpful to have a baseTheme used in all elastic/charts Charts in kibana for times when we change the Theme object. This allows for nothing to break while in the meantime we update any changes to the EUI charts partial themes.

I raised the question in an EC issue, to see how we could do this better (see elastic/elastic-charts#718). As of now, I think a HOC to handle this logic and allow overrides would be the most practicable option. Something like...

const MyComponent = () => (
  <Chart>
    <ThemedSettings theme={EuiThemeOverrides} {...otherSettingsProps} />
    {/* more goodness */}
  </Chart>
)

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 spelling errors here 189843c

54 changes: 39 additions & 15 deletions src/plugins/charts/public/services/theme/theme.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,34 +18,42 @@
*/

import { useEffect, useState } from 'react';
import { map } from 'rxjs/operators';
import { Observable } from 'rxjs';
import { Observable, BehaviorSubject } from 'rxjs';

import { CoreSetup } from 'kibana/public';
import { RecursivePartial, Theme } from '@elastic/charts';
import { DARK_THEME, LIGHT_THEME, PartialTheme, Theme } from '@elastic/charts';
import { EUI_CHARTS_THEME_DARK, EUI_CHARTS_THEME_LIGHT } from '@elastic/eui/dist/eui_charts_theme';

export class ThemeService {
private _chartsTheme$?: Observable<RecursivePartial<Theme>>;

/** Returns default charts theme */
public readonly chartsDefaultTheme = EUI_CHARTS_THEME_LIGHT.theme;
public readonly chartsDefaultBaseTheme = LIGHT_THEME;

private _uiSettingsDarkMode$?: Observable<boolean>;
private _chartsTheme$ = new BehaviorSubject(this.chartsDefaultTheme);
private _chartsBaseTheme$ = new BehaviorSubject(this.chartsDefaultBaseTheme);

/** An observable of the current charts theme */
public get chartsTheme$(): Observable<RecursivePartial<Theme>> {
if (!this._chartsTheme$) {
public chartsTheme$ = this._chartsTheme$.asObservable();

/** An observable of the current charts base theme */
public chartsBaseTheme$ = this._chartsBaseTheme$.asObservable();

/** An observable boolean for dark mode of kibana */
public get darkModeEnabled$(): Observable<boolean> {
if (!this._uiSettingsDarkMode$) {
throw new Error('ThemeService not initialized');
}

return this._chartsTheme$;
return this._uiSettingsDarkMode$;
}

/** A React hook for consuming the charts theme */
public useChartsTheme = () => {
/* eslint-disable-next-line react-hooks/rules-of-hooks */
public useChartsTheme = (): PartialTheme => {
// eslint-disable-next-line react-hooks/rules-of-hooks
const [value, update] = useState(this.chartsDefaultTheme);

/* eslint-disable-next-line react-hooks/rules-of-hooks */
// eslint-disable-next-line react-hooks/rules-of-hooks
useEffect(() => {
const s = this.chartsTheme$.subscribe(update);
return () => s.unsubscribe();
Expand All @@ -54,12 +62,28 @@ export class ThemeService {
return value;
};

/** A React hook for consuming the charts theme */
public useChartsBaseTheme = (): Theme => {
// eslint-disable-next-line react-hooks/rules-of-hooks
const [value, update] = useState(LIGHT_THEME);

// eslint-disable-next-line react-hooks/rules-of-hooks
useEffect(() => {
const s = this.chartsBaseTheme$.subscribe(update);
return () => s.unsubscribe();
}, []);

return value;
};

/** initialize service with uiSettings */
public init(uiSettings: CoreSetup['uiSettings']) {
this._chartsTheme$ = uiSettings
.get$('theme:darkMode')
.pipe(
map((darkMode) => (darkMode ? EUI_CHARTS_THEME_DARK.theme : EUI_CHARTS_THEME_LIGHT.theme))
this._uiSettingsDarkMode$ = uiSettings.get$<boolean>('theme:darkMode');
this._uiSettingsDarkMode$.subscribe((darkMode) => {
this._chartsTheme$.next(
darkMode ? EUI_CHARTS_THEME_DARK.theme : EUI_CHARTS_THEME_LIGHT.theme
);
this._chartsBaseTheme$.next(darkMode ? DARK_THEME : LIGHT_THEME);
});
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,13 @@ import {
ElementClickListener,
XYChartElementEvent,
BrushEndListener,
Theme,
} from '@elastic/charts';

import { i18n } from '@kbn/i18n';
import { IUiSettingsClient } from 'kibana/public';
import { EuiChartThemeType } from '@elastic/eui/dist/eui_charts_theme';
import { Subscription } from 'rxjs';
import { Subscription, combineLatest } from 'rxjs';
import { getServices } from '../../../kibana_services';
import { Chart as IChart } from '../helpers/point_series';

Expand All @@ -56,6 +57,7 @@ export interface DiscoverHistogramProps {

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

function findIntervalFromDuration(
Expand Down Expand Up @@ -126,18 +128,21 @@ export class DiscoverHistogram extends Component<DiscoverHistogramProps, Discove
private subscription?: Subscription;
public state = {
chartsTheme: getServices().theme.chartsDefaultTheme,
chartsBaseTheme: getServices().theme.chartsDefaultBaseTheme,
};

componentDidMount() {
this.subscription = getServices().theme.chartsTheme$.subscribe(
(chartsTheme: EuiChartThemeType['theme']) => this.setState({ chartsTheme })
this.subscription = combineLatest(
getServices().theme.chartsTheme$,
getServices().theme.chartsBaseTheme$
).subscribe(([chartsTheme, chartsBaseTheme]) =>
this.setState({ chartsTheme, chartsBaseTheme })
);
}

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

Expand Down Expand Up @@ -204,7 +209,7 @@ export class DiscoverHistogram extends Component<DiscoverHistogramProps, Discove
const uiSettings = getServices().uiSettings;
const timeZone = getTimezone(uiSettings);
const { chartData } = this.props;
const { chartsTheme } = this.state;
const { chartsTheme, chartsBaseTheme } = this.state;

if (!chartData) {
return null;
Expand Down Expand Up @@ -293,6 +298,7 @@ export class DiscoverHistogram extends Component<DiscoverHistogramProps, Discove
onElementClick={this.onElementClick(xInterval)}
tooltip={tooltipProps}
theme={chartsTheme}
baseTheme={chartsBaseTheme}
/>
<Axis
id="discover-histogram-left-axis"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import { getInterval } from '../../lib/get_interval';
import { areFieldsDifferent } from '../../lib/charts';
import { createXaxisFormatter } from '../../lib/create_xaxis_formatter';
import { STACKED_OPTIONS } from '../../../visualizations/constants';
import { getCoreStart, getUISettings } from '../../../../services';
import { getCoreStart } from '../../../../services';

export class TimeseriesVisualization extends Component {
static propTypes = {
Expand Down Expand Up @@ -237,7 +237,6 @@ export class TimeseriesVisualization extends Component {
}
});

const darkMode = getUISettings().get('theme:darkMode');
return (
<div className="tvbVis" style={styles.tvbVis}>
<TimeSeries
Expand All @@ -246,7 +245,6 @@ export class TimeseriesVisualization extends Component {
onBrush={onBrush}
enableHistogramMode={enableHistogramMode}
backgroundColor={model.background_color}
darkMode={darkMode}
showGrid={Boolean(model.show_grid)}
legend={Boolean(model.show_legend)}
legendPosition={model.legend_position}
Expand Down
Loading