Skip to content

Re-use the filter function for inline filtering#10395

Merged
weltenwort merged 2 commits intoelastic:masterfrom
weltenwort:enh-deduplicate-inline-filter
Feb 16, 2017
Merged

Re-use the filter function for inline filtering#10395
weltenwort merged 2 commits intoelastic:masterfrom
weltenwort:enh-deduplicate-inline-filter

Conversation

@weltenwort
Copy link
Copy Markdown
Member

Re-using the passed filter function improves consistency with the
implementation of the filters in the document details table.

Re-using the passed filter function improves consistency with the
implementation of the filters in the document details table.
Copy link
Copy Markdown
Contributor

@Bargs Bargs left a comment

Choose a reason for hiding this comment

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

Great cleanup! I totally missed this existing filter param while reviewing the previous PR. Left one minor comment.

<% if (filterable) { %>
<span
ng-click="inlineFilter($event, '+')"
ng-click="inlineFilter('<%- column %>', '+')"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We tried passing the column name directly like this in the original PR but there appeared to be encoding issues: https://github.com/elastic/kibana/pull/5344/files#r95694856

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oh, thanks for pointing that out. In that case I'll defer that change to the major cleanup when we get rid of the lodash templating.

weltenwort added a commit that referenced this pull request Feb 16, 2017
The inline filters are now hidden when no `filter` property has been
passed to the row directive, which is more consistent with the filters
in the document details table. The context view relies on this behaviour
to hide the filtering actions.

Improving the `inlineFilter` to actually re-use the passed `filter`
function happens in #10395.
@Bargs
Copy link
Copy Markdown
Contributor

Bargs commented Feb 16, 2017

LGTM

@weltenwort weltenwort merged commit 1a84ce1 into elastic:master Feb 16, 2017
elastic-jasper added a commit that referenced this pull request Feb 16, 2017
Backports PR #10395

**Commit 1:**
Re-use the filter function for inline filtering

Re-using the passed filter function improves consistency with the
implementation of the filters in the document details table.

* Original sha: 44cdfe8
* Authored by Felix Stürmer <stuermer@weltenwort.de> on 2017-02-16T14:45:42Z

**Commit 2:**
Revert back to using $event to avoid encoding problems

* Original sha: eefba73
* Authored by Felix Stürmer <stuermer@weltenwort.de> on 2017-02-16T17:33:49Z
weltenwort pushed a commit that referenced this pull request Feb 16, 2017
Backports PR #10395

**Commit 1:**
Re-use the filter function for inline filtering

Re-using the passed filter function improves consistency with the
implementation of the filters in the document details table.

* Original sha: 44cdfe8
* Authored by Felix Stürmer <stuermer@weltenwort.de> on 2017-02-16T14:45:42Z

**Commit 2:**
Revert back to using $event to avoid encoding problems

* Original sha: eefba73
* Authored by Felix Stürmer <stuermer@weltenwort.de> on 2017-02-16T17:33:49Z
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants