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

[D&D] Adds autosave while editing aggregation #1953

Merged
merged 3 commits into from
Jul 26, 2022
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
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,17 @@ import { SecondaryPanel } from './secondary_panel';

export function ConfigPanel() {
const vizType = useVisualizationType();
const draftAgg = useTypedSelector((state) => state.visualization.activeVisualization?.draftAgg);
const editingState = useTypedSelector(
(state) => state.visualization.activeVisualization?.draftAgg
);
const schemas = vizType.ui.containerConfig.data.schemas;

if (!schemas) return null;

const mainPanel = mapSchemaToAggPanel(schemas);

return (
<EuiForm className={`wizConfig ${draftAgg ? 'showSecondary' : ''}`}>
<EuiForm className={`wizConfig ${editingState ? 'showSecondary' : ''}`}>
<div className="wizConfig__section">{mainPanel}</div>
<SecondaryPanel />
</EuiForm>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,14 @@ import { useIndexPatterns, useVisualizationType } from '../../utils/use';
import { useOpenSearchDashboards } from '../../../../../opensearch_dashboards_react/public';
import { WizardServices } from '../../../types';
import { IAggType } from '../../../../../data/public';
import { saveAgg, editAgg } from '../../utils/state_management/visualization_slice';
import { saveDraftAgg, editDraftAgg } from '../../utils/state_management/visualization_slice';
import { setValid } from '../../utils/state_management/metadata_slice';

const EDITOR_KEY = 'CONFIG_PANEL';

export function SecondaryPanel() {
const draftAgg = useTypedSelector((state) => state.visualization.activeVisualization!.draftAgg);
const [valid, setValid] = useState(true);
const valid = useTypedSelector((state) => state.metadata.editorState.valid[EDITOR_KEY]);
const [touched, setTouched] = useState(false);
const dispatch = useTypedDispatch();
const vizType = useVisualizationType();
Expand Down Expand Up @@ -46,9 +49,26 @@ export function SecondaryPanel() {
const showAggParamEditor = !!(aggConfig && indexPattern);

const closeMenu = useCallback(() => {
// Save the agg if valid else discard
dispatch(saveAgg(valid));
}, [dispatch, valid]);
dispatch(editDraftAgg(undefined));
}, [dispatch]);

const handleSetValid = useCallback(
(isValid: boolean) => {
// Set validity state globally
dispatch(
setValid({
key: EDITOR_KEY,
valid: isValid,
})
);

// Autosave changes if valid
if (valid) {
dispatch(saveDraftAgg());
}
},
[dispatch, valid]
);
Comment on lines +55 to +71
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are isValid and valid supposed to be different? If yes, the naming can improve in a future change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, is isValid is the value passed to the callback function. valid is the value stored in state. Yeah I agree, naming can improve. Will take note to update it in the next PR :)


return (
<div className="wizConfig__section wizConfig--secondary">
Expand All @@ -58,28 +78,28 @@ export function SecondaryPanel() {
className="wizConfig__aggEditor"
agg={aggConfig!}
indexPattern={indexPattern!}
setValidity={setValid}
setValidity={handleSetValid}
setTouched={setTouched}
schemas={schemas}
formIsTouched={false}
groupName={selectedSchema?.group ?? 'none'}
metricAggs={[]}
state={{
data: {},
description: 'Falalala',
title: 'Title for the aggParams',
description: '',
title: '',
}}
setAggParamValue={function <T extends string | number | symbol>(
aggId: string,
paramName: T,
value: any
): void {
aggConfig.params[paramName] = value;
dispatch(editAgg(aggConfig.serialize()));
dispatch(editDraftAgg(aggConfig.serialize()));
}}
onAggTypeChange={function (aggId: string, aggType: IAggType): void {
aggConfig.type = aggType;
dispatch(editAgg(aggConfig.serialize()));
dispatch(editDraftAgg(aggConfig.serialize()));
}}
/>
)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { useTypedDispatch, useTypedSelector } from '../../../utils/state_managem
import { DropboxDisplay, DropboxProps } from '../dropbox';
import { useDrop } from '../../../utils/drag_drop';
import {
editAgg,
editDraftAgg,
reorderAgg,
updateAggConfigParams,
} from '../../../utils/state_management/visualization_slice';
Expand Down Expand Up @@ -88,18 +88,18 @@ export const useDropbox = (props: UseDropboxProps): DropboxProps => {
throw new Error('Missing new aggConfig');
}

dispatch(editAgg(newAggConfig.serialize()));
dispatch(editDraftAgg(newAggConfig.serialize()));
}, [aggConfigs, aggService, aggs, dispatch, indexPattern, schema.name]);

