Click to filter values in Discover doc table #5344

jimmyjones2 · 2015-11-06T21:43:23Z

elasticsearch-release · 2015-11-06T21:43:26Z

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'.

jimmyjones2 · 2015-12-18T21:05:28Z

@rashidkpc Could this be tagged for review please?

rashidkpc · 2015-12-18T21:09:48Z

I think we're still back and forth on how we want this designed. I don't love the look, though I haven't given a lot of thought into how to do it differently. We have a lot of stuff in the review queue for 4.4.0 right now, I don't want to try to jam this into that release.

jimmyjones2 · 2016-02-09T20:18:53Z

@rashidkpc Have you had any more thoughts on this? I spend a lot of my day opening up documents just to filter them, so this would be really handy.

elasticsearch-release · 2016-02-20T23:16:54Z

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'.

spalger · 2016-04-05T15:02:44Z

jenkins, test it

spalger · 2016-04-05T15:33:09Z

This is pretty out of date, tests are failing, interested in getting this updated @jimmyjones2?

jimmyjones2 · 2016-04-05T19:10:21Z

Hey @spalger really keen to get this in, but do you and @rashidkpc want it done differently before you'll merge?

rashidkpc · 2016-04-07T20:50:05Z

What about making the whole value clickable with a modifier key or something? @tbragin other ideas? @simianhacker?

simianhacker · 2016-04-07T20:53:37Z

It seems like it would be cool instead of injecting the buttons into the table field we added them to some kind of tooltip. That way it doesn't cause a reflow when the field is too narrow.

spalger · 2016-04-07T23:12:29Z

/me whispers "right-click"

tbragin · 2016-04-08T23:08:10Z

I agree the current implementation has reflow problems - sometimes the two icons appear on the same row, other times they appear on different rows, and yet another time they appear in different places depending on where you moused over (see first video).

In addition, while I like the discoverability of something available on mouseover and the use of the standard icons, I don't think the behavior is consistent with how table filtering is done in other contexts in Kibana. Since this behavior applies to both Discover tab as well as the raw results table on the Dashboard tab, we have to consider how filtering is done in a the aggregated Data Table as well.

The behavior for raw results table filtering in this PR is
- Filter by clicking on (+) in mouseover
- Invert filter by clicking on (-) in mouseover
- Remove filter by deleting from Filter bar
The behavior for Data table is:
- Filter on table cell click, discoverable by highlighting cell on mouseover
- Invert filter in the filter bar
- Remove filter by deleting from Filter bar

See second video. Would it make sense to have the same type of interaction for both tables?

Regarding other suggestions:

Clickable behavior is a possibility to explore, as I mentioned above
Right-click doesn't seem standard in the Kibana app currently
I can't exactly picture a tooltip idea -- @simianhacker do we do anything like that anywhere else in the UI?

jimmyjones2 · 2016-04-10T13:38:02Z

I agree that table filtering should be the same in all contexts. I'm not a big fan of the Data Table:

I often want to filter out data, but this is a two step process, which can be slow, depending on the size of the index
It's slightly awkward if the field is formatted as a URL as you have to click the cell, but not the text

How about something similar for both where the +/- icons floated over the end of the text if it would otherwise wrap or cause the column to grow in width?

jimmyjones2 · 2016-05-01T21:58:13Z

@tbragin I was thinking of something like below, which doesn't increase the width of columns:

tbragin · 2016-05-03T22:37:30Z

Honestly, I still prefer the current approach of the Data Table, but it may make sense to get another pair of eyes on this. Perhaps @alt74 has an opinion one way or another?

Whatever solution we arrive at here, I'd advocate for keeping the behavior consistent between filtering the Data Table with aggregated results and the "raw results" table, either on Discover or in the context of Dashboard.

epixa · 2016-10-08T14:23:09Z

@jimmyjones2 Sorry that this has stagnated for so long. Do you still want to get this over the line?

elasticmachine · 2016-10-08T14:23:11Z

Can one of the admins verify this patch?

jimmyjones2 · 2016-10-09T19:49:22Z

@epixa Yup, this is really useful for me in $DAYJOB. I've implemented what's in the screenshot on 1 May but not pushed it as there didn't seems to be any consensus...

Bargs · 2016-12-27T16:15:25Z

