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

[Vis Builder] Misc Bar chart fixes #2401

Merged
merged 6 commits into from
Sep 28, 2022
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
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
### 📈 Features/Enhancements

### 🐛 Bug Fixes
* [Vis Builder] Fixes auto bounds for timeseries bar chart visualization ([2401](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2401))
* [Vis Builder] Fixes visualization shift when editing agg ([2401](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2401))
* [Vis Builder] Renames "Histogram" to "Bar" in vis type picker ([2401](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2401))

### 🚞 Infrastructure

Expand Down
18 changes: 9 additions & 9 deletions src/plugins/wizard/opensearch_dashboards.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,19 @@
"server": true,
"ui": true,
"requiredPlugins": [
"dashboard",
"data",
"embeddable",
"expressions",
"navigation",
"savedObjects",
"visualizations"
],
"requiredBundles": [
"charts",
"data",
"opensearchDashboardsReact",
"opensearchDashboardsUtils",
"savedObjects",
"embeddable",
"expressions",
"dashboard",
"visualizations",
"opensearchUiShared",
"visDefaultEditor",
"visTypeVislib"
],
Comment on lines +8 to -21
Copy link
Member

Choose a reason for hiding this comment

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

Nice! Do you think we need better docs about when to use requiredPlugins vs requiredBundles?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep! we should also add required bundles to the Plugin generation template too. I didnt know about them until you called it out

"optionalPlugins": []
]
}
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@
position: absolute;
top: 0;
left: 100%;

.visEditorAggParam--half {
margin: $euiSize 0;
}
}

&.showSecondary > .wizConfig__section {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@ 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 { draftAgg, aggConfigParams } = useTypedSelector(
(state) => state.visualization.activeVisualization!
);
const isEditorValid = useTypedSelector((state) => state.metadata.editor.validity[EDITOR_KEY]);
const [touched, setTouched] = useState(false);
const dispatch = useTypedDispatch();
Expand All @@ -39,9 +41,21 @@ export function SecondaryPanel() {
indexPattern && draftAgg && aggService.createAggConfigs(indexPattern, [cloneDeep(draftAgg)])
);
}, [draftAgg, aggService, indexPattern]);

const aggConfig = aggConfigs?.aggs[0];

const metricAggs = useMemo(
() =>
indexPattern
? aggService.createAggConfigs(
indexPattern,
cloneDeep(
aggConfigParams.filter((aggConfigParam) => aggConfigParam.schema === 'metric')
)
).aggs
: [],
[aggConfigParams, aggService, indexPattern]
);
Comment on lines +46 to +57
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a bit of a hack (to have metric visualization-specific logic here instead of in /visualizations/metric), but I may be misunderstanding what aggConfigParam.schema === 'metric' means in this context. A comment may be helpful to clarify either way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call, will add a comment in a followup PR, but yes metric aggregations are different from the metric visualization. Aggregations are classiied into 2 main categories, metric and bucket aggregations. The bar, line and area chart visualizations need to know if any metric aggregations are performed to correctly calculate the auto bounds of a histogram visualization.

Copy link
Member

Choose a reason for hiding this comment

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

ah, got it, that makes sense


