[Discover-next] add datasource container #7157

kavilla · 2024-07-02T19:30:06Z

Description

Enables the editor to let plugins to mount their own data source component to the data source container.

Related to:
kavilla/queryEnhancements#29

Issues Partially Resolved

#7129

Screenshot

Testing the changes

yarn start:enhancements

Changelog

feat: query editor and dataframes datasources container

Check List

All tests pass
- yarn test:jest
- yarn test:jest_integration
New functionality includes testing.
New functionality has been documented.
Update CHANGELOG.md
Commits are signed per the DCO using --signoff

Enables the editor to let plugins to mount their own data source component to the data source container. Issue partially resolved: opensearch-project#7129 Signed-off-by: Kawika Avilla <kavilla414@gmail.com>

codecov · 2024-07-02T19:45:33Z

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Project coverage is 67.48%. Comparing base (5212f09) to head (e751ed6).
Report is 359 commits behind head on main.

Files with missing lines	Patch %	Lines
...ns/data/public/ui/search_bar/create_search_bar.tsx	0.00%	3 Missing ⚠️
src/plugins/data/public/ui/ui_service.ts	0.00%	3 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #7157    +/-   ##
========================================
  Coverage   67.47%   67.48%            
========================================
  Files        3451     3468    +17     
  Lines       68060    68372   +312     
  Branches    11068    11111    +43     
========================================
+ Hits        45926    46143   +217     
- Misses      19455    19529    +74     
- Partials     2679     2700    +21

Flag	Coverage Δ
Linux_1	`33.04% <0.00%> (-0.01%)`	⬇️
Linux_2	`55.18% <ø> (+<0.01%)`	⬆️
Linux_3	`45.34% <0.00%> (+0.20%)`	⬆️
Linux_4	`34.76% <0.00%> (+<0.01%)`	⬆️
Windows_1	`33.06% <0.00%> (-0.01%)`	⬇️
Windows_2	`55.14% <ø> (+<0.01%)`	⬆️
Windows_3	`45.35% <0.00%> (+0.20%)`	⬆️
Windows_4	`34.76% <0.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

joshuali925

just some questions

joshuali925 · 2024-07-02T21:15:06Z

src/plugins/data/public/ui/query_editor/query_editor.tsx

+    this.setState({ isDataSetsVisible: enhancement?.searchBar?.showDataSetsSelector ?? true });
+    this.setState({
+      isDataSourcesVisible: enhancement?.searchBar?.showDataSourcesSelector ?? true,
+    });


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

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

joshuali925 · 2024-07-02T21:16:29Z

src/plugins/data/public/ui/query_editor/query_editor.tsx

@@ -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} />


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

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

joshuali925 · 2024-07-02T21:18:34Z

src/plugins/data/public/ui/query_editor/query_editor.tsx

@@ -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 && (


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?

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.

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?

joshuali925 · 2024-07-02T21:19:30Z

src/plugins/data/public/ui/query_editor/_query_editor.scss

+    }
+
+    div:is([class$="--group"]) {
+      padding: 0 !important;


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

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

Added data source selector that pulls from saved objects and also added a connections service and passing it with the dataframe. Related to: opensearch-project/OpenSearch-Dashboards#7157

* [Discover-next] add datasource container Enables the editor to let plugins to mount their own data source component to the data source container. Issue partially resolved: #7129 Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * Changeset file for PR #7157 created/updated --------- Signed-off-by: Kawika Avilla <kavilla414@gmail.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> (cherry picked from commit e1c5cfd) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* [Discover-next] add datasource container Enables the editor to let plugins to mount their own data source component to the data source container. Issue partially resolved: #7129 * Changeset file for PR #7157 created/updated --------- (cherry picked from commit e1c5cfd) Signed-off-by: Kawika Avilla <kavilla414@gmail.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>

…pensearch-project#7164) * [Discover-next] add datasource container Enables the editor to let plugins to mount their own data source component to the data source container. Issue partially resolved: opensearch-project#7129 * Changeset file for PR opensearch-project#7157 created/updated --------- (cherry picked from commit e1c5cfd) Signed-off-by: Kawika Avilla <kavilla414@gmail.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>

[Discover-next] add datasource container

2a59f2a

Enables the editor to let plugins to mount their own data source component to the data source container. Issue partially resolved: opensearch-project#7129 Signed-off-by: Kawika Avilla <kavilla414@gmail.com>

kavilla requested review from ananzh, AMoo-Miki, ashwin-pc, joshuarrrr, abbyhu2000, zengyan-amazon, zhongnansu, manasvinibs, ZilongX, Flyingliuhub, curq, bandinib-amzn, SuZhou-Joe, ruanyl, BionIT, xinruiba, zhyuanqi, mengweieric and LDrago27 as code owners July 2, 2024 19:30

github-actions bot added the distinguished-contributor label Jul 2, 2024

kavilla mentioned this pull request Jul 2, 2024

[MDS] datasource selector added kavilla/queryEnhancements#29

Merged

Changeset file for PR opensearch-project#7157 created/updated

e751ed6

kavilla added backport 2.x v2.16.0 discover-next labels Jul 2, 2024

ashwin-pc approved these changes Jul 2, 2024

View reviewed changes

abbyhu2000 approved these changes Jul 2, 2024

View reviewed changes

joshuali925 reviewed Jul 2, 2024

View reviewed changes

kavilla merged commit e1c5cfd into opensearch-project:main Jul 2, 2024
67 of 68 checks passed

opensearch-trigger-bot bot mentioned this pull request Jul 3, 2024

[Backport 2.x] [Discover-next] add datasource container #7164

Merged

ananzh mentioned this pull request Jul 24, 2024

Missing backports from current 2.x to 2.16 #7466

Closed

7 tasks

AMoo-Miki added the in-next label Sep 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover-next] add datasource container #7157

[Discover-next] add datasource container #7157

kavilla commented Jul 2, 2024 •

edited

Loading

codecov bot commented Jul 2, 2024 •

edited

Loading

joshuali925 left a comment

joshuali925 Jul 2, 2024

kavilla Jul 2, 2024

joshuali925 Jul 2, 2024

kavilla Jul 2, 2024

joshuali925 Jul 2, 2024

kavilla Jul 2, 2024

joshuali925 Jul 2, 2024

joshuali925 Jul 2, 2024

kavilla Jul 2, 2024

[Discover-next] add datasource container #7157

[Discover-next] add datasource container #7157

Conversation

kavilla commented Jul 2, 2024 • edited Loading

Description

Issues Partially Resolved

Screenshot

Testing the changes

Changelog

Check List

codecov bot commented Jul 2, 2024 • edited Loading

Codecov Report

joshuali925 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kavilla commented Jul 2, 2024 •

edited

Loading

codecov bot commented Jul 2, 2024 •

edited

Loading