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

Refactor app state and cleanup unused imports #4504

Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
7c4d7d3
Clean up use saved dashboard instance
kavilla Jul 1, 2023
a4ef964
Clean up editor subscriptions
kavilla Jul 1, 2023
6439487
clean up unused imports
kavilla Jul 1, 2023
056874e
update state mgmt
kavilla Jul 5, 2023
abcef81
Update src/plugins/dashboard/public/application/utils/create_dashboar…
kavilla Jul 5, 2023
3b246fa
carry over timeRestore
kavilla Jul 6, 2023
30cdb44
revert back to grid, reference too much of issue without hooks
abbyhu2000 Jul 6, 2023
9335611
currently working
kavilla Jul 6, 2023
562ab2c
clean up editor
kavilla Jul 6, 2023
0810963
fix the link
kavilla Jul 7, 2023
5aad98d
fix error handling in saved dashboards
kavilla Jul 7, 2023
27bdac8
fix create dashboard
kavilla Jul 7, 2023
83ecf46
fix history
kavilla Jul 7, 2023
049159b
fix legacy query
kavilla Jul 7, 2023
d329964
restore logic for used_saved dashboards
kavilla Jul 7, 2023
a0d6492
Add initial migration back to use dashboard container
abbyhu2000 Jul 7, 2023
f2b3cd4
fix nav action to use updated path
kavilla Jul 7, 2023
5d1f17d
restore app state url hybrid
kavilla Jul 8, 2023
2c5dff2
update url
kavilla Jul 8, 2023
be82c17
add some tests and remove conditional from use dash app state waiting…
kavilla Jul 8, 2023
3cfddca
add tests for editor hook
kavilla Jul 9, 2023
c9a0828
Update src/plugins/dashboard/public/application/utils/use/use_editor_…
kavilla Jul 9, 2023
2532263
readd the check
kavilla Jul 9, 2023
ff6e3da
add saved dashboard test
kavilla Jul 9, 2023
7b47c09
add more tests to the use saved dashboard instance test
kavilla Jul 10, 2023
c1f8cfa
skip test for now
kavilla Jul 10, 2023
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 @@ -3,7 +3,7 @@
* SPDX-License-Identifier: Apache-2.0
*/

import React, { useEffect, useState } from 'react';
import React, { useState } from 'react';
import { useParams } from 'react-router-dom';
import { EventEmitter } from 'events';
import { DashboardTopNav } from '../components/dashboard_top_nav';
Expand All @@ -12,95 +12,54 @@ import { useOpenSearchDashboards } from '../../../../opensearch_dashboards_react
import { useSavedDashboardInstance } from '../utils/use/use_saved_dashboard_instance';
import { DashboardServices } from '../../types';
import { useDashboardAppAndGlobalState } from '../utils/use/use_dashboard_app_state';
import { useDashboardContainer } from '../utils/use/use_dashboard_container';
import { useEditorUpdates } from '../utils/use/use_editor_updates';
import {
setBreadcrumbsForExistingDashboard,
setBreadcrumbsForNewDashboard,
} from '../utils/breadcrumbs';

