cleanup visualizations api by ppisljar · Pull Request #59958 · elastic/kibana

ppisljar · 2020-03-11T21:05:40Z

Summary

update visualizations/vis
- typescript
- remove most of the methods
- remove inheritance from eventemitter
update visualizations/expressions/vis
- typescript
- new type to differ from visualizations/vis
introduce new type SavedVis which represents visualization as saved in .kibana index
introduce new type SerializedVis which is serialized Vis object
utilities to convert between SavedVis and SerializedVis
all data information is now present on Vis object (before filters,query,timerange, savedSearchId were missing)

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-03-12T17:13:29Z

Pinging @elastic/kibana-app-arch (Team:AppArch)

...re_plugins/visualizations/public/np_ready/public/embeddable/visualize_embeddable_factory.tsx

sulemanof · 2020-03-19T16:03:21Z

src/legacy/core_plugins/kibana/public/visualize/np_ready/editor/editor.js

+      const visState = stateContainer.getState().vis;
+      const state = {
+        type: visState.type,
+        title: visState.title,
+        params: visState.params,
+        data: {
+          aggs: visState.aggs,
+        },
+      };
+      vis.setState(state);


Seems you skip this suggestion (at least didn't see an answer), but I think it will simplify code.
If you only need to extract aggs and put them into data, you could simplify this:

const { vis: { aggs, ...visState } }= stateContainer.getState();
vis.setState({ ...visState, data: { aggs } });

lukeelmers

Did a pass through to test the various vis types (Chrome macOS); from what I can tell everything seems to be working as expected.

visualizations plugin changes LGTM overall, once we get the last conflicts fixed & a green build.

# Conflicts: # src/legacy/core_plugins/tile_map/public/tile_map_type.js # src/legacy/core_plugins/vis_default_editor/public/components/agg.test.tsx # src/legacy/core_plugins/vis_default_editor/public/components/agg_common_props.ts # src/legacy/core_plugins/vis_default_editor/public/components/agg_group.test.tsx # src/legacy/core_plugins/vis_default_editor/public/components/agg_param_props.ts # src/legacy/core_plugins/vis_default_editor/public/components/agg_params.test.tsx # src/legacy/core_plugins/vis_default_editor/public/components/agg_params_helper.test.ts # src/legacy/core_plugins/vis_default_editor/public/components/agg_params_helper.ts # src/legacy/core_plugins/vis_default_editor/public/components/controls/field.test.tsx # src/legacy/core_plugins/vis_default_editor/public/components/controls/percentiles.test.tsx # src/legacy/core_plugins/vis_default_editor/public/components/controls/test_utils.ts # src/legacy/core_plugins/vis_default_editor/public/components/sidebar/sidebar.tsx # src/legacy/core_plugins/vis_default_editor/public/components/sidebar/state/index.ts # src/legacy/core_plugins/vis_default_editor/public/components/sidebar/state/reducers.ts # src/legacy/core_plugins/vis_default_editor/public/vis_options_props.tsx # src/legacy/core_plugins/vis_type_tagcloud/public/tag_cloud_type.ts

sulemanof · 2020-03-23T09:27:45Z

src/legacy/core_plugins/kibana/public/visualize/np_ready/editor/editor.js

    searchSource.setParent(searchSourceGrandparent);

+    delete savedVis.savedSearchId;
+    delete vis.data.savedSearchId;


We are handling the linked state in url. Callinng stateContainer.transitions.unlinkSavedSearch will set linked: false, which will remove savedSearchId from both savedVis and vis.data objects in handleLinkedSearch, so there is no necessity to do it here.

sulemanof · 2020-03-23T09:41:20Z

src/legacy/core_plugins/vis_default_editor/public/components/sidebar/sidebar_title.tsx

 }

-export function LinkedSearch({ savedSearch, vis }: LinkedSearchProps) {
+export function LinkedSearch({ vis, savedSearch, eventEmitter }: LinkedSearchProps) {


vis is an extra prop in LinkedSearch now

src/legacy/core_plugins/vis_default_editor/public/default_editor.tsx

sulemanof · 2020-03-23T10:14:57Z

src/legacy/core_plugins/vis_default_editor/public/default_editor.tsx

+      query,
+    });
+
+    return () => {};


