Skip to content

Return to dashboard after editing embeddable #62865

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.

Sign up for GitHub

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

Merged
merged 26 commits into from
May 5, 2020
Merged

Return to dashboard after editing embeddable #62865

Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
4060495
updated to match the NP migration
ThomThomson Apr 23, 2020
d0360f1
Made the edit panel action apply an originating app argument instead …
ThomThomson Apr 24, 2020
a4a7767
i8n and wording changes for Visualize top nav
ThomThomson Apr 24, 2020
33ff0e3
Merge branch 'master' of github.com:elastic/kibana into dashboard/vis…
ThomThomson Apr 24, 2020
9d05fe3
fixing jest tests
ThomThomson Apr 27, 2020
a8a2f4a
Merge branch 'master' of github.com:elastic/kibana into dashboard/vis…
ThomThomson Apr 27, 2020
e7b8b47
Merge branch 'master' of github.com:elastic/kibana into dashboard/vis…
ThomThomson Apr 27, 2020
0873c99
Merge branch 'master' of github.com:elastic/kibana into dashboard/vis…
ThomThomson Apr 29, 2020
60340a9
standardized editing and create new flow with an 'originatingApp
ThomThomson Apr 30, 2020
a6d311d
added redirect to origin functionality to lens
ThomThomson Apr 30, 2020
b849857
Merge branch 'master' of github.com:elastic/kibana into dashboard/vis…
ThomThomson Apr 30, 2020
12248c3
added and fixed functional and jest tests
ThomThomson May 1, 2020
5ee17fe
more functional / jest test fixes
ThomThomson May 1, 2020
898a47c
Merge branch 'master' of github.com:elastic/kibana into dashboard/vis…
ThomThomson May 1, 2020
def2506
removed unused translations, switched checks
ThomThomson May 2, 2020
2856ab4
fixed an issue with test_subjects.ts where eui switches always return…
ThomThomson May 2, 2020
679a335
Merge branch 'master' of github.com:elastic/kibana into dashboard/vis…
ThomThomson May 2, 2020
0f9e8d5
undid changes to application start, removed originatingApp variable f…
ThomThomson May 2, 2020
485ee5a
cleaned dif
ThomThomson May 2, 2020
70a2a5f
Added application service to dashboard tests
ThomThomson May 3, 2020
3e5d9dc
null checks for application start in edit panel and visualize embedda…
ThomThomson May 3, 2020
f09d67e
Merge branch 'master' of github.com:elastic/kibana into dashboard/vis…
ThomThomson May 4, 2020
a2fa6b3
fixed typo, added save and return label to origin save modal
ThomThomson May 4, 2020
35ac231
cleaned up subscription with take(1) and fixed an oops
ThomThomson May 4, 2020
dd1ddc6
changed visualize_embeddable_factory to subscribe and unsubscribe dur…
ThomThomson May 5, 2020
bcac17c
Merge branch 'master' into dashboard/visEditRedirect
elasticmachine May 5, 2020
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
3 changes: 2 additions & 1 deletion src/plugins/dashboard/public/application/embeddable/viewport/dashboard_viewport.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import {
import { KibanaContextProvider } from '../../../../../kibana_react/public';
// eslint-disable-next-line
import { embeddablePluginMock } from 'src/plugins/embeddable/public/mocks';
import { applicationServiceMock } from '../../../../../../core/public/mocks';

let dashboardContainer: DashboardContainer | undefined;

Expand All @@ -50,7 +51,7 @@ function getProps(

const start = doStart();
const options: DashboardContainerOptions = {
application: {} as any,
application: applicationServiceMock.createStartContract(),
embeddable: {
getTriggerCompatibleActions: (() => []) as any,
getEmbeddableFactories: start.getEmbeddableFactories,
Expand Down
5 changes: 3 additions & 2 deletions src/plugins/dashboard/public/application/tests/dashboard_container.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import { embeddablePluginMock } from '../../../../embeddable/public/mocks';
import { inspectorPluginMock } from '../../../../inspector/public/mocks';
import { KibanaContextProvider } from '../../../../kibana_react/public';
import { uiActionsPluginMock } from '../../../../ui_actions/public/mocks';
import { applicationServiceMock } from '../../../../../core/public/mocks';

test('DashboardContainer in edit mode shows edit mode actions', async () => {
const inspector = inspectorPluginMock.createStartContract();
Expand All @@ -56,7 +57,7 @@ test('DashboardContainer in edit mode shows edit mode actions', async () => {

const initialInput = getSampleDashboardInput({ viewMode: ViewMode.VIEW });
const options: DashboardContainerOptions = {
application: {} as any,
application: applicationServiceMock.createStartContract(),
embeddable: start,
notifications: {} as any,
overlays: {} as any,
Expand Down Expand Up @@ -84,7 +85,7 @@ test('DashboardContainer in edit mode shows edit mode actions', async () => {
getAllEmbeddableFactories={(() => []) as any}
getEmbeddableFactory={(() => null) as any}
notifications={{} as any}
application={{} as any}
application={options.application}
overlays={{} as any}
inspector={inspector}
SavedObjectFinder={() => null}
Expand Down
1 change: 0 additions & 1 deletion src/plugins/dashboard/public/dashboard_constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
*/

export const DashboardConstants = {
ADD_VISUALIZATION_TO_DASHBOARD_MODE_PARAM: 'addToDashboard',
LANDING_PAGE_PATH: '/dashboards',
CREATE_NEW_DASHBOARD_URL: '/dashboard',
ADD_EMBEDDABLE_ID: 'addEmbeddableId',
Expand Down
1 change: 1 addition & 0 deletions src/plugins/embeddable/public/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import './index.scss';
import { PluginInitializerContext } from 'src/core/public';
import { EmbeddablePublicPlugin } from './plugin';

export { EMBEDDABLE_ORIGINATING_APP_PARAM } from './types';
export {
ACTION_ADD_PANEL,
ACTION_APPLY_FILTER,
Expand Down
12 changes: 7 additions & 5 deletions src/plugins/embeddable/public/lib/actions/edit_panel_action.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,12 @@ import { Embeddable, EmbeddableInput } from '../embeddables';
import { ViewMode } from '../types';
import { ContactCardEmbeddable } from '../test_samples';
import { embeddablePluginMock } from '../../mocks';
import { applicationServiceMock } from '../../../../../core/public/mocks';

const { doStart } = embeddablePluginMock.createInstance();
const start = doStart();
const getFactory = start.getEmbeddableFactory;
const applicationMock = applicationServiceMock.createStartContract();

class EditableEmbeddable extends Embeddable {
public readonly type = 'EDITABLE_EMBEDDABLE';
Expand All @@ -41,7 +43,7 @@ class EditableEmbeddable extends Embeddable {
}

test('is compatible when edit url is available, in edit mode and editable', async () => {
const action = new EditPanelAction(getFactory, {} as any);
const action = new EditPanelAction(getFactory, applicationMock);
expect(
await action.isCompatible({
embeddable: new EditableEmbeddable({ id: '123', viewMode: ViewMode.EDIT }, true),
Expand All @@ -50,7 +52,7 @@ test('is compatible when edit url is available, in edit mode and editable', asyn
});

test('getHref returns the edit urls', async () => {
const action = new EditPanelAction(getFactory, {} as any);
const action = new EditPanelAction(getFactory, applicationMock);
expect(action.getHref).toBeDefined();

if (action.getHref) {
Expand All @@ -64,7 +66,7 @@ test('getHref returns the edit urls', async () => {
});

test('is not compatible when edit url is not available', async () => {
const action = new EditPanelAction(getFactory, {} as any);
const action = new EditPanelAction(getFactory, applicationMock);
const embeddable = new ContactCardEmbeddable(
{
id: '123',
Expand All @@ -83,7 +85,7 @@ test('is not compatible when edit url is not available', async () => {
});

test('is not visible when edit url is available but in view mode', async () => {
const action = new EditPanelAction(getFactory, {} as any);
const action = new EditPanelAction(getFactory, applicationMock);
expect(
await action.isCompatible({
embeddable: new EditableEmbeddable(
Expand All @@ -98,7 +100,7 @@ test('is not visible when edit url is available but in view mode', async () => {
});

test('is not compatible when edit url is available, in edit mode, but not editable', async () => {
const action = new EditPanelAction(getFactory, {} as any);
const action = new EditPanelAction(getFactory, applicationMock);
expect(
await action.isCompatible({
embeddable: new EditableEmbeddable(
Expand Down
21 changes: 18 additions & 3 deletions src/plugins/embeddable/public/lib/actions/edit_panel_action.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ import { ApplicationStart } from 'kibana/public';
import { Action } from 'src/plugins/ui_actions/public';
import { ViewMode } from '../types';
import { EmbeddableFactoryNotFoundError } from '../errors';
import { IEmbeddable } from '../embeddables';
import { EmbeddableStart } from '../../plugin';
import { EMBEDDABLE_ORIGINATING_APP_PARAM, IEmbeddable } from '../..';

export const ACTION_EDIT_PANEL = 'editPanel';

Expand All @@ -35,11 +35,18 @@ export class EditPanelAction implements Action<ActionContext> {
public readonly type = ACTION_EDIT_PANEL;
public readonly id = ACTION_EDIT_PANEL;
public order = 50;
public currentAppId: string | undefined;

constructor(
private readonly getEmbeddableFactory: EmbeddableStart['getEmbeddableFactory'],
private readonly application: ApplicationStart
) {}
) {
if (this.application?.currentAppId$) {
this.application.currentAppId$.subscribe(
Copy link
Contributor

@Dosant Dosant May 4, 2020

Choose a reason for hiding this comment

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

I think we have to somehow unsubscribe here, otherwise will get a memory leak
maybe we could just .pipe(first()) to make sure it unsubscribed itself (if that is sufficient)
same here: https://github.com/elastic/kibana/pull/62865/files#diff-85e9be7a6b60df87e48500d65f596d1aR101

Copy link
Contributor Author

@ThomThomson ThomThomson May 4, 2020

Choose a reason for hiding this comment

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

Oh, good eye. The subscription in edit_panel_action I've cleaned up by using a pipe as suggested.

The subscription in visualize_embeddable_factory is a little different, as it needs to update correctly when the app id changes. To unsubscribe that, I have a method in the factory called unsubscribeSubscriptions. The plugin keeps a reference to the factory, then calls that method in its stop method. This was the best way I could think of to clean up that subscription.

Copy link
Contributor

@streamich streamich May 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The this.application.currentAppId$ is actually a BehaviorSubject it always has a value and it can be accessed synchronously.

My guess is that you should be able to do

let originatingAppParam = getObservableCurrentValue(this.params.start().core.application.currentAppId$);

where

const getObservableCurrentValue = (obs$) => {
  let value;
  obs$.pipe(take(1)).subscribe(anotherValue => value = anotherValue);
  return value;
};

🤞

(appId: string | undefined) => (this.currentAppId = appId)
);
}
}

public getDisplayName({ embeddable }: ActionContext) {
const factory = this.getEmbeddableFactory(embeddable.type);
Expand Down Expand Up @@ -93,7 +100,15 @@ export class EditPanelAction implements Action<ActionContext> {
}

public async getHref({ embeddable }: ActionContext): Promise<string> {
const editUrl = embeddable ? embeddable.getOutput().editUrl : undefined;
let editUrl = embeddable ? embeddable.getOutput().editUrl : undefined;
if (editUrl && this.currentAppId) {
editUrl += `?${EMBEDDABLE_ORIGINATING_APP_PARAM}=${this.currentAppId}`;

// TODO: Remove this after https://github.com/elastic/kibana/pull/63443
if (this.currentAppId === 'kibana') {
editUrl += `:${window.location.hash.split(/[\/\?]/)[1]}`;
}
}
return editUrl ? editUrl : '';
}
}
12 changes: 7 additions & 5 deletions src/plugins/embeddable/public/lib/panel/embeddable_panel.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import {
import { inspectorPluginMock } from '../../../../inspector/public/mocks';
import { EuiBadge } from '@elastic/eui';
import { embeddablePluginMock } from '../../mocks';
import { applicationServiceMock } from '../../../../../core/public/mocks';

const actionRegistry = new Map<string, Action>();
const triggerRegistry = new Map<string, Trigger>();
Expand All @@ -55,6 +56,7 @@ const trigger: Trigger = {
id: CONTEXT_MENU_TRIGGER,
};
const embeddableFactory = new ContactCardEmbeddableFactory((() => null) as any, {} as any);
const applicationMock = applicationServiceMock.createStartContract();

actionRegistry.set(editModeAction.id, editModeAction);
triggerRegistry.set(trigger.id, trigger);
Expand Down Expand Up @@ -159,7 +161,7 @@ test('HelloWorldContainer in view mode hides edit mode actions', async () => {
getAllEmbeddableFactories={start.getEmbeddableFactories}
getEmbeddableFactory={start.getEmbeddableFactory}
notifications={{} as any}
application={{} as any}
application={applicationMock}
overlays={{} as any}
inspector={inspector}
SavedObjectFinder={() => null}
Expand Down Expand Up @@ -199,7 +201,7 @@ const renderInEditModeAndOpenContextMenu = async (
getEmbeddableFactory={start.getEmbeddableFactory}
notifications={{} as any}
overlays={{} as any}
application={{} as any}
application={applicationMock}
inspector={inspector}
SavedObjectFinder={() => null}
/>
Expand Down Expand Up @@ -306,7 +308,7 @@ test('HelloWorldContainer in edit mode shows edit mode actions', async () => {
getEmbeddableFactory={start.getEmbeddableFactory}
notifications={{} as any}
overlays={{} as any}
application={{} as any}
application={applicationMock}
inspector={inspector}
SavedObjectFinder={() => null}
/>
Expand Down Expand Up @@ -369,7 +371,7 @@ test('Updates when hidePanelTitles is toggled', async () => {
getEmbeddableFactory={start.getEmbeddableFactory}
notifications={{} as any}
overlays={{} as any}
application={{} as any}
application={applicationMock}
inspector={inspector}
SavedObjectFinder={() => null}
/>
Expand Down Expand Up @@ -422,7 +424,7 @@ test('Check when hide header option is false', async () => {
getEmbeddableFactory={start.getEmbeddableFactory}
notifications={{} as any}
overlays={{} as any}
application={{} as any}
application={applicationMock}
inspector={inspector}
SavedObjectFinder={() => null}
hideHeader={false}
Expand Down
2 changes: 2 additions & 0 deletions src/plugins/embeddable/public/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ import {
EmbeddableFactoryDefinition,
} from './lib/embeddables';

export const EMBEDDABLE_ORIGINATING_APP_PARAM = 'embeddableOriginatingApp';

export type EmbeddableFactoryRegistry = Map<string, EmbeddableFactory>;

export type EmbeddableFactoryProvider = <
Expand Down
9 changes: 8 additions & 1 deletion src/plugins/saved_objects/public/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,14 @@

import { SavedObjectsPublicPlugin } from './plugin';

export { OnSaveProps, SavedObjectSaveModal, SaveResult, showSaveModal } from './save_modal';
export {
OnSaveProps,
SavedObjectSaveModal,
SavedObjectSaveModalOrigin,
SaveModalState,
SaveResult,
showSaveModal,
} from './save_modal';
export { getSavedObjectFinder, SavedObjectFinderUi, SavedObjectMetaData } from './finder';
export {
SavedObjectLoader,
Expand Down
3 changes: 2 additions & 1 deletion src/plugins/saved_objects/public/save_modal/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,6 @@
* under the License.
*/

export { SavedObjectSaveModal, OnSaveProps } from './saved_object_save_modal';
export { SavedObjectSaveModal, OnSaveProps, SaveModalState } from './saved_object_save_modal';
export { SavedObjectSaveModalOrigin } from './saved_object_save_modal_origin';
export { showSaveModal, SaveResult } from './show_saved_object_save_modal';
13 changes: 8 additions & 5 deletions src/plugins/saved_objects/public/save_modal/saved_object_save_modal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,14 +53,15 @@ interface Props {
onClose: () => void;
title: string;
showCopyOnSave: boolean;
initialCopyOnSave?: boolean;
objectType: string;
confirmButtonLabel?: React.ReactNode;
options?: React.ReactNode;
options?: React.ReactNode | ((state: SaveModalState) => React.ReactNode);
description?: string;
showDescription: boolean;
}

interface State {
export interface SaveModalState {
title: string;
copyOnSave: boolean;
isTitleDuplicateConfirmed: boolean;
Expand All @@ -71,11 +72,11 @@ interface State {

const generateId = htmlIdGenerator();

export class SavedObjectSaveModal extends React.Component<Props, State> {
export class SavedObjectSaveModal extends React.Component<Props, SaveModalState> {
private warning = React.createRef<HTMLDivElement>();
public readonly state = {
title: this.props.title,
copyOnSave: false,
copyOnSave: Boolean(this.props.initialCopyOnSave),
isTitleDuplicateConfirmed: false,
hasTitleDuplicate: false,
isLoading: false,
Expand Down Expand Up @@ -139,7 +140,9 @@ export class SavedObjectSaveModal extends React.Component<Props, State> {

{this.renderViewDescription()}

{this.props.options}
{typeof this.props.options === 'function'
? this.props.options(this.state)
: this.props.options}
</EuiForm>
</EuiModalBody>

Expand Down
Loading