[Vis Builder] Misc Bar chart fixes #2401

ashwin-pc · 2022-09-22T08:20:09Z

Description

This fixes the following issues with the VisBuilder bar chart:

Fixes timeseries bar chart visualization using src/plugins/wizard/public/visualizations/vislib/common/create_vis.ts
Fixes visualization shift when editing agg by passing metricAggs in src/plugins/wizard/public/application/components/data_tab/secondary_panel.tsx
Renames "Histogram" to "Bar" in vis type picker since it is more accurate and histogram is a subset of Bar chart visualizations

Misc styling fixes in

src/plugins/wizard/public/application/components/workspace.scss: Reduces animation impact
src/plugins/wizard/public/application/components/data_tab/config_panel.scss: fixes incorrect margin for custom styled visEditorAggParam

Misc improvements:

Consolidates transform from redux state to saved object: src/plugins/wizard/public/saved_visualizations/transforms.ts
Removes unused plugins to requiredBundles in src/plugins/wizard/opensearch_dashboards.json

The changes are split by commits.

Issues Resolved

Check List

codecov-commenter · 2022-09-22T09:01:52Z

Codecov Report

Merging #2401 (ddbc454) into main (08d0504) will increase coverage by 0.00%.
The diff coverage is 29.41%.

@@           Coverage Diff           @@
##             main    #2401   +/-   ##
=======================================
  Coverage   66.75%   66.75%           
=======================================
  Files        3194     3196    +2     
  Lines       60803    60813   +10     
  Branches     9238     9240    +2     
=======================================
+ Hits        40587    40596    +9     
- Misses      18008    18009    +1     
  Partials     2208     2208

Impacted Files	Coverage Δ
...pplication/components/data_tab/secondary_panel.tsx	`2.32% <0.00%> (-0.24%)`	⬇️
...s/wizard/public/application/components/top_nav.tsx	`5.88% <0.00%> (ø)`
...rd/public/application/utils/get_top_nav_config.tsx	`3.84% <ø> (+0.62%)`	⬆️
...public/application/utils/use/use_index_pattern.tsx	`5.00% <ø> (ø)`
src/plugins/wizard/public/plugin.ts	`27.58% <0.00%> (ø)`
...public/visualizations/common/expression_helpers.ts	`11.11% <0.00%> (ø)`
...public/visualizations/vislib/area/to_expression.ts	`10.00% <0.00%> (+0.90%)`	⬆️
...ualizations/vislib/histogram/histogram_vis_type.ts	`100.00% <ø> (ø)`
...c/visualizations/vislib/histogram/to_expression.ts	`10.00% <0.00%> (+0.90%)`	⬆️
...public/visualizations/vislib/line/to_expression.ts	`10.00% <0.00%> (+0.90%)`	⬆️
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Ashwin P Chandran <[email protected]>

kavilla

LGTM. Verified.

joshuarrrr

A couple minor questions, but looks good!

test/functional/apps/wizard/_base.ts

joshuarrrr · 2022-09-28T20:21:40Z

src/plugins/wizard/opensearch_dashboards.json

+    "dashboard",
+    "data",
+    "embeddable",
+    "expressions",
    "navigation",
+    "savedObjects",
+    "visualizations"
+  ],
+  "requiredBundles": [
    "charts",
-    "data",
    "opensearchDashboardsReact",
    "opensearchDashboardsUtils",
-    "savedObjects",
-    "embeddable",
-    "expressions",
-    "dashboard",
-    "visualizations",
-    "opensearchUiShared",
    "visDefaultEditor",
    "visTypeVislib"
-  ],


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

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

joshuarrrr · 2022-09-28T20:29:25Z