I quite like the idea of floating the controls over the right side of the column, as posted on May 1st. It feels like a very familiar interaction that's used in a lot of other apps (see Google Inbox as one prominent example) and it gives us some room to add more buttons if we need them.

What do you think @cjcenizal ? It looks like we need to make a decision on design before we can move forward with this PR.

tbragin · 2017-01-04T05:07:12Z

@cjcenizal @Bargs It doesn't bother you that we'd have floating controls for filtering on this table and a different interaction for the data table? To me, this feels like it would be confusing for the user.

Bargs · 2017-01-04T15:43:37Z

I'd be in favor of adding the floating controls to the data table as well. There's another request for something just like that here #2184

cjcenizal · 2017-01-04T16:36:32Z

@tbragin @Bargs The May 1 screenshot looks pretty cool to me. @jimmyjones2 Can you update the PR with the latest (and maybe rebase master) so I can try it out? I'd really like to try out the UX and see if we can get this in ASAP.

jimmyjones2 · 2017-01-04T22:53:03Z

@cjcenizal Rebased on master. Let me know what you think!

cjcenizal

@jimmyjones2 I think this is sweet! I played around with it a bit and I'm sure we can find some ways to improve the experience, but overall it's excellent. I think this only needs a couple tweaks and we should be OK to merge.

@Bargs @tbragin I think we can migrate these changes to the data table in another PR. Looking forward, I could see us surfacing these controls in a tooltip or popover but for now I think this is fine. Either way, I think surfacing this functionality in the context of the cell is really useful.

cjcenizal · 2017-01-05T23:50:05Z

src/ui/public/doc_table/components/table_row.js

  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(require('ui/filter_manager'));


Could you change this require call to an import and move it to the top of the file?

import FilterManagerProvider from 'ui/filter_manager'; // and inside the directive... const filterManager = Private(FilterManagerProvider);

cjcenizal · 2017-01-05T23:53:29Z

src/ui/public/doc_table/components/table_row.js

@@ -83,6 +84,14 @@ module.directive('kbnTableRow', function ($compile) {
        createSummaryRow($scope.row, $scope.row._id);
      });

+      $scope.inlineFilter = function ($event, type) {


Can we make this function named so it's easier to read in a call stack?

$scope.inlineFilter = function inlineFilter($event, type) {

cjcenizal · 2017-01-06T00:09:25Z

src/ui/public/doc_table/components/table_row.js

          }));
        }

        $scope.columns.forEach(function (column) {
+          var filterable = mapping[column] ? mapping[column].filterable : false;
+          const flattened = indexPattern.flattenHit(row);
+          filterable = flattened[column] === undefined ? false : filterable;


Normally I try to avoid ternaries but in this case I think a ternary actually makes the code a little more readable. It also lets us avoid mutating the filterable variable:

const flattenedRow = indexPattern.flattenHit(row); const isFilterable = flattenedRow[column] === undefined ? false : !mapping[column] ? false : mapping[column].filterable;

cjcenizal · 2017-01-06T00:11:37Z

src/ui/public/doc_table/components/table_row/cell.html

@@ -10,4 +10,11 @@
 %>
 <td <%= attributes %>>
  <%= formatted %>
+  <span class="table-cell-filter">
+    <% if (filterable) { %>
+      <i>&nbsp;</i>


What does this element do? I tried to removing it and it didn't seem to have an effect.

Oops, was from the old version of the patch!

cjcenizal · 2017-01-06T00:12:09Z

src/ui/public/doc_table/components/table_row/cell.html

+    <% if (filterable) { %>
+      <i>&nbsp;</i>
+      <i ng-click="inlineFilter($event, '+')" class="fa fa-search-plus" data-column="<%- column %>" tooltip="Filter for value" tooltip-append-to-body="1"></i>
+      <i ng-click="inlineFilter($event, '-')" class="fa fa-search-minus" data-column="<%- column %>" tooltip="Filter out value" tooltip-append-to-body="1"></i>


Can we make a few changes here:

Instead of  elements, can we use  elements? (They're more semantic)

Let's format this markup according to our HTML styleguide.

Let's add additional classes to the icons to tweak their styles.

The resulting markup would look like this:

" tooltip="Filter for value" tooltip-append-to-body="1" > " tooltip="Filter out value" tooltip-append-to-body="1" >

BTW I just edited my markup example above... I totally forgot to implement #1 in my list 😄

cjcenizal · 2017-01-06T00:23:48Z

src/ui/public/styles/base.less

+    .table-cell-filter {
+      visibility: visible;
+    }
+  }


Could you move these styles to ui/public/doc_table/doc_table.less and nest everything inside of the discover-table-row class? This should scope everything to just the Discover table.

.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; } } } }

