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

[Visualize] First version of by-value visualize editor #72256

Merged
merged 18 commits into from
Aug 18, 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
Original file line number Diff line number Diff line change
Expand Up @@ -468,9 +468,14 @@ export class DashboardAppController {
const explicitInput = {
savedVis: input,
};
const embeddableId =
'embeddableId' in incomingEmbeddable
? incomingEmbeddable.embeddableId
: undefined;
container.addOrUpdateEmbeddable<EmbeddableInput>(
incomingEmbeddable.type,
explicitInput
explicitInput,
embeddableId
);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,7 @@ export class DashboardContainer extends Container<InheritedChildInput, Dashboard
// TODO: In the current infrastructure, embeddables in a container do not react properly to
// changes. Removing the existing embeddable, and adding a new one is a temporary workaround
// until the container logic is fixed.

const finalPanels = { ...this.input.panels };
delete finalPanels[previousPanelState.explicitInput.id];
const newPanelId = newPanelState.explicitInput?.id ? newPanelState.explicitInput.id : uuid.v4();
Expand All @@ -196,9 +197,10 @@ export class DashboardContainer extends Container<InheritedChildInput, Dashboard
EEI extends EmbeddableInput = EmbeddableInput,
EEO extends EmbeddableOutput = EmbeddableOutput,
E extends IEmbeddable<EEI, EEO> = IEmbeddable<EEI, EEO>
>(type: string, explicitInput: Partial<EEI>) {
if (explicitInput.id && this.input.panels[explicitInput.id]) {
this.replacePanel(this.input.panels[explicitInput.id], {
>(type: string, explicitInput: Partial<EEI>, embeddableId?: string) {
const idToReplace = embeddableId || explicitInput.id;
if (idToReplace && this.input.panels[idToReplace]) {
this.replacePanel(this.input.panels[idToReplace], {
type,
explicitInput: {
...explicitInput,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,18 @@ test('is compatible when edit url is available, in edit mode and editable', asyn
test('redirects to app using state transfer', async () => {
applicationMock.currentAppId$ = of('superCoolCurrentApp');
const action = new EditPanelAction(getFactory, applicationMock, stateTransferMock);
const embeddable = new EditableEmbeddable({ id: '123', viewMode: ViewMode.EDIT }, true);
const input = { id: '123', viewMode: ViewMode.EDIT };
const embeddable = new EditableEmbeddable(input, true);
embeddable.getOutput = jest.fn(() => ({ editApp: 'ultraVisualize', editPath: '/123' }));
await action.execute({ embeddable });
expect(stateTransferMock.navigateToEditor).toHaveBeenCalledWith('ultraVisualize', {
path: '/123',
state: { originatingApp: 'superCoolCurrentApp' },
state: {
originatingApp: 'superCoolCurrentApp',
byValueMode: true,
embeddableId: '123',
valueInput: input,
},
});
});

Expand Down
20 changes: 17 additions & 3 deletions src/plugins/embeddable/public/lib/actions/edit_panel_action.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,12 @@ import { take } from 'rxjs/operators';
import { ViewMode } from '../types';
import { EmbeddableFactoryNotFoundError } from '../errors';
import { EmbeddableStart } from '../../plugin';
import { IEmbeddable, EmbeddableEditorState, EmbeddableStateTransfer } from '../..';
import {
IEmbeddable,
EmbeddableEditorState,
EmbeddableStateTransfer,
SavedObjectEmbeddableInput,
} from '../..';

export const ACTION_EDIT_PANEL = 'editPanel';

Expand Down Expand Up @@ -109,8 +114,17 @@ export class EditPanelAction implements Action<ActionContext> {
const app = embeddable ? embeddable.getOutput().editApp : undefined;
const path = embeddable ? embeddable.getOutput().editPath : undefined;
if (app && path) {
const state = this.currentAppId ? { originatingApp: this.currentAppId } : undefined;
return { app, path, state };
if (this.currentAppId) {
const byValueMode = !(embeddable.getInput() as SavedObjectEmbeddableInput).savedObjectId;
const state: EmbeddableEditorState = {
originatingApp: this.currentAppId,
byValueMode,
valueInput: byValueMode ? embeddable.getInput() : undefined,
embeddableId: embeddable.id,
};
return { app, path, state };
}
return { app, path };
}
}

Expand Down
2 changes: 2 additions & 0 deletions src/plugins/embeddable/public/lib/state_transfer/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export interface EmbeddableEditorState {
originatingApp: string;
byValueMode?: boolean;
valueInput?: EmbeddableInput;
embeddableId?: string;
}

export function isEmbeddableEditorState(state: unknown): state is EmbeddableEditorState {
Expand All @@ -49,6 +50,7 @@ export interface EmbeddablePackageByReferenceState {
export interface EmbeddablePackageByValueState {
type: string;
input: EmbeddableInput;
embeddableId?: string;
}

export type EmbeddablePackageState =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ export function TopNavMenuItem(props: TopNavMenuData) {
iconType: props.iconType,
iconSide: props.iconSide,
'data-test-subj': props.testId,
className: props.className,
};

const btn = props.emphasize ? (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,6 @@ class DefaultEditorController {
]
: visType.editorConfig.optionTabs),
];

this.state = {
vis,
optionTabs,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ export const createVisEmbeddableFromObject = (deps: VisualizeEmbeddableFactoryDe
try {
const visId = vis.id as string;

const editPath = visId ? savedVisualizations.urlFor(visId) : '';
const editPath = visId ? savedVisualizations.urlFor(visId) : '#/edit_by_value';

const editUrl = visId
? getHttp().basePath.prepend(`/app/visualize${savedVisualizations.urlFor(visId)}`)
: '';
Expand Down
10 changes: 9 additions & 1 deletion src/plugins/visualize/public/application/app.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,12 @@ import { Route, Switch, useLocation } from 'react-router-dom';
import { syncQueryStateWithUrl } from '../../../data/public';
import { useKibana } from '../../../kibana_react/public';
import { VisualizeServices } from './types';
import { VisualizeEditor, VisualizeListing, VisualizeNoMatch } from './components';
import {
VisualizeEditor,
VisualizeListing,
VisualizeNoMatch,
VisualizeByValueEditor,
} from './components';
import { VisualizeConstants } from './visualize_constants';

export const VisualizeApp = () => {
Expand All @@ -48,6 +53,9 @@ export const VisualizeApp = () => {

return (
<Switch>
<Route exact path={`${VisualizeConstants.EDIT_BY_VALUE_PATH}`}>
<VisualizeByValueEditor />
Copy link
Contributor

Choose a reason for hiding this comment

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

What if you try instead to use the same <VisualizeEditor /> at this route, but use another effects inside when you trap into /edit_by_value path? It seems this will help to get rid of VisualizeEditorCommon ccomponent and code duplications between VisualizeByValueEditor <---> VisualizeEditor

Copy link
Contributor Author

@majagrubic majagrubic Jul 22, 2020

Choose a reason for hiding this comment

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

I am not sure how? Hooks need to be run in order and they don't like branching statements. I thought having separate components was actually cleaner.

</Route>
<Route path={[VisualizeConstants.CREATE_PATH, `${VisualizeConstants.EDIT_PATH}/:id`]}>
<VisualizeEditor />
</Route>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,4 @@
export { VisualizeListing } from './visualize_listing';
export { VisualizeEditor } from './visualize_editor';
export { VisualizeNoMatch } from './visualize_no_match';
export { VisualizeByValueEditor } from './visualize_byvalue_editor';
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import './visualize_editor.scss';
import React, { useEffect, useState } from 'react';
import { EventEmitter } from 'events';

import { VisualizeInput } from 'src/plugins/visualizations/public';
import { useKibana } from '../../../../kibana_react/public';
import {
useChromeVisibility,
useVisByValue,
useVisualizeAppState,
useEditorUpdates,
useLinkedSearchUpdates,
} from '../utils';
import { VisualizeServices } from '../types';
import { VisualizeEditorCommon } from './visualize_editor_common';

export const VisualizeByValueEditor = () => {
const [originatingApp, setOriginatingApp] = useState<string>();
const { services } = useKibana<VisualizeServices>();
const [eventEmitter] = useState(new EventEmitter());
const [hasUnsavedChanges, setHasUnsavedChanges] = useState(true);
const [embeddableId, setEmbeddableId] = useState<string>();
const [valueInput, setValueInput] = useState<VisualizeInput>();

useEffect(() => {
const { originatingApp: value, embeddableId: embeddableIdValue, valueInput: valueInputValue } =
services.embeddable
.getStateTransfer(services.scopedHistory)
.getIncomingEditorState({ keysToRemoveAfterFetch: ['id', 'embeddableId', 'valueInput'] }) ||
{};
setOriginatingApp(value);
setValueInput(valueInputValue);
setEmbeddableId(embeddableIdValue);
if (!valueInputValue) {
history.back();
}
}, [services]);

const isChromeVisible = useChromeVisibility(services.chrome);

const { byValueVisInstance, visEditorRef, visEditorController } = useVisByValue(
services,
eventEmitter,
isChromeVisible,
valueInput,
originatingApp
);
const { appState, hasUnappliedChanges } = useVisualizeAppState(
services,
eventEmitter,
byValueVisInstance
);
const { isEmbeddableRendered, currentAppState } = useEditorUpdates(
Copy link
Contributor

Choose a reason for hiding this comment

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

The useEditorUpdates subscribes on the appState updates (which can be changed through the url).
Seems you don't need that subscription at all?

services,
eventEmitter,
setHasUnsavedChanges,
appState,
byValueVisInstance,
visEditorController
);
useLinkedSearchUpdates(services, eventEmitter, appState, byValueVisInstance);

useEffect(() => {
// clean up all registered listeners if any is left
return () => {
eventEmitter.removeAllListeners();
};
}, [eventEmitter]);

return (
<VisualizeEditorCommon
visInstance={byValueVisInstance}
appState={appState}
currentAppState={currentAppState}
isChromeVisible={isChromeVisible}
hasUnsavedChanges={hasUnsavedChanges}
hasUnappliedChanges={hasUnappliedChanges}
isEmbeddableRendered={isEmbeddableRendered}
originatingApp={originatingApp}
setOriginatingApp={setOriginatingApp}
setHasUnsavedChanges={setHasUnsavedChanges}
visEditorRef={visEditorRef}
embeddableId={embeddableId}
/>
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,6 @@ import './visualize_editor.scss';
import React, { useEffect, useState } from 'react';
import { useParams } from 'react-router-dom';
import { EventEmitter } from 'events';
import { FormattedMessage } from '@kbn/i18n/react';
import { EuiScreenReaderOnly } from '@elastic/eui';

import { useKibana } from '../../../../kibana_react/public';
import {
Expand All @@ -33,8 +31,7 @@ import {
useLinkedSearchUpdates,
} from '../utils';
import { VisualizeServices } from '../types';
import { ExperimentalVisInfo } from './experimental_vis_info';
import { VisualizeTopNav } from './visualize_top_nav';
import { VisualizeEditorCommon } from './visualize_editor_common';

export const VisualizeEditor = () => {
const { id: visualizationIdFromUrl } = useParams();
Expand Down Expand Up @@ -67,7 +64,9 @@ export const VisualizeEditor = () => {

useEffect(() => {
const { originatingApp: value } =
services.embeddable.getStateTransfer(services.scopedHistory).getIncomingEditorState() || {};
services.embeddable
.getStateTransfer(services.scopedHistory)
.getIncomingEditorState({ keysToRemoveAfterFetch: ['id', 'input'] }) || {};
setOriginatingApp(value);
}, [services]);

Expand All @@ -79,38 +78,19 @@ export const VisualizeEditor = () => {
}, [eventEmitter]);

return (
<div className={`app-container visEditor visEditor--${savedVisInstance?.vis.type.name}`}>
{savedVisInstance && appState && currentAppState && (
<VisualizeTopNav
currentAppState={currentAppState}
hasUnsavedChanges={hasUnsavedChanges}
setHasUnsavedChanges={setHasUnsavedChanges}
isChromeVisible={isChromeVisible}
isEmbeddableRendered={isEmbeddableRendered}
hasUnappliedChanges={hasUnappliedChanges}
originatingApp={originatingApp}
setOriginatingApp={setOriginatingApp}
savedVisInstance={savedVisInstance}
stateContainer={appState}
visualizationIdFromUrl={visualizationIdFromUrl}
/>
)}
{savedVisInstance?.vis?.type?.isExperimental && <ExperimentalVisInfo />}
{savedVisInstance && (
<EuiScreenReaderOnly>
<h1>
<FormattedMessage
id="visualize.pageHeading"
defaultMessage="{chartName} {chartType} visualization"
values={{
chartName: savedVisInstance.savedVis.title,
chartType: savedVisInstance.vis.type.title,
}}
/>
</h1>
</EuiScreenReaderOnly>
)}
<div className={isChromeVisible ? 'visEditor__content' : 'visualize'} ref={visEditorRef} />
</div>
<VisualizeEditorCommon
visInstance={savedVisInstance}
appState={appState}
currentAppState={currentAppState}
isChromeVisible={isChromeVisible}
hasUnsavedChanges={hasUnsavedChanges}
hasUnappliedChanges={hasUnappliedChanges}
isEmbeddableRendered={isEmbeddableRendered}
originatingApp={originatingApp}
setOriginatingApp={setOriginatingApp}
visualizationIdFromUrl={visualizationIdFromUrl}
setHasUnsavedChanges={setHasUnsavedChanges}
visEditorRef={visEditorRef}
/>
);
};
Loading