-
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
[Dashboard Usability] Unified panel options pane #148301
Changes from 23 commits
b98a4ab
8270120
5fe7c32
a118d9a
ecc6c9c
3873430
f1b82c0
c6f3b65
fabc23f
bb76266
676c8cd
5792aaa
22ffadd
6ce97fc
4b3fc93
a100c62
5125c8d
e895508
8d6929f
69c562a
b48aef4
980aaa9
a051bdb
b9c1a8d
761b558
3b67edb
286478d
6b8d436
ae7d063
10bda3c
cbd4ccc
e736261
49ba0a4
3731bc7
284078d
3c6d0fe
491361e
6aa6987
de19d09
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 |
---|---|---|
|
@@ -21,6 +21,13 @@ import { genericEmbeddableInputIsEqual, omitGenericEmbeddableInput } from './dif | |
function getPanelTitle(input: EmbeddableInput, output: EmbeddableOutput) { | ||
return input.hidePanelTitles ? '' : input.title === undefined ? output.defaultTitle : input.title; | ||
} | ||
function getPanelDescription(input: EmbeddableInput, output: EmbeddableOutput) { | ||
return input.hidePanelTitles | ||
? '' | ||
: input.description === undefined | ||
? output.defaultDescription | ||
: input.description; | ||
} | ||
export abstract class Embeddable< | ||
TEmbeddableInput extends EmbeddableInput = EmbeddableInput, | ||
TEmbeddableOutput extends EmbeddableOutput = EmbeddableOutput, | ||
|
@@ -61,6 +68,7 @@ export abstract class Embeddable< | |
|
||
this.output = { | ||
title: getPanelTitle(input, output), | ||
description: getPanelDescription(input, output), | ||
...(this.reportsEmbeddableLoad() | ||
? {} | ||
: { | ||
|
@@ -187,6 +195,10 @@ export abstract class Embeddable< | |
return this.output.title || ''; | ||
} | ||
|
||
public getDescription(): string { | ||
return this.output.description || ''; | ||
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. super tiny nit: Would using |
||
} | ||
|
||
/** | ||
* Returns the top most parent embeddable, or itself if this embeddable | ||
* is not within a parent. | ||
|
@@ -283,6 +295,7 @@ export abstract class Embeddable< | |
this.inputSubject.next(newInput); | ||
this.updateOutput({ | ||
title: getPanelTitle(this.input, this.output), | ||
description: getPanelDescription(this.input, this.output), | ||
} as Partial<TEmbeddableOutput>); | ||
if (oldLastReloadRequestTime !== newInput.lastReloadRequestTime) { | ||
this.reload(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,8 +17,7 @@ import classNames from 'classnames'; | |
import React, { ReactNode } from 'react'; | ||
import { Subscription } from 'rxjs'; | ||
import deepEqual from 'fast-deep-equal'; | ||
import { CoreStart, OverlayStart, ThemeServiceStart } from '@kbn/core/public'; | ||
import { toMountPoint } from '@kbn/kibana-react-plugin/public'; | ||
import { CoreStart, ThemeServiceStart } from '@kbn/core/public'; | ||
import { isPromise } from '@kbn/std'; | ||
import { UsageCollectionStart } from '@kbn/usage-collection-plugin/public'; | ||
import { MaybePromise } from '@kbn/utility-types'; | ||
|
@@ -43,13 +42,12 @@ import { ViewMode } from '../types'; | |
import { EmbeddablePanelError } from './embeddable_panel_error'; | ||
import { RemovePanelAction } from './panel_header/panel_actions'; | ||
import { AddPanelAction } from './panel_header/panel_actions/add_panel/add_panel_action'; | ||
import { CustomizePanelTitleAction } from './panel_header/panel_actions/customize_title/customize_panel_action'; | ||
import { CustomizePanelAction } from './panel_header/panel_actions/customize_panel/customize_panel_action'; | ||
import { PanelHeader } from './panel_header/panel_header'; | ||
import { InspectPanelAction } from './panel_header/panel_actions/inspect_panel_action'; | ||
import { EditPanelAction } from '../actions'; | ||
import { CustomizePanelModal } from './panel_header/panel_actions/customize_title/customize_panel_modal'; | ||
import { EmbeddableStart } from '../../plugin'; | ||
import { EmbeddableStateTransfer, isSelfStyledEmbeddable } from '..'; | ||
import { EmbeddableStateTransfer, isSelfStyledEmbeddable, CommonlyUsedRange } from '..'; | ||
|
||
const sortByOrderField = ( | ||
{ order: orderA }: { order?: number }, | ||
|
@@ -85,6 +83,8 @@ interface Props { | |
getActions?: UiActionsService['getTriggerCompatibleActions']; | ||
getEmbeddableFactory?: EmbeddableStart['getEmbeddableFactory']; | ||
getAllEmbeddableFactories?: EmbeddableStart['getEmbeddableFactories']; | ||
dateFormat?: string; | ||
commonlyUsedRanges?: CommonlyUsedRange[]; | ||
overlays?: CoreStart['overlays']; | ||
notifications?: CoreStart['notifications']; | ||
application?: CoreStart['application']; | ||
|
@@ -121,7 +121,7 @@ interface InspectorPanelAction { | |
} | ||
|
||
interface BasePanelActions { | ||
customizePanelTitle: CustomizePanelTitleAction; | ||
customizePanel: CustomizePanelAction; | ||
addPanel: AddPanelAction; | ||
inspectPanel: InspectPanelAction; | ||
removePanel: RemovePanelAction; | ||
|
@@ -281,6 +281,7 @@ export class EmbeddablePanel extends React.Component<Props, State> { | |
if (this.state.error) contentAttrs['data-error'] = true; | ||
|
||
const title = this.props.embeddable.getTitle(); | ||
const description = this.props.embeddable.getDescription(); | ||
const headerId = this.generateId(); | ||
|
||
const selfStyledOptions = isSelfStyledEmbeddable(this.props.embeddable) | ||
|
@@ -302,13 +303,14 @@ export class EmbeddablePanel extends React.Component<Props, State> { | |
getActionContextMenuPanel={this.getActionContextMenuPanel} | ||
hidePanelTitle={this.state.hidePanelTitle || !!selfStyledOptions?.hideTitle} | ||
isViewMode={viewOnlyMode} | ||
customizeTitle={ | ||
'customizePanelTitle' in this.state.universalActions | ||
? this.state.universalActions.customizePanelTitle | ||
customizePanel={ | ||
'customizePanel' in this.state.universalActions | ||
? this.state.universalActions.customizePanel | ||
: undefined | ||
} | ||
closeContextMenu={this.state.closeContextMenu} | ||
title={title} | ||
description={description} | ||
index={this.props.index} | ||
badges={this.state.badges} | ||
notifications={this.state.notifications} | ||
|
@@ -397,34 +399,16 @@ export class EmbeddablePanel extends React.Component<Props, State> { | |
) { | ||
return actions; | ||
} | ||
const createGetUserData = (overlays: OverlayStart, theme: ThemeServiceStart) => | ||
async function getUserData(context: { embeddable: IEmbeddable }) { | ||
return new Promise<{ title: string | undefined; hideTitle?: boolean }>((resolve) => { | ||
const session = overlays.openModal( | ||
toMountPoint( | ||
<CustomizePanelModal | ||
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. Wow, I didn't realise this code lived right inside the embeddable panel before, nice clean up! |
||
embeddable={context.embeddable} | ||
updateTitle={(title, hideTitle) => { | ||
session.close(); | ||
resolve({ title, hideTitle }); | ||
}} | ||
cancel={() => session.close()} | ||
/>, | ||
{ theme$: theme.theme$ } | ||
), | ||
{ | ||
'data-test-subj': 'customizePanel', | ||
} | ||
); | ||
}); | ||
}; | ||
|
||
// Universal actions are exposed on the context menu for every embeddable, they bypass the trigger | ||
// registry. | ||
return { | ||
...actions, | ||
customizePanelTitle: new CustomizePanelTitleAction( | ||
createGetUserData(this.props.overlays, this.props.theme) | ||
customizePanel: new CustomizePanelAction( | ||
this.props.overlays, | ||
this.props.theme, | ||
this.props.commonlyUsedRanges, | ||
this.props.dateFormat | ||
), | ||
addPanel: new AddPanelAction( | ||
this.props.getEmbeddableFactory, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,177 @@ | ||
/* | ||
* 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. | ||
*/ | ||
|
||
import { overlayServiceMock } from '@kbn/core-overlays-browser-mocks'; | ||
import { themeServiceMock } from '@kbn/core-theme-browser-mocks'; | ||
import { | ||
TimeRangeEmbeddable, | ||
TimeRangeContainer, | ||
TIME_RANGE_EMBEDDABLE, | ||
} from '../../../../test_samples/embeddables'; | ||
import { CustomTimeRangeBadge } from './custom_time_range_badge'; | ||
|
||
// TODO make sure this is a functional test | ||
|
||
// test('Removing custom time range from badge resets embeddable back to container time', async () => { | ||
// const container = new TimeRangeContainer( | ||
// { | ||
// timeRange: { from: 'now-15m', to: 'now' }, | ||
// panels: { | ||
// '1': { | ||
// type: TIME_RANGE_EMBEDDABLE, | ||
// explicitInput: { | ||
// id: '1', | ||
// timeRange: { from: '1', to: '2' }, | ||
// }, | ||
// }, | ||
// '2': { | ||
// type: TIME_RANGE_EMBEDDABLE, | ||
// explicitInput: { | ||
// id: '2', | ||
// }, | ||
// }, | ||
// }, | ||
// id: '123', | ||
// }, | ||
// () => undefined | ||
// ); | ||
|
||
// await container.untilEmbeddableLoaded('1'); | ||
// await container.untilEmbeddableLoaded('2'); | ||
|
||
// const child1 = container.getChild<TimeRangeEmbeddable>('1'); | ||
// const child2 = container.getChild<TimeRangeEmbeddable>('2'); | ||
|
||
// const openModalMock = jest.fn(); | ||
// openModalMock.mockReturnValue({ close: jest.fn() }); | ||
|
||
// new CustomTimeRangeBadge( | ||
// overlayServiceMock.createStartContract(), | ||
// themeServiceMock.createStartContract(), | ||
// [], | ||
// 'MM YYYY' | ||
// ).execute({ | ||
// embeddable: child1, | ||
// }); | ||
|
||
// await nextTick(); | ||
// const openModal = openModalMock.mock.calls[0][0] as ReactElement; | ||
|
||
// const wrapper = mount(openModal); | ||
// findTestSubject(wrapper, 'removePerPanelTimeRangeButton').simulate('click'); | ||
|
||
// const promise = Rx.merge(child1.getInput$(), container.getOutput$(), container.getInput$()) | ||
// .pipe(skip(4), take(1)) | ||
// .toPromise(); | ||
|
||
// container.updateInput({ timeRange: { from: 'now-10m', to: 'now-5m' } }); | ||
|
||
// await promise; | ||
|
||
// expect(child1.getInput().timeRange).toEqual({ from: 'now-10m', to: 'now-5m' }); | ||
// expect(child2.getInput().timeRange).toEqual({ from: 'now-10m', to: 'now-5m' }); | ||
// }); | ||
|
||
test(`badge is not compatible with embeddable that inherits from parent`, async () => { | ||
const container = new TimeRangeContainer( | ||
{ | ||
timeRange: { from: 'now-15m', to: 'now' }, | ||
panels: { | ||
'1': { | ||
type: TIME_RANGE_EMBEDDABLE, | ||
explicitInput: { | ||
id: '1', | ||
}, | ||
}, | ||
}, | ||
id: '123', | ||
}, | ||
() => undefined | ||
); | ||
|
||
await container.untilEmbeddableLoaded('1'); | ||
|
||
const child = container.getChild<TimeRangeEmbeddable>('1'); | ||
|
||
const compatible = await new CustomTimeRangeBadge( | ||
overlayServiceMock.createStartContract(), | ||
themeServiceMock.createStartContract(), | ||
[], | ||
'MM YYYY' | ||
).isCompatible({ | ||
embeddable: child, | ||
}); | ||
expect(compatible).toBe(false); | ||
}); | ||
|
||
test(`badge is compatible with embeddable that has custom time range`, async () => { | ||
const container = new TimeRangeContainer( | ||
{ | ||
timeRange: { from: 'now-15m', to: 'now' }, | ||
panels: { | ||
'1': { | ||
type: TIME_RANGE_EMBEDDABLE, | ||
explicitInput: { | ||
id: '1', | ||
timeRange: { to: '123', from: '456' }, | ||
}, | ||
}, | ||
}, | ||
id: '123', | ||
}, | ||
() => undefined | ||
); | ||
|
||
await container.untilEmbeddableLoaded('1'); | ||
|
||
const child = container.getChild<TimeRangeEmbeddable>('1'); | ||
|
||
const compatible = await new CustomTimeRangeBadge( | ||
overlayServiceMock.createStartContract(), | ||
themeServiceMock.createStartContract(), | ||
[], | ||
'MM YYYY' | ||
).isCompatible({ | ||
embeddable: child, | ||
}); | ||
expect(compatible).toBe(true); | ||
}); | ||
|
||
test('Attempting to execute on incompatible embeddable throws an error', async () => { | ||
const container = new TimeRangeContainer( | ||
{ | ||
timeRange: { from: 'now-15m', to: 'now' }, | ||
panels: { | ||
'1': { | ||
type: TIME_RANGE_EMBEDDABLE, | ||
explicitInput: { | ||
id: '1', | ||
}, | ||
}, | ||
}, | ||
id: '123', | ||
}, | ||
() => undefined | ||
); | ||
|
||
await container.untilEmbeddableLoaded('1'); | ||
|
||
const child = container.getChild<TimeRangeEmbeddable>('1'); | ||
|
||
const badge = await new CustomTimeRangeBadge( | ||
overlayServiceMock.createStartContract(), | ||
themeServiceMock.createStartContract(), | ||
[], | ||
'MM YYYY' | ||
); | ||
|
||
async function check() { | ||
await badge.execute({ embeddable: child }); | ||
} | ||
await expect(check()).rejects.toThrow(Error); | ||
}); |
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.
Is there a way to make this slightly more understandable? Double ternaries can sometimes be head-scratching.