Then can you add these styles to this file, to tweak the appearance of the filter icons? The icons should end up looking like this:

/** * 1. Align icon with text in cell. */ .docTableRowFilterIcon { font-size: 14px; line-height: 1; /* 1 */ & + & { margin-left: 5px; } }

cjcenizal · 2017-01-06T00:29:51Z

src/ui/public/doc_table/components/table_row.js

@@ -24,9 +24,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) {


Thanks for making this improvement! :D

jimmyjones2 · 2017-01-06T21:31:04Z

@cjcenizal Done, thanks for the help/feedback!

cjcenizal · 2017-01-06T22:34:40Z

src/ui/public/doc_table/doc_table.less

+    & + & {
+      margin-left: 5px;
+    }
+  }


One minor tweak: could you un-nest this selector entirely so that this file looks like this:

doc-table { // styles... } /** * 1. Align icon with text in cell. */ .docTableRowFilterIcon { font-size: 14px; line-height: 1; /* 1 */ & + & { margin-left: 5px; } }

This removes unnecessary specificity and fixes a bug caused by the & + & selector.

… & + & selector

jimmyjones2 · 2017-01-08T13:53:06Z

@cjcenizal Done, thanks again for your help

cjcenizal

LGTM!

Bargs

Functionality looks great! I left a few minor inline comments.

@tbragin per @cjcenizal's comment are you good with merging this and updating the data table later?

Bargs · 2017-01-11T22:35:13Z

src/ui/public/doc_table/components/table_row/cell.html

+      <span
+        ng-click="inlineFilter($event, '+')"
+        class="fa fa-search-plus docTableRowFilterIcon"
+        data-column="<%- column %>"


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 %>', '-')"

As I mentioned in my other comment, ignore this.

Bargs · 2017-01-11T22:58:32Z

src/ui/public/doc_table/components/table_row.js

+            ? false
+            : !mapping[column]
+            ? false
+            : mapping[column].filterable;


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 ?

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?

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.

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.

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.

Bargs · 2017-01-11T23:16:39Z

src/ui/public/doc_table/components/table_row.js

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


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.

@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.

@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?

Bargs · 2017-01-11T23:19:20Z

src/ui/public/doc_table/components/table_row.js

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

-          if (i === 0 && !reuse) {
+          if (!reuse) {


Curious why this was changed?

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

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.

jimmyjones2 · 2017-01-14T22:06:00Z

@Bargs Applied requested changes

Bargs · 2017-01-17T21:28:05Z

jenkins, test this

tbragin · 2017-01-18T14:44:05Z

@Bargs I'd personally prefer updating the data table in the same release, so user does not have to deal with two different interactions for some time. So if we merge it, could we not backport until we can get the PR together to get both filtering behaviors to be consistent?

@cjcenizal Would you agree?

cjcenizal · 2017-01-18T16:28:49Z

@tbragin (Caveat: I don't really have enough experience with interacting with Discover and a Data Table in the same workflow to have a good feeling for this, so I'd weight my opinion about 20%.)

Because the Data Table is a smaller piece of the Visualize app, and the table in Discover is a core part of that app's UI, I would be OK with some temporary divergence between the two. If we release in this state, then I think what will happen is:

Users will gain a nice feature of Discover.
They'll use a Data Table, notice the feature is missing, and generate demand for this feature to be ported over (maybe someone will even open a PR).

Would you agree? That doesn't seem so bad to me.

cjcenizal · 2017-01-18T16:47:29Z

For the record, here is an example of how these two UIs look side by side in Dashboard. Our concern is that showing two similar UIs in the same context, but with different interaction paradigms, will create confusion for the user:

lukasolson · 2017-01-18T17:20:01Z

This is awesome! I would have to agree with @tbragin. Consistency between the doc table and data table is important.

Bargs · 2017-01-18T20:16:32Z

Sounds like there's enough agreement that we should implement the same functionality in data table before merging this.

@jimmyjones2 would you have an interest in taking a crack at it? If not (totally understandable) we can pick it up.

jimmyjones2 · 2017-01-20T19:35:51Z

@Bargs Sorry don't have the time at the moment :( Don't want this PR to languish too much longer though

Bargs · 2017-01-20T21:16:01Z

No worries @jimmyjones2, I just didn't want to yank the reins from you if you had a burning desire to keep working on it. We'll try to push it it over the line.

Bargs · 2017-01-20T23:02:27Z

Just put up #9989 to extend this to the data table

Add note about splitting ECS update PRs

tbragin assigned alt74 May 3, 2016

epixa added Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. help wanted adoptme and removed help wanted adoptme labels Oct 8, 2016

Bargs added the :Discovery label Nov 4, 2016

epixa unassigned alt74 Dec 21, 2016

Bargs self-assigned this Dec 27, 2016

Click to filter values in Discover doc table

2db1f9f

cjcenizal suggested changes Jan 6, 2017

View reviewed changes

Changes suggested by @cjcenizal

7a2d69e

cjcenizal added the review label Jan 6, 2017

cjcenizal reviewed Jan 6, 2017

View reviewed changes

Un-nest selector to avoid unnecessary specifity and fix bug caused by…

1082a99

… & + & selector

cjcenizal approved these changes Jan 10, 2017

View reviewed changes

Bargs suggested changes Jan 11, 2017

View reviewed changes

Bargs added updates_needed and removed review labels Jan 12, 2017

Move flattenedRow to the scope as suggested by @Bargs

ebf8c50

Bargs added review and removed updates_needed labels Jan 17, 2017

Bargs added updates_needed and removed review labels Jan 17, 2017

Bargs mentioned this pull request Jan 20, 2017

Click to filter values in Discover doc table and Visualize data table #9989

Merged

lukasolson closed this Jan 20, 2017

MadameSheema pushed a commit to MadameSheema/kibana that referenced this pull request Jan 31, 2023

Merge pull request elastic#5344 from efd6/tranches

4def133

Add note about splitting ECS update PRs

Click to filter values in Discover doc table #5344

Click to filter values in Discover doc table #5344

Conversation

jimmyjones2 commented Nov 6, 2015

elasticsearch-release commented Nov 6, 2015

jimmyjones2 commented Dec 18, 2015

rashidkpc commented Dec 18, 2015

jimmyjones2 commented Feb 9, 2016

elasticsearch-release commented Feb 20, 2016

spalger commented Apr 5, 2016

spalger commented Apr 5, 2016

jimmyjones2 commented Apr 5, 2016

rashidkpc commented Apr 7, 2016

simianhacker commented Apr 7, 2016

spalger commented Apr 7, 2016

tbragin commented Apr 8, 2016

jimmyjones2 commented Apr 10, 2016 • edited

jimmyjones2 commented May 1, 2016

tbragin commented May 3, 2016

epixa commented Oct 8, 2016

elasticmachine commented Oct 8, 2016

jimmyjones2 commented Oct 9, 2016

Bargs commented Dec 27, 2016

tbragin commented Jan 4, 2017

Bargs commented Jan 4, 2017

cjcenizal commented Jan 4, 2017 • edited

jimmyjones2 commented Jan 4, 2017

cjcenizal 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

cjcenizal Jan 6, 2017 • edited

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

jimmyjones2 commented Jan 6, 2017

Choose a reason for hiding this comment

jimmyjones2 commented Jan 8, 2017

cjcenizal left a comment

Choose a reason for hiding this comment

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

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

jimmyjones2 Jan 12, 2017 • edited

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

jimmyjones2 commented Jan 14, 2017

Bargs commented Jan 17, 2017

tbragin commented Jan 18, 2017 • edited

cjcenizal commented Jan 18, 2017

cjcenizal commented Jan 18, 2017

lukasolson commented Jan 18, 2017

Bargs commented Jan 18, 2017

jimmyjones2 commented Jan 20, 2017

Bargs commented Jan 20, 2017

Bargs commented Jan 20, 2017

jimmyjones2 commented Apr 10, 2016 •

edited

cjcenizal commented Jan 4, 2017 •

edited

cjcenizal Jan 6, 2017 •

edited

jimmyjones2 Jan 12, 2017 •

edited

tbragin commented Jan 18, 2017 •

edited