changing vis.API.events #25280

ppisljar · 2018-11-07T10:30:38Z

Summary

Moves filtering functions out of Vis.

preparation for #23185

QA: no functional canges

Dev-Docs

filtering inside your visualization will need to be updated

vis.API.events.addFilter no longer exists, instead you should rather call vis.API.events.filter({ table, column, row, value })

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes [i18n support]~~(https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md)
~~[ ] 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~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
~~[ ] This includes a feature addition or change that requires a release note and was labeled appropriately~~

elasticmachine · 2018-11-07T10:48:12Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-11-07T14:58:15Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-11-07T19:39:53Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-11-08T08:38:48Z

Pinging @elastic/kibana-app

elasticmachine · 2018-11-08T08:53:13Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

markov00

Code LGTM. Some minor comments.
Tested on chrome/osx using brushes and filters

markov00 · 2018-11-08T09:01:30Z

src/ui/public/vis/vis_types/base_vis_type.js

+      const _defaults = {
+        // name, title, description, icon, image
+        category: CATEGORY.OTHER,
+        visualization: null,       // must be a class with render/resize/destroy methods


nit: Can we remove this visualization default? We already have a check on opts thats trows an exception if visualization is a falsy value.

What about checking if the visualization object has the required methods?

no idea why this shows as added code .... it was just indented

markov00 · 2018-11-08T09:03:02Z

src/ui/public/vis/vis_types/base_vis_type.js

+            defaultAction: visFilters.addFilter,
+          }
+        },
+        stage: 'production',


nit: What about having the stage default as lab, so we don't accidentally create a production vis?

no idea why this shows as added code .... it was just indented

markov00 · 2018-11-08T09:03:51Z

src/ui/public/vis/vis_types/base_vis_type.js

