Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[discuss][UiActions] Remove duplicate apply filter action? #77485

Merged
merged 3 commits into from
Sep 22, 2020
Merged

[discuss][UiActions] Remove duplicate apply filter action? #77485

merged 3 commits into from
Sep 22, 2020

Conversation

Dosant
Copy link
Contributor

@Dosant Dosant commented Sep 15, 2020 •
edited
Loading

Summary

I noticed that there are 2 different applyFilterActions:

  1. https://github.com/elastic/kibana/blob/master/src/plugins/data/public/actions/apply_filter_action.ts
  2. https://github.com/elastic/kibana/blob/master/src/plugins/embeddable/public/lib/actions/apply_filter_action.ts

Looks like 2nd one is not used. Probably usage was dropped somewhere along NP refactoring's and all apply filter functionality is based on 1st action.

@ppisljar explained the idea behind first:

so one works by talking to the filtermanager directly and the other one updates filters on the embeddable, the first one only makes sense if you have filter bar in view (dashboard, visualize editor, discover)

This idea makes sense, but it is not how it is implemented now.

Since these two actions were confusing there are two options:

  1. Remove it for now. Get back to this pattern later when we have a use case, where global action doesn't work.
  2. Make embeddable work with 2nd action. This is not that quick since all containers have to be capable of applying filters themselves. That 2nd action need to be improved to handle filter selection and extracting timeRange from filters.

Because 2nd action is not used, there seem to be no real use case for it now and to make it work some additional work is needed, I'd propose to just remove it for now and get back to this pattern latter.

@Dosant Dosant changed the title [wip] remove duplicate apply filter action [discuss] Remove duplicate apply filter action? Sep 15, 2020
@Dosant Dosant added Feature:UIActions UI actions. These are client side only, not related to the server side actions.. release_note:skip Skip the PR/issue when compiling release notes Team:AppArch technical debt Improvement of the software architecture and operational architecture v7.10.0 v8.0.0 discuss labels Sep 15, 2020
@Dosant Dosant marked this pull request as ready for review September 15, 2020 16:46
@Dosant Dosant requested a review from a team as a code owner September 15, 2020 16:46
@elasticmachine
Copy link
Contributor

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

@Dosant Dosant changed the title [discuss] Remove duplicate apply filter action? [discuss][UiActions] Remove duplicate apply filter action? Sep 15, 2020
@botelastic botelastic bot added the Feature:Embedding Embedding content via iFrame label Sep 16, 2020
@Dosant
Copy link
Contributor Author

Dosant commented Sep 17, 2020

@ppisljar, I'll wait for you to confirm before merging,
otherwise, if not removing it, we'd need to work on a plan a refactor to make use of this action

ppisljar
ppisljar approved these changes Sep 21, 2020
View reviewed changes
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok to remove

@Dosant
Copy link
Contributor Author

Dosant commented Sep 21, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
embeddable 94 -1 95

page load bundle size

id value diff baseline
embeddable 430.2KB -4.9KB 435.1KB

History

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

@Dosant Dosant merged commit 8cf508f into elastic:master Sep 22, 2020
Dosant added a commit to Dosant/kibana that referenced this pull request Sep 22, 2020
@Dosant
[UiActions] Remove duplicate apply filter action (elastic#77485)
1ad9755 
Removing unused action
@Dosant Dosant mentioned this pull request Sep 22, 2020
Merged
Dosant added a commit that referenced this pull request Sep 22, 2020
@Dosant
[UiActions] Remove duplicate apply filter action (#77485) (#78119)
2a6e6e0 
Removing unused action
gmmorris added a commit to gmmorris/kibana that referenced this pull request Sep 22, 2020
@gmmorris
Merge branch 'master' into actions/pagerduty-dedup
56e81d2 
* master: (45 commits)
  [CSM] Use stacked chart for page views (elastic#78042)
  [Enterprise Search] Fix various plugin states when app has error connecting to Enterprise Search (elastic#78091)
  Remove service map beta badge (elastic#78039)
  [Enterprise Search] Rename "telemetry" to "stats" (elastic#78124)
  [Alerting] optimize calculation of unmuted alert instances (elastic#78021)
  call .destroy on ace when react component unmounts (elastic#78132)
  [Ingest Manager] Fix agent action acknowledgement (elastic#78089)
  [Upgrade Assistant] Rename "telemetry" to "stats" (elastic#78127)
  [Security Solution] Refactor Hosts Kpi to use Search Strategy (elastic#77606)
  Bump backport to 5.6.0 (elastic#78097)
  [Actions] adds a Test Connector tab in the Connectors list (elastic#77365)
  [Uptime] Improve ping chart axis (elastic#77992)
  [TSVB] Fields dropdowns are not populated if one of the indices is missing (elastic#77363)
  [UiActions] Remove duplicate apply filter action  (elastic#77485)
  [APM] Use transaction metrics for transaction error rate (elastic#78009)
  [ES-ARCHIVER] Fix bug when query flag is empty (elastic#77983)
  Edit UI text strings in Integrations and Fleet tabs (elastic#75837)
  [baseline capture] switch to large workers (elastic#78109)
  [SECURITY_SOLUTION] list UI is backwards compatible (elastic#77956)
  [Mappings editor] Add support for point field type (elastic#77543)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ppisljar ppisljar ppisljar approved these changes

@streamich streamich streamich approved these changes

Assignees
No one assigned
Labels
discuss Feature:Embedding Embedding content via iFrame Feature:UIActions UI actions. These are client side only, not related to the server side actions.. release_note:skip Skip the PR/issue when compiling release notes technical debt Improvement of the software architecture and operational architecture v7.10.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Dosant @elasticmachine @kibanamachine @streamich @ppisljar