const selectedSchema = useMemo(
() => schemas.find((schema) => schema.name === aggConfig?.schema),
[aggConfig?.schema, schemas]
Expand Down Expand Up @@ -93,7 +107,7 @@ export function SecondaryPanel() {
schemas={schemas}
formIsTouched={touched}
groupName={selectedSchema?.group ?? 'none'}
metricAggs={[]}
metricAggs={metricAggs}
state={{
data: {},
description: '',
Expand Down
23 changes: 14 additions & 9 deletions src/plugins/wizard/public/application/components/top_nav.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { useIndexPatterns, useSavedWizardVis } from '../utils/use';
import { useTypedSelector, useTypedDispatch } from '../utils/state_management';
import { setEditorState } from '../utils/state_management/metadata_slice';
import { useCanSave } from '../utils/use/use_can_save';
import { saveStateToSavedObject } from '../../saved_visualizations/transforms';

export const TopNav = () => {
// id will only be set for the edit route
Expand All @@ -32,26 +33,29 @@ export const TopNav = () => {

const saveDisabledReason = useCanSave();
const savedWizardVis = useSavedWizardVis(visualizationIdFromUrl);
const { selected: indexPattern } = useIndexPatterns();

const config = useMemo(() => {
if (savedWizardVis === undefined) return;

const { visualization: visualizationState, style: styleState } = rootState;
if (!savedWizardVis || !indexPattern) return;

return getTopNavConfig(
{
visualizationIdFromUrl,
savedWizardVis,
visualizationState,
styleState,
savedWizardVis: saveStateToSavedObject(savedWizardVis, rootState, indexPattern),
saveDisabledReason,
dispatch,
},
services
);
}, [rootState, savedWizardVis, services, visualizationIdFromUrl, saveDisabledReason, dispatch]);

const indexPattern = useIndexPatterns().selected;
}, [
savedWizardVis,
visualizationIdFromUrl,
rootState,
indexPattern,
saveDisabledReason,
dispatch,
services,
]);

// reset validity before component destroyed
useUnmount(() => {
Expand All @@ -65,6 +69,7 @@ export const TopNav = () => {
config={config}
setMenuMountPoint={setHeaderActionMenu}
indexPatterns={indexPattern ? [indexPattern] : []}
showDatePicker={!!indexPattern?.timeFieldName ?? true}
Copy link
Member

Choose a reason for hiding this comment

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

nit - with the boolean cast, do you still need nullish coalescing?

Copy link
Member Author

Choose a reason for hiding this comment

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

good call. will fix in followup :)

showSearchBar
showSaveQuery
useDefaultBehaviors
Expand Down
18 changes: 15 additions & 3 deletions src/plugins/wizard/public/application/components/workspace.scss
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

$animation-time: 3;
$animation-multiplier: 5;
$total-duartion: $animation-time * $animation-multiplier;
$keyframe-multiplier: 1 / $animation-multiplier;

.wizWorkspace {
display: grid;
-ms-grid-rows: auto $euiSizeM 1fr;
Expand All @@ -21,22 +27,28 @@
}

&__handFieldSvg {
animation: wizDragAnimation 6s ease-in-out infinite forwards;
animation: wizDragAnimation #{$total-duartion}s ease-in-out infinite forwards;
position: absolute;
top: 34.5%;
}
}

@media (prefers-reduced-motion) {
.wizWorkspace__handFieldSvg {
animation: none;
}
}
joshuarrrr marked this conversation as resolved.
Show resolved Hide resolved

@keyframes wizDragAnimation {
0% {
transform: none;
}

30% {
#{$keyframe-multiplier * 50%} {
transform: translate(116%, -80%);
}

60% {
#{$keyframe-multiplier * 100%} {
transform: none;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,33 +38,19 @@ import {
} from '../../../../saved_objects/public';
import { WizardServices } from '../..';
import { WizardVisSavedObject } from '../../types';
import { StyleState, VisualizationState, AppDispatch } from './state_management';
import { AppDispatch } from './state_management';
import { EDIT_PATH } from '../../../common';
import { setEditorState } from './state_management/metadata_slice';
interface TopNavConfigParams {
visualizationIdFromUrl: string;
savedWizardVis: WizardVisSavedObject;
visualizationState: VisualizationState;
styleState: StyleState;
saveDisabledReason?: string;
dispatch: AppDispatch;
}

export const getTopNavConfig = (
{
visualizationIdFromUrl,
savedWizardVis,
visualizationState,
styleState,
saveDisabledReason,
dispatch,
}: TopNavConfigParams,
{
history,
toastNotifications,
i18n: { Context: I18nContext },
data: { indexPatterns },
}: WizardServices
{ visualizationIdFromUrl, savedWizardVis, saveDisabledReason, dispatch }: TopNavConfigParams,
{ history, toastNotifications, i18n: { Context: I18nContext } }: WizardServices
) => {
const topNavConfig: TopNavMenuData[] = [
{
Expand Down Expand Up @@ -94,16 +80,6 @@ export const getTopNavConfig = (
return;
}
const currentTitle = savedWizardVis.title;
const indexPattern = await indexPatterns.get(visualizationState.indexPattern || '');
savedWizardVis.searchSourceFields = {
index: indexPattern,
};
const vizStateWithoutIndex = {
searchField: visualizationState.searchField,
activeVisualization: visualizationState.activeVisualization,
};
savedWizardVis.visualizationState = JSON.stringify(vizStateWithoutIndex);
savedWizardVis.styleState = JSON.stringify(styleState);
savedWizardVis.title = newTitle;
savedWizardVis.description = newDescription;
savedWizardVis.copyOnSave = newCopyOnSave;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ export const useIndexPatterns = () => {
services: { data },
} = useOpenSearchDashboards<WizardServices>();

let foundSelected: IndexPattern;
let foundSelected: IndexPattern | undefined;
if (!loading && !error) {
foundSelected = indexPatterns.filter((p) => p.id === indexId)[0];
if (foundSelected === undefined) {
Expand Down Expand Up @@ -47,6 +47,6 @@ export const useIndexPatterns = () => {
indexPatterns,
error,
loading,
selected: foundSelected!,
selected: foundSelected,
};
};
4 changes: 2 additions & 2 deletions src/plugins/wizard/public/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import { EDIT_PATH, PLUGIN_ID, PLUGIN_NAME, WIZARD_SAVED_OBJECT } from '../commo
import { TypeService } from './services/type_service';
import { getPreloadedStore } from './application/utils/state_management';
import {
setAggService,
setSearchService,
setIndexPatterns,
setHttp,
setSavedWizardLoader,
Expand Down Expand Up @@ -150,7 +150,7 @@ export class WizardPlugin
});

// Register plugin services
setAggService(data.search.aggs);
setSearchService(data.search);
setExpressionLoader(expressions.ExpressionLoader);
setReactExpressionRenderer(expressions.ReactExpressionRenderer);
setHttp(core.http);
Expand Down
6 changes: 3 additions & 3 deletions src/plugins/wizard/public/plugin_services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@ import { HttpStart, IUiSettingsClient } from '../../../core/public';
import { ExpressionsStart } from '../../expressions/public';
import { TypeServiceStart } from './services/type_service';

export const [getAggService, setAggService] = createGetterSetter<
DataPublicPluginStart['search']['aggs']
>('data.search.aggs');
export const [getSearchService, setSearchService] = createGetterSetter<
DataPublicPluginStart['search']
>('data.search');

export const [getExpressionLoader, setExpressionLoader] = createGetterSetter<
ExpressionsStart['ExpressionLoader']
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import { coreMock } from '../../../../core/public/mocks';
import { getStubIndexPattern } from '../../../../plugins/data/public/test_utils';
import { IndexPattern } from '../../../data/public';
import { RootState } from '../application/utils/state_management';
import { WizardVisSavedObject } from '../types';
import { saveStateToSavedObject } from './transforms';

const getConfig = (cfg: any) => cfg;

describe('transforms', () => {
describe('saveStateToSavedObject', () => {
let TEST_INDEX_PATTERN_ID;
let savedObject;
let rootState: RootState;
let indexPattern: IndexPattern;

beforeEach(() => {
TEST_INDEX_PATTERN_ID = 'test-pattern';
savedObject = {} as WizardVisSavedObject;
rootState = {
metadata: { editor: { state: 'loading', validity: {} } },
style: '',
visualization: {
searchField: '',
indexPattern: TEST_INDEX_PATTERN_ID,
activeVisualization: {
name: 'bar',
aggConfigParams: [],
},
},
};
indexPattern = getStubIndexPattern(
TEST_INDEX_PATTERN_ID,
getConfig,
null,
[],
coreMock.createSetup()
);
});

test('should save root state information into saved object', async () => {
saveStateToSavedObject(savedObject, rootState, indexPattern);

expect(savedObject.visualizationState).not.toContain(TEST_INDEX_PATTERN_ID);
expect(savedObject.styleState).toEqual(JSON.stringify(rootState.style));
expect(savedObject.searchSourceFields?.index?.id).toEqual(TEST_INDEX_PATTERN_ID);
});

test('should fail if the index pattern does not match the value on state', () => {
rootState.visualization.indexPattern = 'Some-other-pattern';

expect(() =>
saveStateToSavedObject(savedObject, rootState, indexPattern)
).toThrowErrorMatchingInlineSnapshot(
`"indexPattern id should match the value in redux state"`
);
});
});
});
28 changes: 28 additions & 0 deletions src/plugins/wizard/public/saved_visualizations/transforms.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import produce from 'immer';
import { IndexPattern } from '../../../data/public';
import { RootState, VisualizationState } from '../application/utils/state_management';
import { WizardVisSavedObject } from '../types';

export const saveStateToSavedObject = (
obj: WizardVisSavedObject,
state: RootState,
indexPattern: IndexPattern
): WizardVisSavedObject => {
if (state.visualization.indexPattern !== indexPattern.id)
throw new Error('indexPattern id should match the value in redux state');

obj.visualizationState = JSON.stringify(
produce(state.visualization, (draft: VisualizationState) => {
delete draft.indexPattern;
})
);
obj.styleState = JSON.stringify(state.style);
obj.searchSourceFields = { index: indexPattern };

return obj;
};
joshuarrrr marked this conversation as resolved.
Show resolved Hide resolved
Loading