const onEditField = useCallback(
(aggId) => {
(aggId: string) => {
Copy link
Member

Choose a reason for hiding this comment

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

nice

const aggConfig = aggConfigs?.aggs.find((agg) => agg.id === aggId);

if (!aggConfig) {
throw new Error('Could not find agg in aggConfigs');
}

dispatch(editAgg(aggConfig.serialize()));
dispatch(editDraftAgg(aggConfig.serialize()));
},
[aggConfigs?.aggs, dispatch]
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ export const Workspace: FC = ({ children }) => {
) : (
<EuiFlexItem className="wizWorkspace__empty">
<EuiEmptyPrompt
title={<h2>Drop some fields to start</h2>}
title={<h2>Add a field to start</h2>}
body={
<>
<p>Drag a field to the configuration panel to generate a visualization.</p>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import { createSlice, PayloadAction } from '@reduxjs/toolkit';
import { WizardServices } from '../../../types';

export interface MetadataState {
editorState: {
valid: {
// Vaidity for each section in the editor
ashwin-pc marked this conversation as resolved.
Show resolved Hide resolved
[key: string]: boolean;
};
};
}

const initialState: MetadataState = {
editorState: {
valid: {},
},
};

export const getPreloadedState = async ({
types,
data,
}: WizardServices): Promise<MetadataState> => {
const preloadedState = { ...initialState };

return preloadedState;
};

export const slice = createSlice({
name: 'metadata',
initialState,
reducers: {
setValid: (state, action: PayloadAction<{ key: string; valid: boolean }>) => {
Copy link
Member

Choose a reason for hiding this comment

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

nit - I think it would be a little more clear if this was setIsValid/isValid, since we're using bools.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I'll fix the naming in a follow-up PR. Miki didn't like it the way it is too.

const { key, valid } = action.payload;
state.editorState.valid[key] = valid;
},
setState: (_state, action: PayloadAction<MetadataState>) => {
return action.payload;
},
},
});

export const { reducer } = slice;
export const { setValid, setState } = slice.actions;
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,19 @@ import { PreloadedState } from '@reduxjs/toolkit';
import { WizardServices } from '../../..';
import { getPreloadedState as getPreloadedStyleState } from './style_slice';
import { getPreloadedState as getPreloadedVisualizationState } from './visualization_slice';
import { getPreloadedState as getPreloadedMetadataState } from './metadata_slice';
import { RootState } from './store';

export const getPreloadedState = async (
services: WizardServices
): Promise<PreloadedState<RootState>> => {
const styleState = await getPreloadedStyleState(services);
const visualizationState = await getPreloadedVisualizationState(services);
const metadataState = await getPreloadedMetadataState(services);

return {
style: styleState,
visualization: visualizationState,
metadata: metadataState,
};
};
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,14 @@
import { combineReducers, configureStore, PreloadedState } from '@reduxjs/toolkit';
import { reducer as styleReducer } from './style_slice';
import { reducer as visualizationReducer } from './visualization_slice';
import { reducer as metadataReducer } from './metadata_slice';
import { WizardServices } from '../../..';
import { getPreloadedState } from './preload';

const rootReducer = combineReducers({
style: styleReducer,
visualization: visualizationReducer,
metadata: metadataReducer,
});

export const configurePreloadedStore = (preloadedState: PreloadedState<RootState>) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,15 +59,13 @@ export const slice = createSlice({
setSearchField: (state, action: PayloadAction<string>) => {
state.searchField = action.payload;
},
editAgg: (state, action: PayloadAction<CreateAggConfigParams>) => {
editDraftAgg: (state, action: PayloadAction<CreateAggConfigParams | undefined>) => {
state.activeVisualization!.draftAgg = action.payload;
Copy link
Member

Choose a reason for hiding this comment

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

does it need to be action?.payload to handle when the action is undefined?

Copy link
Member Author

Choose a reason for hiding this comment

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

Action can never be undefined. Action always has the type and payload properties. Payload however can be undefined

},
saveAgg: (state, action: PayloadAction<boolean>) => {
const saveDraft = action.payload;
saveDraftAgg: (state, action: PayloadAction<undefined>) => {
const draftAgg = state.activeVisualization!.draftAgg;

// Delete the aggConfigParam if the save is not true
if (saveDraft && draftAgg) {
if (draftAgg) {
const aggIndex = state.activeVisualization!.aggConfigParams.findIndex(
(agg) => agg.id === draftAgg.id
);
Expand All @@ -78,7 +76,6 @@ export const slice = createSlice({
state.activeVisualization!.aggConfigParams.splice(aggIndex, 1, draftAgg);
}
}
delete state.activeVisualization!.draftAgg;
},
reorderAgg: (
state,
Expand Down Expand Up @@ -116,9 +113,9 @@ export const {
setActiveVisualization,
setIndexPattern,
setSearchField,
editAgg,
editDraftAgg,
saveDraftAgg,
updateAggConfigParams,
saveAgg,
reorderAgg,
setState,
} = slice.actions;