Move filter bar into data plugin #35460

lizozom · 2019-04-23T12:10:44Z

Summary

Deangularized parts of query_filter and push_filters
Moved query_filter and push_filters to reside in filter_manager (filter_bar is now the UI component)
Moved filter bar to data plugin.

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~~

…plugin/filter-bar

…er-bar remains the UI component + directive

…plugin/filter-bar

elasticmachine · 2019-04-23T12:10:47Z

Pinging @elastic/kibana-app-arch

elasticmachine · 2019-04-23T12:56:30Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

cchaos

Stylesheet moves lgtm

elasticmachine · 2019-04-23T14:37:38Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

… to removal of Promise class from code). Now digest is being called once when mapAndFlatten is called.

elasticmachine · 2019-04-24T16:21:36Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-24T16:42:21Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

Bargs

There still seems to be a digest issue. For example adding a new filter via the filter bar doesn’t take effect in Discover until some other action triggers a digest cycle (like mouse hovering over the date histogram).

Also left a few minor comments inline. Other than that things look pretty good!

src/legacy/ui/public/filter_manager/lib/map_filter.js

Bargs · 2019-04-24T16:30:09Z

src/legacy/ui/public/vis/vis_filters.js

  const queryFilter = Private(FilterBarQueryFilterProvider);

  const pushFilters = (filters, simulate) => {
    const appState = getAppState();
    if (filters.length && !simulate) {
      const flatFilters = _.flatten(filters);
      const deduplicatedFilters = flatFilters.filter((v, i) => i === flatFilters.findIndex(f => _.isEqual(v, f)));
-      filterBarPushFilters(appState)(deduplicatedFilters);
+      Promise.resolve(pushFilterBarFilters(appState, deduplicatedFilters));


Why is this returning a promise now? The only usage of it I see doesn't seem to handle the promise.

Bargs · 2019-04-24T16:47:06Z

src/legacy/ui/public/filter_manager/query_filter.js

-        if (!filterState.filters) {
-          filterState.filters = [];
-        }
+    return Promise.resolve(mapAndFlattenFilters(indexPatterns, filters)).then((mappedFilters) => {


Does this need wrapped in Promise.resolve since mapAndFlattenFilters returns a Promise?

lukeelmers

This is a big one, but looking solid so far! Did an initial read through the code (haven't tested yet), and added a few notes for discussion.

It might make this a little easier to review if it were separated out into 2 smaller PRs (deangularize, then move). But based on the commit history that may be a non-trivial change at this point, so I reviewed anyway.

Also, since we actually relocate the filter bar here, would you mind updating the dev docs entry accordingly?

src/legacy/core_plugins/data/public/index_patterns/index_patterns_service.ts

lukeelmers · 2019-04-26T20:46:48Z

src/legacy/core_plugins/data/public/index.ts

+          this.applyFilters.loadLegacyDirectives();
+          this.filterBar.loadLegacyDirectives();
+        },
+        ...this.applyFilters.setup(),


Is it necessary for apply filters to be its own service? It doesn't feel like it has much going on to justify existing as an independent service.

To me it would feel more natural to have a filter service that includes both filter bar and apply filters... along with any other filter-related utilities we may add in the future.

lukeelmers · 2019-04-26T20:51:40Z

src/legacy/core_plugins/data/public/apply_filters/apply_filter_service.ts

+    // nothing to do here yet
+  }
+
+  public loadLegacyDirectives() {


In the new platform, any items that are part of a plugin's public contract should be returned by that plugin's setup method. I propose we follow the same convention in our services for consistency.

In this case, that would mean making loadLegacyDirectives private, and returning it from the public setup method. Or even better, just add it to the setup method directly:

public setup() { return { ApplyFiltersPopover, loadLegacyDirectives: once(setupDirective), }; }

This also ensures that ReturnType<ApplyFiltersService['setup']> captures the entire contract of this service

lukeelmers · 2019-04-26T20:52:42Z

src/legacy/core_plugins/data/public/apply_filters/apply_filter_service.ts

+export class ApplyFiltersService {
+  private setupDirectives: any;
+  public setup() {
+    this.setupDirectives = _.once(setupDirective);


looks like lodash isn't imported in this file... should probably also prefer import { once } from 'lodash'; instead of _

lukeelmers · 2019-04-26T20:53:16Z

src/legacy/core_plugins/data/public/apply_filters/apply_filter_service.ts

+import { setupDirective } from './directive';
+
+/**
+ * Search Bar Service


Suggested change

* Search Bar Service

* Apply Filters Service

(if this really needs to be a separate service -- see my other note on that)

lukeelmers · 2019-04-26T20:54:41Z

src/legacy/core_plugins/data/public/filter_bar/filter_bar_service.ts

+    // nothing to do here yet
+  }
+
+  public loadLegacyDirectives() {


Same note here on the convention of returning the public contract from the setup method

lukeelmers · 2019-04-26T20:55:45Z

src/legacy/core_plugins/data/public/filter_bar/filter_bar_service.ts

-        });
-      };
+/**
+ * Search Bar Service


Suggested change

* Search Bar Service

* Filter Bar Service

(if this needs to be it's own service)

lukeelmers · 2019-04-26T20:56:49Z

src/legacy/ui/public/events.js

@@ -30,7 +30,7 @@ import { createLegacyClass } from './utils/legacy_class';

 const location = 'EventEmitter';

-export function EventsProvider(Private, Promise) {


woo hoo 🎊

lukeelmers · 2019-04-26T20:58:01Z

src/legacy/ui/public/filter_manager/__tests__/_add_filters.js

-      $rootScope.$digest();
-
+    it('should add filters to appState', async function () {
+      await queryFilter.addFilters(filters);


thank you for sorting out these test updates. i have a feeling we will have a lot of these to do over time, so appreciate you blazing the trail here.

Liza Katz added 15 commits April 18, 2019 13:30

Cleaned up usage of angular dependencies from apply filters \ filter bar

e9212d7

Cleaned up usage of angular dependencies from exact time filter

dd1f3d9

Moved apply filters to data plugin

4998bd9

Improved syntax of filters using index patterns

23d99f1

Merge remote-tracking branch 'upstream/master' into newplatform/data-…

942d083

…plugin/filter-bar

Removed unused Private from EventsProvider

227dec9

Fixing filter bar lib tests 1

c19644c

Fix filter bar lib tests 2

9215077

Deleted unused push_filter

b233d38

Changed push filters not to be a provider

edc0d7d

Moved push filters and query manager to filter_manager, so that filt…

8ec5f6a

…er-bar remains the UI component + directive

Changed filter view absolute import path to relative

8b6db9d

Updating filter bar plugin (not fully updated yet!)

6336612

Merge remote-tracking branch 'upstream/master' into newplatform/data-…

92d758e

…plugin/filter-bar

Updated uses of filter bar to use data plugin

788c4a5

lizozom added chore Feature:New Platform v8.0.0 :AppArch v7.2.0 labels Apr 23, 2019

lizozom requested review from spalger, lukeelmers, Bargs and ppisljar April 23, 2019 12:10

lizozom requested a review from a team as a code owner April 23, 2019 12:10

lizozom self-assigned this Apr 23, 2019

eslint --fix

580e927

$rootScope.digest to replace use of custom Promise

a4c7519

cchaos approved these changes Apr 23, 2019

View reviewed changes

Liza Katz added 3 commits April 24, 2019 15:40

Changed add filter tests to reflect change in digestion behavior (due…

e8eb8a9

… to removal of Promise class from code). Now digest is being called once when mapAndFlatten is called.

Fixed digest issues with @splager

57ae861

removed index.d.ts import from index patterns

1a97718

Bargs suggested changes Apr 24, 2019

View reviewed changes

lukeelmers mentioned this pull request Apr 26, 2019

Fix re-exporting types from data plugin. #35682

Closed

lukeelmers requested changes Apr 26, 2019

View reviewed changes

lukeelmers added the release_note:skip Skip the PR/issue when compiling release notes label Apr 26, 2019

lizozom closed this May 23, 2019

stacey-gammon mentioned this pull request Jun 30, 2019

Migrating away from ui/public #26505

Closed

94 tasks

lizozom deleted the newplatform/data-plugin/filter-bar branch November 14, 2019 13:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move filter bar into data plugin #35460

Move filter bar into data plugin #35460

lizozom commented Apr 23, 2019

elasticmachine commented Apr 23, 2019

elasticmachine commented Apr 23, 2019

cchaos left a comment

elasticmachine commented Apr 23, 2019

elasticmachine commented Apr 24, 2019

elasticmachine commented Apr 24, 2019

Bargs left a comment

Bargs Apr 24, 2019

Bargs Apr 24, 2019

lukeelmers left a comment

lukeelmers Apr 26, 2019

lukeelmers Apr 26, 2019

lukeelmers Apr 26, 2019

lukeelmers Apr 26, 2019

lukeelmers Apr 26, 2019

lukeelmers Apr 26, 2019

lukeelmers Apr 26, 2019

lukeelmers Apr 26, 2019

		@@ -30,7 +30,7 @@ import { createLegacyClass } from './utils/legacy_class';

		const location = 'EventEmitter';

		export function EventsProvider(Private, Promise) {

Move filter bar into data plugin #35460

Move filter bar into data plugin #35460

Conversation

lizozom commented Apr 23, 2019

Summary

Checklist

elasticmachine commented Apr 23, 2019

elasticmachine commented Apr 23, 2019

💔 Build Failed

cchaos left a comment

Choose a reason for hiding this comment

elasticmachine commented Apr 23, 2019

💔 Build Failed

elasticmachine commented Apr 24, 2019

💔 Build Failed

elasticmachine commented Apr 24, 2019

💔 Build Failed

Bargs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukeelmers 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