src/plugins/wizard/public/visualizations/vislib/histogram/histogram_vis_type.ts

 export const createHistogramConfig = (): VisualizationTypeOptions<HistogramOptionsDefaults> => ({
  name: 'histogram',
-  title: 'Histogram',
+  title: 'Bar',


So do you still think it makes sense for the vis type name to be "histogram" (as well as the filenames), even thought the title is bar? Or would it be better to just rename to "bar" more globally?

I did not update the key to bar because vislib refers to it as histogram. When we update the visualization to use the new library we can fix this there.

ah, makes sense, thanks.

src/plugins/wizard/public/saved_visualizations/transforms.ts

src/plugins/wizard/public/application/components/workspace.scss

joshuarrrr · 2022-09-28T21:22:24Z

src/plugins/wizard/public/application/components/data_tab/secondary_panel.tsx

+  const metricAggs = useMemo(
+    () =>
+      indexPattern
+        ? aggService.createAggConfigs(
+            indexPattern,
+            cloneDeep(
+              aggConfigParams.filter((aggConfigParam) => aggConfigParam.schema === 'metric')
+            )
+          ).aggs
+        : [],
+    [aggConfigParams, aggService, indexPattern]
+  );


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.

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.

ah, got it, that makes sense

joshuarrrr · 2022-09-28T21:28:58Z

src/plugins/wizard/public/application/components/top_nav.tsx

@@ -65,6 +69,7 @@ export const TopNav = () => {
        config={config}
        setMenuMountPoint={setHeaderActionMenu}
        indexPatterns={indexPattern ? [indexPattern] : []}
+        showDatePicker={!!indexPattern?.timeFieldName ?? true}


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

good call. will fix in followup :)

opensearch-trigger-bot · 2022-09-28T21:50:36Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-2401-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 83f13067fd4d5e5270037734b6ae82784ec692a5
# Push it to GitHub
git push --set-upstream origin backport/backport-2401-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-2401-to-2.x.

opensearch-trigger-bot · 2022-09-28T21:50:37Z

The backport to 2.3 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.3 2.3
# Navigate to the new working tree
cd .worktrees/backport-2.3
# Create a new branch
git switch --create backport/backport-2401-to-2.3
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 83f13067fd4d5e5270037734b6ae82784ec692a5
# Push it to GitHub
git push --set-upstream origin backport/backport-2401-to-2.3
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.3

Then, create a pull request where the base branch is 2.3 and the compare/head branch is backport/backport-2401-to-2.3.

* consolidates state to saved object serialization Signed-off-by: Ashwin P Chandran <[email protected]> * fixes histogram agg Signed-off-by: Ashwin P Chandran <[email protected]> * fixes orderBy Signed-off-by: Ashwin P Chandran <[email protected]> * style fixes Signed-off-by: Ashwin P Chandran <[email protected]> * updates base test Signed-off-by: Ashwin P Chandran <[email protected]> * Updates changelog Signed-off-by: Ashwin P Chandran <[email protected]> Signed-off-by: Ashwin P Chandran <[email protected]> (cherry picked from commit 83f1306)

* consolidates state to saved object serialization Signed-off-by: Ashwin P Chandran <[email protected]> * fixes histogram agg Signed-off-by: Ashwin P Chandran <[email protected]> * fixes orderBy Signed-off-by: Ashwin P Chandran <[email protected]> * style fixes Signed-off-by: Ashwin P Chandran <[email protected]> * updates base test Signed-off-by: Ashwin P Chandran <[email protected]> * Updates changelog Signed-off-by: Ashwin P Chandran <[email protected]> Signed-off-by: Ashwin P Chandran <[email protected]> (cherry picked from commit 83f1306) Co-authored-by: Ashwin P Chandran <[email protected]>

* consolidates state to saved object serialization Signed-off-by: Ashwin P Chandran <[email protected]> * fixes histogram agg Signed-off-by: Ashwin P Chandran <[email protected]> * fixes orderBy Signed-off-by: Ashwin P Chandran <[email protected]> * style fixes Signed-off-by: Ashwin P Chandran <[email protected]> * updates base test Signed-off-by: Ashwin P Chandran <[email protected]> * Updates changelog Signed-off-by: Ashwin P Chandran <[email protected]> Signed-off-by: Ashwin P Chandran <[email protected]> Signed-off-by: Sergey V. Osipov <[email protected]>

ashwin-pc added the vis builder label Sep 22, 2022

ashwin-pc requested a review from a team as a code owner September 22, 2022 08:20

ashwin-pc added backport 2.x backport 2.3 labels Sep 22, 2022

ashwin-pc added 5 commits September 28, 2022 00:13

consolidates state to saved object serialization

63cd90a

Signed-off-by: Ashwin P Chandran <[email protected]>

fixes histogram agg

3617277

Signed-off-by: Ashwin P Chandran <[email protected]>

fixes orderBy

dce7a65

Signed-off-by: Ashwin P Chandran <[email protected]>

style fixes

6f420b0

Signed-off-by: Ashwin P Chandran <[email protected]>

updates base test

7ee788a

Signed-off-by: Ashwin P Chandran <[email protected]>

ashwin-pc force-pushed the fix/timeseries branch from 4a6b704 to 7ee788a Compare September 28, 2022 00:19

Updates changelog

074cca7

Signed-off-by: Ashwin P Chandran <[email protected]>

kavilla approved these changes Sep 28, 2022

View reviewed changes

joshuarrrr approved these changes Sep 28, 2022

View reviewed changes

ashwin-pc merged commit 83f1306 into opensearch-project:main Sep 28, 2022

kavilla added backport 2.x and removed backport 2.x labels Sep 28, 2022

opensearch-trigger-bot bot mentioned this pull request Sep 28, 2022

[Backport 2.x] [Vis Builder] Misc Bar chart fixes #2445

Merged

kavilla added v2.3.1 and removed backport 2.3 labels Sep 28, 2022

opensearch-trigger-bot bot mentioned this pull request Sep 28, 2022

[Backport 2.3] [Vis Builder] Misc Bar chart fixes #2447

Merged

AMoo-Miki added bug Something isn't working and removed v2.3.1 labels Nov 5, 2022

AMoo-Miki added the v2.4.0 'Issues and PRs related to version v2.4.0' label Nov 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Vis Builder] Misc Bar chart fixes #2401

[Vis Builder] Misc Bar chart fixes #2401

ashwin-pc commented Sep 22, 2022 •

edited

Loading

codecov-commenter commented Sep 22, 2022 •

edited

Loading

kavilla left a comment

joshuarrrr left a comment

joshuarrrr Sep 28, 2022

ashwin-pc Sep 28, 2022

joshuarrrr Sep 28, 2022

ashwin-pc Sep 28, 2022

joshuarrrr Sep 29, 2022

joshuarrrr Sep 28, 2022

ashwin-pc Sep 28, 2022

joshuarrrr Sep 29, 2022

joshuarrrr Sep 28, 2022

ashwin-pc Sep 28, 2022

opensearch-trigger-bot bot commented Sep 28, 2022

opensearch-trigger-bot bot commented Sep 28, 2022

[Vis Builder] Misc Bar chart fixes #2401

[Vis Builder] Misc Bar chart fixes #2401

Conversation

ashwin-pc commented Sep 22, 2022 • edited Loading

Description

Issues Resolved

Check List

codecov-commenter commented Sep 22, 2022 • edited Loading

Codecov Report

kavilla left a comment

Choose a reason for hiding this comment

joshuarrrr 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

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Sep 28, 2022

opensearch-trigger-bot bot commented Sep 28, 2022

ashwin-pc commented Sep 22, 2022 •

edited

Loading

codecov-commenter commented Sep 22, 2022 •

edited

Loading