export const DashboardEditor = () => {
const { id: dashboardIdFromUrl } = useParams<{ id: string }>();
const { services } = useOpenSearchDashboards<DashboardServices>();
const { chrome } = services;
const isChromeVisible = useChromeVisibility(chrome);
const isChromeVisible = useChromeVisibility({ chrome });
const [eventEmitter] = useState(new EventEmitter());

const { savedDashboard: savedDashboardInstance, dashboard } = useSavedDashboardInstance(
const { savedDashboard: savedDashboardInstance, dashboard } = useSavedDashboardInstance({
services,
eventEmitter,
isChromeVisible,
dashboardIdFromUrl
);
dashboardIdFromUrl,
});

const { appState } = useDashboardAppAndGlobalState(
const { appState, currentContainer, indexPatterns } = useDashboardAppAndGlobalState({
services,
eventEmitter,
savedDashboardInstance
);

const { dashboardContainer, indexPatterns } = useDashboardContainer(
services,
dashboard,
savedDashboardInstance,
appState
);
dashboard,
});

const { isEmbeddableRendered, currentAppState } = useEditorUpdates(
const { isEmbeddableRendered, currentAppState } = useEditorUpdates({
services,
eventEmitter,
dashboard,
savedDashboardInstance,
dashboardContainer,
appState
);

useEffect(() => {
if (currentAppState && dashboard) {
if (savedDashboardInstance?.id) {
chrome.setBreadcrumbs(
setBreadcrumbsForExistingDashboard(
savedDashboardInstance.title,
currentAppState.viewMode,
dashboard.isDirty
)
);
chrome.docTitle.change(savedDashboardInstance.title);
} else {
chrome.setBreadcrumbs(
setBreadcrumbsForNewDashboard(currentAppState.viewMode, dashboard.isDirty)
);
}
}
}, [currentAppState, savedDashboardInstance, chrome, dashboard]);

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

return (
<div>
<div>
{savedDashboardInstance &&
appState &&
dashboardContainer &&
currentAppState &&
dashboard && (
<DashboardTopNav
isChromeVisible={isChromeVisible}
savedDashboardInstance={savedDashboardInstance}
stateContainer={appState}
dashboard={dashboard}
currentAppState={currentAppState}
isEmbeddableRendered={isEmbeddableRendered}
indexPatterns={indexPatterns}
dashboardContainer={dashboardContainer}
dashboardIdFromUrl={dashboardIdFromUrl}
/>
)}
{savedDashboardInstance && appState && currentAppState && currentContainer && dashboard && (
<DashboardTopNav
isChromeVisible={isChromeVisible}
savedDashboardInstance={savedDashboardInstance}
appState={appState!}
dashboard={dashboard}
currentAppState={currentAppState}
isEmbeddableRendered={isEmbeddableRendered}
indexPatterns={indexPatterns}
currentContainer={currentContainer}
dashboardIdFromUrl={dashboardIdFromUrl}
/>
)}
</div>
</div>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@ import { Dashboard } from '../../dashboard';
interface DashboardTopNavProps {
isChromeVisible: boolean;
savedDashboardInstance: any;
stateContainer: DashboardAppStateContainer;
appState: DashboardAppStateContainer;
dashboard: Dashboard;
currentAppState: DashboardAppState;
isEmbeddableRendered: boolean;
indexPatterns: IndexPattern[];
dashboardContainer?: DashboardContainer;
currentContainer?: DashboardContainer;
dashboardIdFromUrl?: string;
}

Expand All @@ -37,11 +37,11 @@ export enum UrlParams {
const TopNav = ({
isChromeVisible,
savedDashboardInstance,
stateContainer,
appState,
dashboard,
currentAppState,
isEmbeddableRendered,
dashboardContainer,
currentContainer,
indexPatterns,
dashboardIdFromUrl,
}: DashboardTopNavProps) => {
Expand All @@ -57,11 +57,11 @@ const TopNav = ({

const handleRefresh = useCallback(
(_payload: any, isUpdate?: boolean) => {
if (!isUpdate && dashboardContainer) {
dashboardContainer.reload();
if (!isUpdate && currentContainer) {
currentContainer.reload();
}
},
[dashboardContainer]
[currentContainer]
);

const isEmbeddedExternally = Boolean(queryParameters.get('embed'));
Expand All @@ -78,12 +78,12 @@ const TopNav = ({
useEffect(() => {
if (isEmbeddableRendered) {
const navActions = getNavActions(
stateContainer,
appState,
savedDashboardInstance,
services,
dashboard,
dashboardIdFromUrl,
dashboardContainer
currentContainer
);
setTopNavMenu(
getTopNavConfig(
Expand All @@ -97,9 +97,9 @@ const TopNav = ({
currentAppState,
services,
dashboardConfig,
dashboardContainer,
currentContainer,
savedDashboardInstance,
stateContainer,
appState,
isEmbeddableRendered,
dashboard,
dashboardIdFromUrl,
Expand Down Expand Up @@ -139,7 +139,7 @@ const TopNav = ({
showSaveQuery={services.dashboardCapabilities.saveQuery as boolean}
savedQuery={undefined}
onSavedQueryIdChange={(savedQueryId?: string) => {
stateContainer.transitions.set('savedQuery', savedQueryId);
appState.transitions.set('savedQuery', savedQueryId);
}}
savedQueryId={currentAppState?.savedQuery}
onQuerySubmit={handleRefresh}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,9 @@ export class DashboardContainer extends Container<InheritedChildInput, Dashboard
public readonly type = DASHBOARD_CONTAINER_TYPE;

public renderEmpty?: undefined | (() => React.ReactNode);
public getChangesFromAppStateForContainerState?: (containerInput: any) => any;
public updateAppStateUrl?: undefined | ((pathname: string, replace: boolean) => void);
public updateAppStateUrl?:
| undefined
Copy link
Member

Choose a reason for hiding this comment

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

nit: You dont need this when you already have the ?

| (({ replace, pathname }: { replace: boolean; pathname?: string }) => void);

private embeddablePanel: EmbeddableStart['EmbeddablePanel'];

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
* under the License.
*/

import 'react-grid-layout/css/styles.css';
import 'react-resizable/css/styles.css';

// @ts-ignore
Expand All @@ -39,7 +38,7 @@ import classNames from 'classnames';
import _ from 'lodash';
import React from 'react';
import { Subscription } from 'rxjs';
import ReactGridLayout, { Layout } from 'react-grid-layout';
import ReactGridLayout, { Layout, ReactGridLayoutProps } from 'react-grid-layout';
import { GridData } from '../../../../common';
import { ViewMode, EmbeddableChildPanel, EmbeddableStart } from '../../../embeddable_plugin';
import { DASHBOARD_GRID_COLUMN_COUNT, DASHBOARD_GRID_HEIGHT } from '../dashboard_constants';
Expand Down Expand Up @@ -76,9 +75,9 @@ function ResponsiveGrid({
size: { width: number };
isViewMode: boolean;
layout: Layout[];
onLayoutChange: () => void;
onLayoutChange: ReactGridLayoutProps['onLayoutChange'];
children: JSX.Element[];
maximizedPanelId: string;
maximizedPanelId?: string;
useMargins: boolean;
}) {
// This is to prevent a bug where view mode changes when the panel is expanded. View mode changes will trigger
Expand Down Expand Up @@ -171,7 +170,7 @@ class DashboardGridUi extends React.Component<DashboardGridProps, State> {
let layout;
try {
layout = this.buildLayoutFromPanels();
} catch (error) {
} catch (error: any) {
console.error(error); // eslint-disable-line no-console

isLayoutInvalid = true;
Expand Down Expand Up @@ -283,6 +282,7 @@ class DashboardGridUi extends React.Component<DashboardGridProps, State> {
}}
>
<EmbeddableChildPanel
key={panel.type}
embeddableId={panel.explicitInput.id}
container={this.props.container}
PanelComponent={this.props.PanelComponent}
Expand All @@ -304,7 +304,7 @@ class DashboardGridUi extends React.Component<DashboardGridProps, State> {
isViewMode={isViewMode}
layout={this.buildLayoutFromPanels()}
onLayoutChange={this.onLayoutChange}
maximizedPanelId={this.state.expandedPanelId}
maximizedPanelId={this.state.expandedPanelId!}
Copy link
Member

Choose a reason for hiding this comment

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

why is the ! needed?

useMargins={this.state.useMargins}
>
{this.renderPanels()}
Expand Down
7 changes: 2 additions & 5 deletions src/plugins/dashboard/public/application/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,14 @@ import { DashboardServices } from '../types';
export * from './embeddable';
export * from './actions';

export const renderApp = (
{ element, appBasePath, onAppLeave }: AppMountParameters,
services: DashboardServices
) => {
export const renderApp = ({ element }: AppMountParameters, services: DashboardServices) => {
addHelpMenuToAppChrome(services.chrome, services.docLinks);

const app = (
<Router history={services.history}>
<OpenSearchDashboardsContextProvider services={services}>
<services.i18n.Context>
<DashboardApp onAppLeave={onAppLeave} />
<DashboardApp />
</services.i18n.Context>
</OpenSearchDashboardsContextProvider>
</Router>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,9 @@ export function migrateAppState(
delete appState.uiState;
}

appState.panels.forEach((panel) => {
panel.version = opensearchDashboardsVersion;
});
// appState.panels.forEach((panel) => {
Copy link
Member

Choose a reason for hiding this comment

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

Lets either remove this if its not necessary or add a comment here with a tracking issue that mentions how this can be removed.

// panel.version = opensearchDashboardsVersion;
// });

return appState;
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import { updateSavedDashboard } from './update_saved_dashboard';

import { DashboardAppStateContainer } from '../../types';
import { Dashboard } from '../../dashboard';
import { SavedObjectDashboard } from '../../saved_dashboards';

/**
* Saves the dashboard.
Expand All @@ -43,7 +44,7 @@ import { Dashboard } from '../../dashboard';
export function saveDashboard(
timeFilter: TimefilterContract,
stateContainer: DashboardAppStateContainer,
savedDashboard: any,
savedDashboard: SavedObjectDashboard,
Copy link
Member

Choose a reason for hiding this comment

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

Nice

saveOptions: SavedObjectSaveOpts,
dashboard: Dashboard
): Promise<string> {
Expand All @@ -54,7 +55,9 @@ export function saveDashboard(
// TODO: should update Dashboard class in the if(id) block
Copy link
Member

Choose a reason for hiding this comment

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

Is this TODO still necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's worth it to keep as we as a fast follow we should look into using redux slice to keep track of a dashboard and should be updated in this block.

return savedDashboard.save(saveOptions).then((id: string) => {
if (id) {
dashboard.id = id;
return id;
}
return id;
});
}
Loading