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

Click to filter values in Discover doc table #5344

Closed
wants to merge 4 commits into from
Closed

Click to filter values in Discover doc table #5344

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2db1f9f
Click to filter values in Discover doc table
jimmyjones2 Nov 6, 2015
7a2d69e
Changes suggested by @cjcenizal
jimmyjones2 Jan 6, 2017
1082a99
Un-nest selector to avoid unnecessary specifity and fix bug caused by…
jimmyjones2 Jan 8, 2017
ebf8c50
Move flattenedRow to the scope as suggested by @Bargs
jimmyjones2 Jan 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 26 additions & 5 deletions src/ui/public/doc_table/components/table_row.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import noWhiteSpace from 'ui/utils/no_white_space';
import openRowHtml from 'ui/doc_table/components/table_row/open.html';
import detailsHtml from 'ui/doc_table/components/table_row/details.html';
import uiModules from 'ui/modules';
import FilterManagerProvider from 'ui/filter_manager';
const module = uiModules.get('app/discover');


Expand All @@ -24,9 +25,10 @@ const MIN_LINE_LENGTH = 20;
* <tr ng-repeat="row in rows" kbn-table-row="row"></tr>
* ```
*/
module.directive('kbnTableRow', function ($compile) {
module.directive('kbnTableRow', ['$compile', 'Private', function ($compile, Private) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for making this improvement! :D

const cellTemplate = _.template(noWhiteSpace(require('ui/doc_table/components/table_row/cell.html')));
const truncateByHeightTemplate = _.template(noWhiteSpace(require('ui/partials/truncate_by_height.html')));
const filterManager = Private(FilterManagerProvider);

return {
restrict: 'A',
Expand Down Expand Up @@ -83,27 +85,46 @@ module.directive('kbnTableRow', function ($compile) {
createSummaryRow($scope.row, $scope.row._id);
});

$scope.inlineFilter = function inlineFilter($event, type) {
const column = $($event.target).data().column;
const field = $scope.indexPattern.fields.byName[column];
$scope.indexPattern.popularizeField(field, 1);
filterManager.add(field, $scope.flattenedRow[column], type, $scope.indexPattern.id);
};

// create a tr element that lists the value for each *column*
function createSummaryRow(row) {
const indexPattern = $scope.indexPattern;
$scope.flattenedRow = indexPattern.flattenHit(row);

// We just create a string here because its faster.
const newHtmls = [
openRowHtml
];

const mapping = indexPattern.fields.byName;
if (indexPattern.timeFieldName) {
newHtmls.push(cellTemplate({
timefield: true,
formatted: _displayField(row, indexPattern.timeFieldName)
formatted: _displayField(row, indexPattern.timeFieldName),
filterable: mapping[indexPattern.timeFieldName].filterable,
column: indexPattern.timeFieldName
}));
}

$scope.columns.forEach(function (column) {
const isFilterable =
$scope.flattenedRow[column] === undefined
? false
: !mapping[column]
? false
: mapping[column].filterable;
Copy link
Contributor

Choose a reason for hiding this comment

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

I had a difficult time parsing this. Personally I think the following would be more readable:

const isFilterable = !_.isUndefined(flattenedRow[column]) && _.get(mapping, `${column}.filterable`, false);

Thoughts @cjcenizal ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I had a few issues with this:

  • I had to look up _.get to understand what each argument is for.
  • I also had to hold a lot more state in my head to map input to output (like the double negative of "not undefined").
  • Also, get returns the default value only for undefined, but this logic returns false if mapping[column] is falsy, so it's not quite a true replacement.

I think the reason I like the ternary is because it has the same pattern as a function with early exits, which makes it really easy for me to map input to output. Maybe the best route is to actually create a function that does just that?

function isFilterable(row, mapping, column) {
  if (row[column] === undefined) {
    return false;
  }

  if (!mapping[column]) {
    return false;
  }

  return  mapping[column].filterable;
}

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I generally dislike custom functions where a general one will do. Now I have to find and understand isFilterable which helps me only in this context. If I don't know how _.get works, I look it up, and now I know how _.get works everywhere it's used. It's kind of a "teach a man to fish" sort of thing.

Also, get returns the default value only for undefined, but this logic returns false if mapping[column] is falsy, so it's not quite a true replacement.

First of all, that's not true: http://codepen.io/anon/pen/EZKNNN?editors=0011
But even if it were, false would be the correct value in that situation...

I also had to hold a lot more state in my head to map input to output (like the double negative of "not undefined").

I'd be ok with flattenedRow[column] !== undefined if you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Having let it percolate overnight, I think we should just leave the ternary. It was sort of a silly thing to worry about in the first place.

Copy link
Contributor

Choose a reason for hiding this comment

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

So it turns out we have an eslint rule against nested ternaries:

/var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-intake/src/ui/public/doc_table/components/table_row.js
  117:13  error  Do not nest ternary expressions  no-nested-ternary

@jimmyjones2 would you mind implementing the function @cjcenizal suggested above to replace the ternary? Sorry for the back and forth, this should be the last change assuming tests pass.

newHtmls.push(cellTemplate({
timefield: false,
sourcefield: (column === '_source'),
formatted: _displayField(row, column, true)
formatted: _displayField(row, column, true),
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's pass a rawValue param here (and for the timefield above) and pass it to the inlineFilter call in the template in the same way I suggested passing the column name. Then the call to flattenHit can move up to line 99, share the variable throughout createSummaryRow and it can be removed entirely from inlineFilter (since the field value will be passed as a param). This way flattenHit only needs to be called once per row.

Copy link
Contributor Author

@jimmyjones2 jimmyjones2 Jan 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@Bargs I was worried the raw field value could contain special characters such as quotes - would this be safe to pass into an HTML element attribute and back into inlineFilter unscathed? Just tried a field value containing single and double quotes and it broke.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jimmyjones2 That's a great point. Ignore both my comments about passing literal values into inlineFilter in the template.

If you could do me just one other favor, I think this'll be good to merge. Could you add a flattenedRow attribute to the scope and use that everywhere, rather than re-flattening each time?

filterable: isFilterable,
column: column
}));
});

Expand All @@ -128,7 +149,7 @@ module.directive('kbnTableRow', function ($compile) {
// rebuild cells since we modified the children
$cells = $el.children();

if (i === 0 && !reuse) {
if (!reuse) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious why this was changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Bargs Can't exactly remember now, but without that change it doesn't work!

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I looked back in the history, and I guess index 0 is the toggle control which previously was the only element that needed compiled. Now we need to compile the individual cells, so this makes sense.

$toggleScope = $scope.$new();
$compile($target)($toggleScope);
}
Expand Down Expand Up @@ -160,4 +181,4 @@ module.directive('kbnTableRow', function ($compile) {
}
}
};
});
}]);
19 changes: 19 additions & 0 deletions src/ui/public/doc_table/components/table_row/cell.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,23 @@
%>
<td <%= attributes %>>
<%= formatted %>
<span class="table-cell-filter">
<% if (filterable) { %>
<span
ng-click="inlineFilter($event, '+')"
class="fa fa-search-plus docTableRowFilterIcon"
data-column="<%- column %>"
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove the data-column and avoid having to inspect the target of the event in the callback by modifying the ng-click like this:

ng-click="inlineFilter('<%= column %>', '-')"

Copy link
Contributor

Choose a reason for hiding this comment

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

As I mentioned in my other comment, ignore this.

tooltip="Filter for value"
tooltip-append-to-body="1"
></span>

<span
ng-click="inlineFilter($event, '-')"
class="fa fa-search-minus docTableRowFilterIcon"
data-column="<%- column %>"
tooltip="Filter out value"
tooltip-append-to-body="1"
></span>
<% } %>
</span>
</td>
32 changes: 32 additions & 0 deletions src/ui/public/doc_table/doc_table.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,26 @@ doc-table {
word-break: break-word;
}

.discover-table-row {
td {
position: relative;

.table-cell-filter {
position: absolute;
background-color: rgba(255, 255, 255, 0.75);
white-space: nowrap;
right: 0;
visibility: hidden;
}

&:hover {
.table-cell-filter {
visibility: visible;
}
}
}
}

.loading {
opacity: @loading-opacity;
}
Expand All @@ -31,3 +51,15 @@ doc-table {
opacity: @loading-opacity;
}
}

/**
* 1. Align icon with text in cell.
*/
.docTableRowFilterIcon {
font-size: 14px;
line-height: 1; /* 1 */

& + & {
margin-left: 5px;
}
}
1 change: 0 additions & 1 deletion src/ui/public/styles/base.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,6 @@ kbn-table, .kbn-table {
visibility: visible;
}
}

}

//== Generic Table
Expand Down