Extra returning a function, which doesn't do anything, should be removed.

sulemanof · 2020-03-23T10:16:40Z

src/legacy/core_plugins/vis_default_editor/public/default_editor_controller.tsx

-  savedObj: VisSavedObject;
+  vis: Vis;
+  eventEmitter: EventEmitter;
+  embeddableHandler: any;


AFAIK this should be VisualizeEmbeddable type, shouldn't it?

sulemanof · 2020-03-23T10:23:12Z

src/legacy/core_plugins/vis_default_editor/public/vis_options_props.tsx

  isTabSelected: boolean;
  stateParams: VisParamType;
  vis: Vis;
-  uiState: PersistedState;


When you removed passing down the uiState into vis options tab, we lost next functionality:

create a gauge vis;

change a color for a metric in the legend (in options tab you'll see the Reset colors button) - the uiState should be preserved in the url;

reload the page - you'll lost the actual ui state (color is lost, button is not shown)

src/legacy/core_plugins/vis_type_vislib/public/components/options/point_series/point_series.tsx

sulemanof · 2020-03-23T11:14:26Z

Whila naviating back in TSVB there is an error appeared:

Steps to reproduce:

create a TSVB vis;
change the agg to be grouped by any term;
go back - error occured, visualization is not updated;

sulemanof

All of the specified issues were resolved!
Apart from that, didn't find anything else broken in visualize (was mostly focused on testing there)
Left some nits, but overall LGTM!
Great step over moving to NP!

sulemanof · 2020-03-23T14:58:49Z

src/legacy/core_plugins/kibana/public/visualize/np_ready/types.d.ts

    state: VisualizeAppState
  ) => ({ query, parentFilters }: { query?: Query; parentFilters?: Filter[] }) => VisualizeAppState;
  updateVisState: (state: VisualizeAppState) => (vis: PureVisState) => VisualizeAppState;
+  updateUiState: (state: VisualizeAppState) => (uiState: PersistedState) => VisualizeAppState;


sulemanof · 2020-03-23T16:28:13Z

src/legacy/core_plugins/kibana/public/visualize/np_ready/editor/lib/visualize_app_state.ts

        linked: false,
      }),
      updateVisState: state => newVisState => ({ ...state, vis: toObject(newVisState) }),
+      updateUiState: state => newUiState => ({ ...state, uiState: newUiState }),


Extra method

kibanamachine · 2020-03-23T18:19:23Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 153d062

History

💚 Build #35360 succeeded fdf874c
💚 Build #35288 succeeded 6fa5568
💚 Build #34537 succeeded b4da458
💔 Build #34454 failed 3591126
💔 Build #34444 failed 19f9a6c46f1e98fef9a2668f9f15dc10bf392278

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