+    shouldMarkAsExperimentalInUI() {
+      //we are not making a distinction in the UI if a plugin is experimental and/or labs.
+      //we just want to indicate it is special. the current flask icon is sufficient for that.
+      return this.stage === 'experimental' || this.stage === 'lab';


nit: What about move these strings to an enum similar to:

const STAGE_TYPES = { LAB: 'lab', EXPERIMENTAL: 'experimental', PRODUCTION: 'production', }

no idea why this shows as added code .... it was just indented

markov00 · 2018-11-08T09:13:27Z

src/ui/public/visualize/loader/embedded_visualize_handler.ts

+    this.events$ = this.vis.eventsSubject.asObservable().pipe(share());
+    this.events$.subscribe(event => {
+      if (this.actions[event.name]) {
+        this.actions[event.name](event.data);


q: do we enforce actions to be functions somewhere?

we don't, we need to better type this

markov00 · 2018-11-08T09:27:50Z

src/ui/public/vis/vis.js

@@ -75,12 +72,8 @@ export function VisProvider(Private, indexPatterns, getAppState) {
        events: {
          // the filter method will be removed in the near feature
          // you should rather use addFilter method below


I'm a bit confused what is the right one, this:

vis.API.events.addFilter no longer exists, instead you should rather call vis.API.events.filter({ table, column, row, value })

or

the filter method will be removed in the near feature you should rather use addFilter method below

ha :) i need to remove that comment

markov00 · 2018-11-08T09:33:39Z

src/ui/public/vis/vis_filters.js

@@ -41,26 +42,26 @@ const getTerms = (table, columnIndex, rowIndex) => {
  }))];
 };

-export function VisFiltersProvider(Private, getAppState) {
+const createFilter = (data, columnIndex, rowIndex, cellValue) => {


nit: Is there a reason to use arrow function here? can't we use named functions?

is there a reason not to use arrow functions ?

markov00 · 2018-11-08T09:37:38Z

src/ui/public/visualize/loader/embedded_visualize_handler.ts

@@ -128,6 +130,23 @@ export class EmbeddedVisualizeHandler {
    this.vis.openInspector = this.openInspector;
    this.vis.hasInspector = this.hasInspector;

+    // init default actions
+    forEach(this.vis.type.events, (event, eventName) => {


nit: can we get rid of lodash here? Object.keys(this.vis.type.events).forEach

markov00 · 2018-11-08T09:47:11Z

src/ui/public/vis/vis.js

-            onBrushEvent(event, getAppState());
-          }
+          filter: event => this.eventsSubject.next({ name: 'filterBucket', data: event }),
+          brush: event => this.eventsSubject.next({ name: 'brush', data: event }),


To naming this consistently it's better to rewrite this as:

filter: data => this.eventsSubject.next({ name: 'filterBucket', data }), brush: data => this.eventsSubject.next({ name: 'brush', data }),

So your event is an object with {name, data} as it's used on the subscriptions

elasticmachine · 2018-11-08T13:08:06Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

lukeelmers

Code LGTM. Pulled down and tested, no apparent functional issues.

elasticmachine · 2018-11-16T13:52:43Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

ppisljar added the WIP Work in progress label Nov 7, 2018

ppisljar changed the title ~~[WIP] moving filtering out of Vis~~ [WIP] changing vis.API.events Nov 7, 2018

ppisljar force-pushed the ref/filteringEmbeddable branch from 76bd2dd to af833ed Compare November 7, 2018 13:15

ppisljar changed the title ~~[WIP] changing vis.API.events~~ changing vis.API.events Nov 7, 2018

ppisljar force-pushed the ref/filteringEmbeddable branch from ed27206 to fa8dec9 Compare November 8, 2018 06:41

ppisljar added review v7.0.0 v6.6.0 Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Team:Visualizations Visualization editors, elastic-charts and infrastructure and removed WIP Work in progress labels Nov 8, 2018

ppisljar requested review from markov00 and lukeelmers November 8, 2018 08:38

markov00 approved these changes Nov 8, 2018

View reviewed changes

lukeelmers approved these changes Nov 9, 2018

View reviewed changes

ppisljar added 3 commits November 16, 2018 12:39

moving filters out of Vis

3cedf9e

updating unit tests

188fdd5

updating based on review

88e6af8

ppisljar force-pushed the ref/filteringEmbeddable branch from 4b1b834 to 88e6af8 Compare November 16, 2018 11:40

ppisljar merged commit 6dd2c1c into elastic:master Nov 16, 2018

ppisljar deleted the ref/filteringEmbeddable branch November 16, 2018 14:00

ppisljar mentioned this pull request Nov 16, 2018

[6.x] changing vis.API.events (#25280) #25802

Merged

ppisljar added a commit to ppisljar/kibana that referenced this pull request Nov 16, 2018

changing vis.API.events (elastic#25280)

56403c9

ppisljar added a commit that referenced this pull request Nov 18, 2018

changing vis.API.events (#25280) (#25802)

420ca93

lukasolson mentioned this pull request Dec 4, 2018

Fix discover histogram #26600

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

changing vis.API.events #25280

changing vis.API.events #25280

ppisljar commented Nov 7, 2018 •

edited

Loading

elasticmachine commented Nov 7, 2018

elasticmachine commented Nov 7, 2018

elasticmachine commented Nov 7, 2018

elasticmachine commented Nov 8, 2018

elasticmachine commented Nov 8, 2018

markov00 left a comment

markov00 Nov 8, 2018

ppisljar Nov 8, 2018

markov00 Nov 8, 2018

ppisljar Nov 8, 2018

markov00 Nov 8, 2018

ppisljar Nov 8, 2018

markov00 Nov 8, 2018

ppisljar Nov 8, 2018

markov00 Nov 8, 2018

ppisljar Nov 8, 2018

markov00 Nov 8, 2018

ppisljar Nov 8, 2018

markov00 Nov 8, 2018

markov00 Nov 8, 2018

markov00 Nov 8, 2018

elasticmachine commented Nov 8, 2018

lukeelmers left a comment

elasticmachine commented Nov 16, 2018

changing vis.API.events #25280

changing vis.API.events #25280

Conversation

ppisljar commented Nov 7, 2018 • edited Loading

Summary

Dev-Docs

filtering inside your visualization will need to be updated

Checklist

For maintainers

elasticmachine commented Nov 7, 2018

💔 Build Failed

elasticmachine commented Nov 7, 2018

💔 Build Failed

elasticmachine commented Nov 7, 2018

💔 Build Failed

elasticmachine commented Nov 8, 2018

elasticmachine commented Nov 8, 2018

💚 Build Succeeded

markov00 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

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

elasticmachine commented Nov 8, 2018

💚 Build Succeeded

lukeelmers left a comment

Choose a reason for hiding this comment

elasticmachine commented Nov 16, 2018

💚 Build Succeeded

ppisljar commented Nov 7, 2018 •

edited

Loading