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

[Discover-next] add datasource container #7157

Merged
merged 2 commits into from
Jul 2, 2024
Merged

[Discover-next] add datasource container #7157

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
2a59f2a
[Discover-next] add datasource container
kavilla Jul 2, 2024
e751ed6
Changeset file for PR #7157 created/updated
opensearch-changeset-bot[bot] Jul 2, 2024
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: 2 additions & 0 deletions changelogs/fragments/7157.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
feat:
- Query editor and dataframes datasources container ([#7157](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/7157))
16 changes: 16 additions & 0 deletions src/plugins/data/public/ui/query_editor/_query_editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,22 @@
}

.osdQueryEditor__dataSourceWrapper {
.dataSourceSelect {
border-bottom: $euiBorderThin !important;

:first-child {
box-shadow: none !important;
height: 100%;
border-radius: 0;
}

div:is([class$="--group"]) {
padding: 0 !important;
Copy link
Member

Choose a reason for hiding this comment

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

i see !important is used in many places already, is it avoidable?

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 actually the OUI combo box. I would like to avoid it but some of the classes everywhere have like added padded so this is to avoid the height of the flex box getting pushed around

}
}
}

.osdQueryEditor__dataSetWrapper {
.dataExplorerDSSelect {
border-bottom: $euiBorderThin !important;

Expand Down
31 changes: 26 additions & 5 deletions src/plugins/data/public/ui/query_editor/query_editor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ export interface QueryEditorProps {
indexPatterns: Array<IIndexPattern | string>;
dataSource?: DataSource;
query: Query;
dataSourceContainerRef?: React.RefCallback<HTMLDivElement>;
containerRef?: React.RefCallback<HTMLDivElement>;
settings: Settings;
disableAutoFocus?: boolean;
Expand Down Expand Up @@ -54,6 +55,7 @@ interface Props extends QueryEditorProps {

interface State {
isDataSourcesVisible: boolean;
isDataSetsVisible: boolean;
isSuggestionsVisible: boolean;
index: number | null;
suggestions: QuerySuggestion[];
Expand All @@ -77,7 +79,8 @@ const KEY_CODES = {
// eslint-disable-next-line import/no-default-export
export default class QueryEditorUI extends Component<Props, State> {
public state: State = {
isDataSourcesVisible: true,
isDataSourcesVisible: false,
isDataSetsVisible: true,
isSuggestionsVisible: false,
index: null,
suggestions: [],
Expand Down Expand Up @@ -212,7 +215,10 @@ export default class QueryEditorUI extends Component<Props, State> {
: undefined;
this.onChange(newQuery, dateRange);
this.onSubmit(newQuery, dateRange);
this.setState({ isDataSourcesVisible: enhancement?.searchBar?.showDataSourceSelector ?? true });
this.setState({ isDataSetsVisible: enhancement?.searchBar?.showDataSetsSelector ?? true });
this.setState({
isDataSourcesVisible: enhancement?.searchBar?.showDataSourcesSelector ?? true,
});
Comment on lines +218 to +221
Copy link
Member

Choose a reason for hiding this comment

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

is it better to combine the setStates into one call? I forgot if react optimizes this

Copy link
Member Author

Choose a reason for hiding this comment

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

you're right that would be the better approach i'll fast follow

};

private initPersistedLog = () => {
Expand All @@ -227,10 +233,19 @@ export default class QueryEditorUI extends Component<Props, State> {

const isDataSourcesVisible =
this.props.settings.getQueryEnhancements(this.props.query.language)?.searchBar
?.showDataSourceSelector ?? true;
?.showDataSourcesSelector ?? true;
this.setState({ isDataSourcesVisible });
};

private initDataSetsVisibility = () => {
if (this.componentIsUnmounting) return;

const isDataSetsVisible =
this.props.settings.getQueryEnhancements(this.props.query.language)?.searchBar
?.showDataSetsSelector ?? true;
this.setState({ isDataSetsVisible });
};

public onMouseEnterSuggestion = (index: number) => {
this.setState({ index });
};
Expand All @@ -246,6 +261,7 @@ export default class QueryEditorUI extends Component<Props, State> {
this.initPersistedLog();
// this.fetchIndexPatterns().then(this.updateSuggestions);
this.initDataSourcesVisibility();
this.initDataSetsVisibility();
}

public componentDidUpdate(prevProps: Props) {
Expand Down Expand Up @@ -280,6 +296,11 @@ export default class QueryEditorUI extends Component<Props, State> {
<EuiFlexItem grow={false}>
<EuiFlexGroup gutterSize="xs" alignItems="center" className={`${className}__wrapper`}>
<EuiFlexItem grow={false}>{this.props.prepend}</EuiFlexItem>
{this.state.isDataSourcesVisible && (
<EuiFlexItem grow={false} className={`${className}__dataSourceWrapper`}>
<div ref={this.props.dataSourceContainerRef} />
Copy link
Member

Choose a reason for hiding this comment

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

what's the consideration to expose a ref rather than directly rendering the component here?

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks for asking this one:

i'm using the data source management plugin for that component. so to avoid a circular dependency and not introduce datasourcemanagements as a required bundle. i do it in the query enhancements which can take that optional depedency

</EuiFlexItem>
)}
<EuiFlexItem grow={false} className={`${className}__languageWrapper`}>
<QueryLanguageSelector
language={this.props.query.language}
Expand All @@ -288,8 +309,8 @@ export default class QueryEditorUI extends Component<Props, State> {
appName={this.services.appName}
/>
</EuiFlexItem>
{this.state.isDataSourcesVisible && (
<EuiFlexItem grow={false} className={`${className}__dataSourceWrapper`}>
{this.state.isDataSetsVisible && (
Copy link
Member

Choose a reason for hiding this comment

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

should this be isDataSetVisible? I see some are plural some are singular, do we plan to allow multiple dataSets/dataSources on UI or just one?

Copy link
Member Author

Choose a reason for hiding this comment

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

scared me. had to check. yeah i modified it for this because plural makes sense for me like its the selector which can select multiple datasets. But tbh, I was on the fence too. I just wanted to updated in this file so its both plural.

Copy link
Member

Choose a reason for hiding this comment

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

got it, it's minor but i wasn't sure if it's on purpose. if it's plural should it be areDataSetsVisible or dataSetsVisible?

<EuiFlexItem grow={false} className={`${className}__dataSetWrapper`}>
<div ref={this.props.containerRef} />
</EuiFlexItem>
)}
Expand Down
4 changes: 3 additions & 1 deletion src/plugins/data/public/ui/query_editor/query_editor_top_row.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {
} from '@elastic/eui';
import classNames from 'classnames';
import { compact, isEqual } from 'lodash';
import React, { useRef, useState } from 'react';
import React, { useState } from 'react';
import {
DataSource,
IDataPluginServices,
Expand All @@ -38,6 +38,7 @@ const QueryEditor = withOpenSearchDashboards(QueryEditorUI);
// @internal
export interface QueryEditorTopRowProps {
query?: Query;
dataSourceContainerRef?: React.RefCallback<HTMLDivElement>;
containerRef?: React.RefCallback<HTMLDivElement>;
settings?: Settings;
onSubmit: (payload: { dateRange: TimeRange; query?: Query }) => void;
Expand Down Expand Up @@ -234,6 +235,7 @@ export default function QueryEditorTopRow(props: QueryEditorTopRowProps) {
dataSource={props.dataSource}
prepend={props.prepend}
query={parsedQuery}
dataSourceContainerRef={props.dataSourceContainerRef}
containerRef={props.containerRef}
settings={props.settings!}
screenTitle={props.screenTitle}
Expand Down
9 changes: 9 additions & 0 deletions src/plugins/data/public/ui/search_bar/create_search_bar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
data: Omit<DataPublicPluginStart, 'ui'>;
storage: IStorageWrapper;
settings: Settings;
setDataSourceContainerRef: (ref: HTMLDivElement | null) => void;
setContainerRef: (ref: HTMLDivElement | null) => void;
}

Expand Down Expand Up @@ -138,6 +139,7 @@
storage,
data,
settings,
setDataSourceContainerRef,
setContainerRef,
}: StatefulSearchBarDeps) {
// App name should come from the core application service.
Expand Down Expand Up @@ -174,6 +176,12 @@
notifications: core.notifications,
});

const dataSourceContainerRef = useCallback((node) => {

Check warning on line 179 in src/plugins/data/public/ui/search_bar/create_search_bar.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/data/public/ui/search_bar/create_search_bar.tsx#L179 

Added line #L179 was not covered by tests
if (node) {
setDataSourceContainerRef(node);

Check warning on line 181 in src/plugins/data/public/ui/search_bar/create_search_bar.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/data/public/ui/search_bar/create_search_bar.tsx#L181 

Added line #L181 was not covered by tests
}
}, []);

const containerRef = useCallback((node) => {
if (node) {
setContainerRef(node);
Expand Down Expand Up @@ -220,6 +228,7 @@
filters={filters}
query={query}
settings={settings}
dataSourceContainerRef={dataSourceContainerRef}
containerRef={containerRef}
onFiltersUpdated={defaultFiltersUpdated(data.query)}
onRefreshChange={defaultOnRefreshChange(data.query)}
Expand Down
2 changes: 2 additions & 0 deletions src/plugins/data/public/ui/search_bar/search_bar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ export interface SearchBarOwnProps {
// Query bar - should be in SearchBarInjectedDeps
query?: Query;
settings?: Settings;
dataSourceContainerRef?: React.RefCallback<HTMLDivElement>;
containerRef?: React.RefCallback<HTMLDivElement>;
// Show when user has privileges to save
showSaveQuery?: boolean;
Expand Down Expand Up @@ -490,6 +491,7 @@ class SearchBarUI extends Component<SearchBarProps, State> {
queryEditor = (
<QueryEditorTopRow
timeHistory={this.props.timeHistory}
dataSourceContainerRef={this.props.dataSourceContainerRef}
containerRef={this.props.containerRef}
settings={this.props.settings}
query={this.state.query}
Expand Down
4 changes: 3 additions & 1 deletion src/plugins/data/public/ui/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ export interface QueryEnhancement {
// Leave blank to support all data sources
// supportedDataSourceTypes?: Record<string, GenericDataSource>;
searchBar?: {
showDataSourceSelector?: boolean;
showDataSetsSelector?: boolean;
showDataSourcesSelector?: boolean;
showQueryInput?: boolean;
showFilterBar?: boolean;
showDatePicker?: boolean;
Expand Down Expand Up @@ -66,5 +67,6 @@ export interface IUiStart {
SearchBar: React.ComponentType<StatefulSearchBarProps>;
SuggestionsComponent: React.ComponentType<SuggestionsComponentProps>;
Settings: Settings;
dataSourceContainer$: Observable<HTMLDivElement | null>;
container$: Observable<HTMLDivElement | null>;
}
7 changes: 7 additions & 0 deletions src/plugins/data/public/ui/ui_service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
enhancementsConfig: ConfigSchema['enhancements'];
private queryEnhancements: Map<string, QueryEnhancement> = new Map();
private queryEditorExtensionMap: Record<string, QueryEditorExtensionConfig> = {};
private dataSourceContainer$ = new BehaviorSubject<HTMLDivElement | null>(null);

Check warning on line 32 in src/plugins/data/public/ui/ui_service.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/data/public/ui/ui_service.ts#L32 

Added line #L32 was not covered by tests
private container$ = new BehaviorSubject<HTMLDivElement | null>(null);

constructor(initializerContext: PluginInitializerContext<ConfigSchema>) {
Expand Down Expand Up @@ -62,6 +63,10 @@
queryEditorExtensionMap: this.queryEditorExtensionMap,
});

const setDataSourceContainerRef = (ref: HTMLDivElement | null) => {
this.dataSourceContainer$.next(ref);

Check warning on line 67 in src/plugins/data/public/ui/ui_service.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/data/public/ui/ui_service.ts#L66-L67 

Added lines #L66 - L67 were not covered by tests
};

const setContainerRef = (ref: HTMLDivElement | null) => {
this.container$.next(ref);
};
Expand All @@ -71,6 +76,7 @@
data: dataServices,
storage,
settings: Settings,
setDataSourceContainerRef,
setContainerRef,
});

Expand All @@ -79,6 +85,7 @@
SearchBar,
SuggestionsComponent,
Settings,
dataSourceContainer$: this.dataSourceContainer$,
container$: this.container$,
};
}
Expand Down
Loading