* master: (26 commits) [Alerting] Fixes flaky test in Alert Instances Details page (elastic#60893) cleanup visualizations api (elastic#59958) Inline timezoneProvider function, remove ui/vis/lib/timezone (elastic#60475) [SIEM] Adds 'Open one signal' Cypress test (elastic#60484) [UA] Upgrade assistant migration meta data can become stale (elastic#60789) [Metrics Alerts] Remove metric field from doc count on backend (elastic#60679) [Uptime] Skip failing location test temporarily (elastic#60938) [ML] Disabling datafeed editing when job is running (elastic#60751) Adding `authc.invalidateAPIKeyAsInternalUser` (elastic#60717) [SIEM] Add license check to ML Rule form (elastic#60691) Adding `authc.grantAPIKeyAsInternalUser` (elastic#60423) Support Histogram Data Type (elastic#59387) [Upgrade Assistant] Fix edge case where reindex op can falsely be seen as stale (elastic#60770) [SIEM] [Cases] Update case icons (elastic#60812) [TSVB] Fix percentiles band mode (elastic#60741) Fix formatter on range aggregation (elastic#58651) Goodbye, legacy data plugin 👋 (elastic#60449) [Metrics UI] Alerting for metrics explorer and inventory (elastic#58779) [Remote clustersadopt changes to remote info API (elastic#60795) Only run xpack siem cypress in PRs when there are siem changes (elastic#60661) ...

ppisljar force-pushed the api/vis1 branch from 3986927 to e712666 Compare March 12, 2020 17:11

ppisljar added the Team:AppArch label Mar 12, 2020

ppisljar marked this pull request as ready for review March 12, 2020 19:12

ppisljar requested a review from a team March 12, 2020 19:12

ppisljar requested a review from a team as a code owner March 12, 2020 19:12

ppisljar added release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v8.0.0 labels Mar 12, 2020

ppisljar force-pushed the api/vis1 branch 2 times, most recently from 8f93b09 to 999ceeb Compare March 16, 2020 07:03

ppisljar changed the title ~~Api/vis1~~ cleanup visualizations api Mar 16, 2020

ppisljar force-pushed the api/vis1 branch 4 times, most recently from e4d16bf to 8d0d452 Compare March 16, 2020 15:38

lukeelmers reviewed Mar 17, 2020

View reviewed changes

...re_plugins/visualizations/public/np_ready/public/embeddable/visualize_embeddable_factory.tsx Outdated Show resolved Hide resolved

ppisljar force-pushed the api/vis1 branch 4 times, most recently from a11b2da to 4082665 Compare March 17, 2020 11:23

ppisljar added 9 commits March 17, 2020 04:30

updating vis and savedvis

e446394

embeddable

ca104a7

discover

bb80361

visualize editor

f4c11a8

vis types

4697225

fixing jest tests

bfa59c7

fixing karma tests

33c9173

fixes

7c1e4ac

more fixes

6bc01c2

fixes

b4da458

ppisljar force-pushed the api/vis1 branch from 7f68857 to b4da458 Compare March 19, 2020 14:16

sulemanof reviewed Mar 19, 2020

View reviewed changes

lukeelmers approved these changes Mar 21, 2020

View reviewed changes

ppisljar added 3 commits March 22, 2020 23:29

fixing editor history again

9615604

fixing merge

6fa5568

sulemanof reviewed Mar 23, 2020

View reviewed changes

src/legacy/core_plugins/vis_default_editor/public/default_editor.tsx Show resolved Hide resolved

sulemanof reviewed Mar 23, 2020

View reviewed changes

src/legacy/core_plugins/vis_type_vislib/public/components/options/point_series/point_series.tsx Outdated Show resolved Hide resolved

fixing based on review

6589f01

ppisljar added 2 commits March 23, 2020 05:06

fixing uiState

cac892b

fixing tsvb issue

fdf874c

sulemanof approved these changes Mar 23, 2020

View reviewed changes

updating based on review

153d062

ppisljar merged commit ed55531 into elastic:master Mar 23, 2020

ppisljar mentioned this pull request Mar 23, 2020

[7.x] cleanup visualizations api (#59958) #60959

Merged

ppisljar added a commit to ppisljar/kibana that referenced this pull request Mar 23, 2020

cleanup visualizations api (elastic#59958)

3a0742f

ppisljar added a commit that referenced this pull request Mar 24, 2020

cleanup visualizations api (#59958) (#60959)

80a7fc3

sulemanof mentioned this pull request Mar 25, 2020

[Discover] Link generated for visualize button in sidebar no longer works #61213

Closed

timroes mentioned this pull request Mar 25, 2020

[Discover][Visualize] Fix visualizing a selected field in discover search #61226

Merged

7 tasks

kertal mentioned this pull request Apr 8, 2020

Auto scaling interval not shown in Discover #62753

Closed

ThomThomson mentioned this pull request Sep 3, 2020

[Input Controls] Fix Resize Resetting Selections #76573

Merged

2 tasks

Conversation

ppisljar commented Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Mar 12, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukeelmers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sulemanof commented Mar 23, 2020

Uh oh!

sulemanof left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Mar 23, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ppisljar commented Mar 11, 2020 •

edited

Loading