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.

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

[D&D] Fixes autosave with debounce #1965

Merged
merged 4 commits into from
Jul 27, 2022
Merged

[D&D] Fixes autosave with debounce #1965

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e4860c0
fix: autosave editing
ashwin-pc Jul 27, 2022
cdc40c0
chore: improve workspace animation
ashwin-pc Jul 27, 2022
fc88b26
fix: show invalid field when editing
ashwin-pc Jul 27, 2022
ce083d2
fix: header offset
ashwin-pc Jul 27, 2022
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
2 changes: 1 addition & 1 deletion src/plugins/wizard/public/application/_variables.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
@import "@elastic/eui/src/global_styling/variables/header";
@import "@elastic/eui/src/global_styling/variables/form";

$osdHeaderOffset: $euiHeaderHeightCompensation * 2;
$osdHeaderOffset: $euiHeaderHeightCompensation;
$wizSideNavWidth: 470px;
4 changes: 4 additions & 0 deletions src/plugins/wizard/public/application/app.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,7 @@
"sideNav workspace";
height: calc(100vh - #{$osdHeaderOffset});
}

.headerIsExpanded .wizLayout {
height: calc(100vh - #{$osdHeaderOffset * 2});
}
Comment on lines +13 to +15
Copy link
Member

Choose a reason for hiding this comment

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

nice, thanks for fixing - beat me to it!

26 changes: 19 additions & 7 deletions src/plugins/wizard/public/application/components/data_tab/secondary_panel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import React, { useCallback, useMemo, useState } from 'react';
import { cloneDeep } from 'lodash';
import { useDebounce } from 'react-use';
import { useTypedDispatch, useTypedSelector } from '../../utils/state_management';
import { DefaultEditorAggParams } from '../../../../../vis_default_editor/public';
import { Title } from './title';
Expand All @@ -13,13 +14,15 @@ import { useOpenSearchDashboards } from '../../../../../opensearch_dashboards_re
import { WizardServices } from '../../../types';
import { IAggType } from '../../../../../data/public';
import { saveDraftAgg, editDraftAgg } from '../../utils/state_management/visualization_slice';
import { setValid } from '../../utils/state_management/metadata_slice';
import { setValidity } from '../../utils/state_management/metadata_slice';

const EDITOR_KEY = 'CONFIG_PANEL';

export function SecondaryPanel() {
const draftAgg = useTypedSelector((state) => state.visualization.activeVisualization!.draftAgg);
const valid = useTypedSelector((state) => state.metadata.editorState.valid[EDITOR_KEY]);
const isEditorValid = useTypedSelector(
(state) => state.metadata.editorState.validity[EDITOR_KEY]
);
const [touched, setTouched] = useState(false);
const dispatch = useTypedDispatch();
const vizType = useVisualizationType();
Expand Down Expand Up @@ -56,18 +59,27 @@ export function SecondaryPanel() {
(isValid: boolean) => {
// Set validity state globally
dispatch(
setValid({
setValidity({
key: EDITOR_KEY,
valid: isValid,
})
);
},
[dispatch]
);

// Autosave changes if valid
if (valid) {
// Autosave is agg value has changed and edits are valid
Copy link
Member

Choose a reason for hiding this comment

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

did you mean "Autosave if agg value..."?

useDebounce(
() => {
if (isEditorValid) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we check if isTouched?

Copy link
Member Author

Choose a reason for hiding this comment

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

isTouched is a weird flag that makes more sense in the legacy editor. It does not actually tell us if the editor was touched or not. Rather, it tells us if an invalid field was touched or not after we have detected that it is invalid. That isnt useful for the auto save though since we only want to know if the value has changed and if the fields are valid. we can tell that the value has changed based on the dependency array for the useDebounce function and if its valid based on the isEditorValid flag.

I'm setting touched to true if its not valid so that the editor displays the invalid field indicator. Dont ask me why, but thats how the DefaultEditorAggParams wants the prop :/

dispatch(saveDraftAgg());
} else {
// To indicate that an invalid edit was made
setTouched(true);
}
},
[dispatch, valid]
200,
[draftAgg, isEditorValid]
);

return (
Expand All @@ -81,7 +93,7 @@ export function SecondaryPanel() {
setValidity={handleSetValid}
setTouched={setTouched}
schemas={schemas}
formIsTouched={false}
formIsTouched={touched}
groupName={selectedSchema?.group ?? 'none'}
metricAggs={[]}
state={{
Expand Down
14 changes: 11 additions & 3 deletions src/plugins/wizard/public/application/components/workspace.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@
}

&__handFieldSvg {
animation: wizDragAnimation 2s ease-in-out infinite alternate;
animation: wizDragAnimation 6s ease-in-out infinite forwards;
Copy link
Member

Choose a reason for hiding this comment

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

do we have the specs for this are we kind of eyeballing it?

Copy link
Member Author

Choose a reason for hiding this comment

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

For now its eyeballing. Dont want it to be too distracting, which is why i changed it to include a pause and take longer to loop.

position: absolute;
top: 43%;
top: 34.5%;
}
}

Expand All @@ -27,7 +27,15 @@
transform: none;
}

30% {
transform: translate(116%, -80%);
}

60% {
transform: none;
}

100% {
transform: translate(65%, -30%);
transform: none;
}
}
10 changes: 5 additions & 5 deletions src/plugins/wizard/public/application/utils/state_management/metadata_slice.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { WizardServices } from '../../../types';

export interface MetadataState {
editorState: {
valid: {
validity: {
// Validity for each section in the editor
[key: string]: boolean;
};
Expand All @@ -17,7 +17,7 @@ export interface MetadataState {

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

Expand All @@ -34,9 +34,9 @@ export const slice = createSlice({
name: 'metadata',
initialState,
reducers: {
setValid: (state, action: PayloadAction<{ key: string; valid: boolean }>) => {
setValidity: (state, action: PayloadAction<{ key: string; valid: boolean }>) => {
const { key, valid } = action.payload;
state.editorState.valid[key] = valid;
state.editorState.validity[key] = valid;
},
setState: (_state, action: PayloadAction<MetadataState>) => {
return action.payload;
Expand All @@ -45,4 +45,4 @@ export const slice = createSlice({
});

export const { reducer } = slice;
export const { setValid, setState } = slice.actions;
export const { setValidity, setState } = slice.actions;