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

Jump to bottom

[D&D] Fix: Topnav updates aggregation parameters #1905

Merged
merged 2 commits into from
Jul 20, 2022
Merged

[D&D] Fix: Topnav updates aggregation parameters #1905

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e06dbd1
fix(D&D): Fixes top nav query and timerange
ashwin-pc Jul 15, 2022
b03fe59
fix(D&D): Handles topnav state update
ashwin-pc Jul 17, 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
6 changes: 3 additions & 3 deletions src/plugins/wizard/public/application/components/top_nav.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,10 @@ export const TopNav = () => {
appName={PLUGIN_ID}
config={config}
setMenuMountPoint={setHeaderActionMenu}
showSearchBar={true}
useDefaultBehaviors={true}
screenTitle="Test"
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 still need a defined screen title or does it default to the app name or something?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's an optional parameter that can be the title of the visualization but it can be ignored. When ignored the title is OpenSearch Dashboards. We can add it in future when we have the saved object stuff all polished up.

indexPatterns={indexPattern ? [indexPattern] : []}
showSearchBar
showSaveQuery
useDefaultBehaviors
/>
</div>
);
Expand Down
29 changes: 25 additions & 4 deletions src/plugins/wizard/public/application/components/workspace.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,9 @@ import {
EuiPanel,
EuiPopover,
} from '@elastic/eui';
import React, { FC, useState, useMemo, useEffect, Fragment } from 'react';
import React, { FC, useState, useMemo, useEffect, useLayoutEffect } from 'react';
import { useOpenSearchDashboards } from '../../../../opensearch_dashboards_react/public';
import { IExpressionLoaderParams } from '../../../../expressions/public';
import { WizardServices } from '../../types';
import { validateSchemaState } from '../utils/validate_schema_state';
import { useTypedDispatch, useTypedSelector } from '../utils/state_management';
Expand All @@ -32,10 +33,16 @@ export const Workspace: FC = ({ children }) => {
services: {
expressions: { ReactExpressionRenderer },
notifications: { toasts },
data,
},
} = useOpenSearchDashboards<WizardServices>();
const { toExpression, ui } = useVisualizationType();
const [expression, setExpression] = useState<string>();
const [searchContext, setSearchContext] = useState<IExpressionLoaderParams['searchContext']>({
query: data.query.queryString.getQuery(),
filters: data.query.filterManager.getFilters(),
timeRange: data.query.timefilter.timefilter.getTime(),
});
const rootState = useTypedSelector((state) => state);

useEffect(() => {
Expand All @@ -57,6 +64,20 @@ export const Workspace: FC = ({ children }) => {
loadExpression();
}, [rootState, toExpression, toasts, ui.containerConfig.data.schemas]);

useLayoutEffect(() => {
const subscription = data.query.state$.subscribe(({ state }) => {
setSearchContext({
query: state.query,
timeRange: state.time,
filters: state.filters,
});
});

return () => {
subscription.unsubscribe();
};
}, [data.query.state$]);

return (
<section className="wizWorkspace">
<EuiFlexGroup className="wizCanvasControls">
Expand All @@ -66,13 +87,13 @@ export const Workspace: FC = ({ children }) => {
</EuiFlexGroup>
<EuiPanel className="wizCanvas">
{expression ? (
<ReactExpressionRenderer expression={expression} />
<ReactExpressionRenderer expression={expression} searchContext={searchContext} />
Copy link
Member

@ruanyl ruanyl Jul 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

ReactExpressionRenderer has a prop reload$:

An observable which can be used to re-run the expression without destroying the component

Does it make sense to use that instead of prop searchContext?
nvm, it seems the loader requires searchContext anyway.

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, for now either the searchContext or expression will need to change causing a reload anyways. But thanks for calling that out. This will be useful in instances where neither change but you still want to trigger a reload.

) : (
<EuiFlexItem className="wizWorkspace__empty">
<EuiEmptyPrompt
title={<h2>Drop some fields here to start</h2>}
body={
<Fragment>
<>
<p>Drag a field directly to the canvas or axis to generate a visualization.</p>
<span className="wizWorkspace__container">
<EuiIcon className="wizWorkspace__fieldSvg" type={fields_bg} size="original" />
Expand All @@ -82,7 +103,7 @@ export const Workspace: FC = ({ children }) => {
size="original"
/>
</span>
</Fragment>
</>
}
/>
</EuiFlexItem>
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/wizard/public/visualizations/metric/to_expression.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,5 +171,5 @@ export const toExpression = async ({ style: styleState, visualization }: MetricR

const ast = buildExpression([opensearchaggs, metricVis]);

return ast.toString();
return `opensearchDashboards | opensearch_dashboards_context | ${ast.toString()}`;
Copy link
Member

Choose a reason for hiding this comment

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

I assume this change is intended, and maybe it's obvious for someone who understands expressions, but an explanatory comment would seem useful.

Copy link
Member Author

Choose a reason for hiding this comment

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

The toExpression function needs some TLC as a whole. I plan on refactoring this when we have more than one viz type. opensearchDashboards | opensearch_dashboards_context are the expression functions that get the top nav context and pass it along to the agg function. For now ill leave it as is since this is how the build_pipeline.ts does it too.

I want to abstract away most of this logic into utility functions so that all we have here is some sort of builder function that simply chains the expression function builders.

};