-
Notifications
You must be signed in to change notification settings - Fork 918
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
Changes from all commits
63cd90a
3617277
dce7a65
6f420b0
7ee788a
074cca7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(); | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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] | ||
|
@@ -93,7 +107,7 @@ export function SecondaryPanel() { | |
schemas={schemas} | ||
formIsTouched={touched} | ||
groupName={selectedSchema?.group ?? 'none'} | ||
metricAggs={[]} | ||
metricAggs={metricAggs} | ||
state={{ | ||
data: {}, | ||
description: '', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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(() => { | ||
|
@@ -65,6 +69,7 @@ export const TopNav = () => { | |
config={config} | ||
setMenuMountPoint={setHeaderActionMenu} | ||
indexPatterns={indexPattern ? [indexPattern] : []} | ||
showDatePicker={!!indexPattern?.timeFieldName ?? true} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit - with the boolean cast, do you still need nullish coalescing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good call. will fix in followup :) |
||
showSearchBar | ||
showSaveQuery | ||
useDefaultBehaviors | ||
|
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"` | ||
); | ||
}); | ||
}); | ||
}); |
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
|
There was a problem hiding this comment.
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
vsrequiredBundles
?There was a problem hiding this comment.
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