Indicate search results are limited to the sample size in a saved search on dashboard #10827

stacey-gammon · 2017-03-21T12:49:11Z

stacey-gammon · 2017-03-22T19:48:18Z

Jenkins failure: (Fatal error: unexpected end of file�)

Running "esvm:test" (esvm) task
starting up "test" cluster
INFO -  - cluster - Downloading & installing from "master" branch.
Fatal error: unexpected end of file�
runbld>>> <<<<<<<<<<<< SCRIPT EXECUTION END <<<<<<<<<<<<
runbld>>> DURATION: 234394ms
runbld>>> STDOUT: 81598 bytes
runbld>>> STDERR: 2050 bytes

jenkins, test this

spalger · 2017-03-24T17:28:34Z

src/ui/public/doc_table/doc_table.html

@@ -20,6 +20,12 @@
          class="discover-table-row"></tr>
      </tbody>
    </table>
+    <div ng-if="hits.length === sampleSize && page.last" class="discover-table-footer">
+      <center>


@cjcenizal I'm tempted to say we shouldn't use <center> tags, do you agree?

Ah, yes definitely. Centering should be done in CSS, preferably with flexbox, but using text-align: center is also an option. In this case, I'd check the .discover-table-footer class to see if it makes sense to add these styles to that class. If there are situations where we don't want to center it's content, then create a new class, e.g..discover-table-footer-prompt, and apply the style there. The updated markup would look like this:

<div ng-if="hits.length === sampleSize && page.last" class="discover-table-footer"> <div class="discover-table-footer-prompt"> These are the first {{sampleSize}} documents matching your search, refine your search to see others. </div> </div>

Eventually we'll want to replace these styles with UI Framework components, of course.

spalger · 2017-03-24T17:31:24Z

src/ui/public/doc_table/doc_table.html

@@ -20,6 +20,12 @@
          class="discover-table-row"></tr>
      </tbody>
    </table>
+    <div ng-if="hits.length === sampleSize && page.last" class="discover-table-footer">


In discover, $scope.hits is the total number of results. Maybe we should pass that to the doc_table and it should use that to determine if this should be shown (which means it could also tell the user how many rows are hidden).

We can't tell the user how many rows are hidden when using this from dashboard because we don't have the total number of results in that case, we are only passing searchSource.

Discover won't ever show this part because it uses infiniteScroll and passes hits instead of using searchSource & setting hits. I agree it's unintuitive that hits is two different things depending on infiniteScroll and searchSource, but I don't want to get into a refactor since this is a Mend it Monday low fruit issue and if I make larger modifications it should probably go through testing. Thoughts?

Oh the other thing is that in the discover footer now, there is a back to top link where the logic is in discover, not in doc table. I got rid of that, since it doesn't make sense esp if this error message goes on top, not on the bottom.

Forgot that this is used outside of discover

jimmyjones2 · 2017-03-24T19:02:01Z

The other issue in #4161 was that on the first page you don't know that the results have been limited. Could there be some kind of indication on the pager perhaps, or at the top of the first page?

stacey-gammon · 2017-03-27T14:16:56Z

Took into account @jimmyjones2 feedback and moved the warning back to the first page, on top. Split out the styles since it's no longer a footer, and was never part of discover anyhow. And got rid of the center tag.

@cjcenizal @snide I went back to the first page, on top ^^

trevan · 2017-03-27T14:37:32Z

@stacey-gammon, one minor problem I have is that it takes up a lot of space on the first page. I have several dashboards that have multiple discover searches on it. I'd like to see as much data as possible but now with this warning and with the pagination, I see a lot less than I did which requires either scrolling or increasing the panel sizes.

jimmyjones2 · 2017-03-27T16:30:23Z

Could the warning be inline with the pager, so doesn't consume vertical space? Would just "limited to first 500 results" suffice?

stacey-gammon · 2017-03-29T19:48:52Z

Using the new pager styles:

cjcenizal · 2017-03-29T20:06:19Z

Hmm, I think it does look better on the right side. I think we can declutter the UI by hiding the warning message behind a tooltip. @snide @uboness thoughts?

@stacey-gammon If we go this route, we'll need to add a custom class to the Bar element here, which adds margin: 5px 5px 0 to adjust the spacing.

uboness · 2017-03-30T13:57:25Z

+1 on right alignment

regarding the message, not sure if the tooltip will be too hidden. We could have a "calmer" text showing closer to the nav control... not sure... need to see it in action.. might be that a tooltip is good enough

snide · 2017-03-30T17:12:15Z

Right alignment is good. I'd use whatever our light gray text color is and make the text more succinct to keep it from being so heavy. "Limited to 500 results. Refine your search." should get the point across.

stacey-gammon · 2017-03-30T17:18:16Z

@snide - Good suggestions, I'll add them shortly.
@cjcenizal - Note that the warning message only appears on the very last page so I don't think it'll be too intrusive (or at least that's how I have it implemented now). It will only show up after the user pages through 10 times, so I think we want it a little more obvious than the icon (IMO).

uboness · 2017-03-30T17:22:11Z

Note that the warning message only appears on the very last page so I don't think it'll be too intrusive (or at least that's how I have it implemented now). It will only show up after the user pages through 10 times, so I think we want it a little more obvious than the icon (IMO).

++

'd use whatever our light gray text color is and make the text more succinct to keep it from being so heavy. "Limited to 500 results. Refine your search." should get the point across.

++

cjcenizal · 2017-03-30T17:27:17Z

@snide Good suggestions! Let's do it.

stacey-gammon · 2017-03-30T18:16:40Z

Latest iteration:

cjcenizal · 2017-03-30T18:22:17Z

@stacey-gammon Looks good, but the order should go [Limited message] [Result count] [Buttons], to retain as close a pattern to what we currently use:

stacey-gammon · 2017-03-30T18:27:49Z

Oh good call, forgot I swapped them earlier. This looks better.

stacey-gammon · 2017-03-30T19:53:51Z

jenkins test this

spalger

LGTM

stacey-gammon · 2017-03-31T12:58:44Z

@cjcenizal Does looks good refer only to UI/UX or did you check out the code as well?

cjcenizal · 2017-03-31T15:46:06Z

Just the screenshot. I can do a code review too if you want.

stacey-gammon · 2017-03-31T18:38:20Z

@cjcenizal I do need a second reviewer so that would be great, but can tag someone else if you are busy!

cjcenizal

Nice work! I have some suggestions on how to tweak the UI so it looks like this (rows removed for demo purposes):

The warning text and pagination text was slightly off-alignment, so I adjust that, and then added some padding around the tool bars.

cjcenizal · 2017-03-31T22:40:22Z

src/ui/public/doc_table/doc_table.js


          return $scope.searchSource.onResults().then(onResults);
        }).catch(notify.fatal);

        $scope.searchSource.onError(notify.error).catch(notify.fatal);
      }));

+      const limitTo = $filter('limitTo');
+      const calculateItemsOnPage = () => {


How about moving limitTo and calculateItemsOnPage up to line 53 where the other non-scope variables are? This way the non-scope and scope variables/methods will be grouped together, and you can remove the eslint-disable-line annotation on 111.

cjcenizal · 2017-03-31T22:42:08Z

src/ui/public/doc_table/doc_table.less

+    background-color: @doc-table-warning-bg;
+    padding: 5px 10px;
+    text-align: center;
+  }


Is this class being used anywhere?

cjcenizal · 2017-03-31T22:42:56Z

src/ui/public/doc_table/doc_table.html

+  ng-class="{ loading: searchSource.activeFetchCount > 0 }"
+>
+  <div ng-if="!infiniteScroll">
+    <div class="kuiBar">


I updated UI Framework with a new treatment for making the text gray. Can you rebase and update the markup to this:

<div class="kuiBar docTableBar"> <div class="kuiBarSection"> <div ng-if="shouldShowLimitedResultsWarning()" class="kuiToolBarText kuiSubduedText"> {{ limitedResultsWarning }} </div>

cjcenizal · 2017-03-31T22:44:14Z

src/ui/public/doc_table/doc_table.less

@@ -8,6 +8,16 @@ doc-table {
  flex: 1 1 100%;
  flex-direction: column; /* 1 */

+  .docTablePagerInfoMessage {


Can you delete this class and add this new one instead? And can you unnest it and put it on line 3, outside of doc-table?

.docTableBar { margin: 5px 5px 0; }

cjcenizal · 2017-03-31T22:44:44Z

src/ui/public/doc_table/doc_table.html

@@ -20,7 +40,27 @@
          class="discover-table-row"></tr>
      </tbody>
    </table>
-  </paginate>
+    <!-- ToolBarFooter -->
+    <div class="kuilBar">


And this can be updated too:

<div class="kuilBar docTableBar"> <div class="kuiBarSection"> <div ng-if="shouldShowLimitedResultsWarning()" class="kuiToolBarText kuiSubduedText"> {{ limitedResultsWarning }} </div>

cjcenizal · 2017-03-31T22:45:10Z

src/ui/public/styles/variables/for-theme.less

@@ -41,6 +41,7 @@

 @discover-table-footer-bg: @well-bg;
 @discover-field-filter-bg: @well-bg;
+@doc-table-warning-bg: @well-bg;


You can delete this.

cjcenizal · 2017-03-31T22:47:35Z

src/ui/public/filter_bar/filter_bar.js

@@ -46,10 +45,6 @@ module.directive('filterBar', function (Private, Promise, getAppState) {

      $scope.state = getAppState();

-      // Don't show filter "pinnability" when in embedded mode, as it doesn't make sense in that context
-      // as there will be no cross app navigation for which the filter should persist.
-      $scope.showFilterPin = () => chrome.getVisible();


Can you explain the context behind these changes? At first glance they don't seem related to the other changes in this PR.

Weird. I had another PR that accidentally got merged into this one, so I reverted it, hence the commit history on this one was a little weird: 1) merge unrelated PR. 2) Do actual work. 3) Notice mistake and revert PR.

But now that original PR is merged for real, and I guess the revert is making this show up as actually removing that code.

Anyway, good catch, I'll add it back in.

Showing the message only on the last page, at the bottom of the search results.

…ount

stacey-gammon · 2017-04-04T14:48:52Z

@cjcenizal Feedback addressed, lmk if there is anything else!

cjcenizal

Awesome! I found a couple more minor things and then we're good to go.

cjcenizal · 2017-04-04T15:14:09Z

src/ui/public/doc_table/doc_table.html

+      <div class="kuiBarSection">
+        <div
+          ng-if="shouldShowLimitedResultsWarning()"
+          class="kuiToolBarText kuiSubduedText">


This closing bracket should be on the next line.

cjcenizal · 2017-04-04T15:14:20Z

src/ui/public/doc_table/doc_table.html

+      <div class="kuiBarSection">
+        <div
+          ng-if="shouldShowLimitedResultsWarning()"
+          class="kuiToolBarText kuiSubduedText">


Same here re closing bracket.

cjcenizal · 2017-04-04T15:16:51Z

src/ui/public/doc_table/doc_table.html

+  ng-class="{ loading: searchSource.activeFetchCount > 0 }"
+>
+  <div ng-if="!infiniteScroll">
+    <div class="kuilBar docTableBar">


Looks like a typo here: kuilBar should be kuiBar.

cjcenizal · 2017-04-04T15:16:57Z

src/ui/public/doc_table/doc_table.html

@@ -23,7 +45,29 @@
        ></tr>
      </tbody>
    </table>
-  </paginate>
+    <!-- ToolBarFooter -->
+    <div class="kuilBar docTableBar">


kuilBar should be kuiBar.

cjcenizal

LGTM

…rch on dashboard (elastic#10827) * Indicate results are limited to 500 in a saved search on dashboard Showing the message only on the last page, at the bottom of the search results. * Put warning on top, not on bottom, separate styles * Use landing page style pager buttons and show the correct total hit count * Move back to the right, shorten error message, lighten text * swap order of pager buttons and numbers * Address code review comments * Code review comments part 2

…rch on dashboard (#10827) (#11075) * Indicate results are limited to 500 in a saved search on dashboard Showing the message only on the last page, at the bottom of the search results. * Put warning on top, not on bottom, separate styles * Use landing page style pager buttons and show the correct total hit count * Move back to the right, shorten error message, lighten text * swap order of pager buttons and numbers * Address code review comments * Code review comments part 2

stacey-gammon requested review from snide and weltenwort March 21, 2017 12:49

stacey-gammon added review v5.4.0 labels Mar 21, 2017

tbragin added the :Sharing label Mar 21, 2017

stacey-gammon force-pushed the warn-limited-doc-table-view branch from bd6a2bb to 2bd57fa Compare March 22, 2017 19:35

stacey-gammon force-pushed the warn-limited-doc-table-view branch from 2bd57fa to 8f5a148 Compare March 23, 2017 12:54

spalger reviewed Mar 24, 2017

View reviewed changes

stacey-gammon force-pushed the warn-limited-doc-table-view branch 2 times, most recently from 23d0d62 to e361825 Compare March 27, 2017 14:15

stacey-gammon mentioned this pull request Mar 27, 2017

Indicate dashboard search panel limit of 500 results #4161

Closed

stacey-gammon force-pushed the warn-limited-doc-table-view branch from e361825 to ca1cc4a Compare March 29, 2017 19:48

stacey-gammon added the Feature:Dashboard Dashboard related features label Mar 30, 2017

spalger approved these changes Mar 30, 2017

View reviewed changes

stacey-gammon removed the request for review from snide March 31, 2017 12:57

stacey-gammon removed the request for review from weltenwort March 31, 2017 18:38

cjcenizal suggested changes Mar 31, 2017

View reviewed changes

stacey-gammon force-pushed the warn-limited-doc-table-view branch 3 times, most recently from 8690e14 to 14a0aa4 Compare April 1, 2017 10:53

stacey-gammon added 6 commits April 3, 2017 09:06

Indicate results are limited to 500 in a saved search on dashboard

bc2ad26

Showing the message only on the last page, at the bottom of the search results.

Put warning on top, not on bottom, separate styles

1efe5ae

Use landing page style pager buttons and show the correct total hit c…

f6b13ab

…ount

Move back to the right, shorten error message, lighten text

2f14418

swap order of pager buttons and numbers

ad47941

Address code review comments

95f836e

stacey-gammon force-pushed the warn-limited-doc-table-view branch from 14a0aa4 to 95f836e Compare April 3, 2017 13:06

cjcenizal suggested changes Apr 4, 2017

View reviewed changes

Code review comments part 2

4bc704f

cjcenizal approved these changes Apr 5, 2017

View reviewed changes

stacey-gammon merged commit a08f0db into elastic:master Apr 6, 2017

stacey-gammon mentioned this pull request Apr 6, 2017

[5.x] Indicate search results are limited to the sample size in a saved sea… #11075

Merged

stacey-gammon deleted the warn-limited-doc-table-view branch April 6, 2017 14:44

stacey-gammon mentioned this pull request Apr 26, 2017

Visualize landing page doesn't handle more than 100 visualizations #11415

Closed

Indicate search results are limited to the sample size in a saved search on dashboard #10827

Indicate search results are limited to the sample size in a saved search on dashboard #10827

Conversation

stacey-gammon commented Mar 21, 2017

stacey-gammon commented Mar 22, 2017

Choose a reason for hiding this comment

cjcenizal Mar 24, 2017 • edited Loading

Choose a reason for hiding this comment

spalger Mar 24, 2017 • edited Loading

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 Mar 24, 2017

stacey-gammon commented Mar 27, 2017

trevan commented Mar 27, 2017

jimmyjones2 commented Mar 27, 2017

stacey-gammon commented Mar 29, 2017

cjcenizal commented Mar 29, 2017 • edited Loading

uboness commented Mar 30, 2017

snide commented Mar 30, 2017

stacey-gammon commented Mar 30, 2017

uboness commented Mar 30, 2017

cjcenizal commented Mar 30, 2017

stacey-gammon commented Mar 30, 2017

cjcenizal commented Mar 30, 2017

stacey-gammon commented Mar 30, 2017

stacey-gammon commented Mar 30, 2017

spalger left a comment

Choose a reason for hiding this comment

stacey-gammon commented Mar 31, 2017

cjcenizal commented Mar 31, 2017

stacey-gammon commented Mar 31, 2017

cjcenizal left a comment • edited Loading

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

stacey-gammon commented Apr 4, 2017

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal left a comment

Choose a reason for hiding this comment

cjcenizal Mar 24, 2017 •

edited

Loading

spalger Mar 24, 2017 •

edited

Loading

cjcenizal commented Mar 29, 2017 •

edited

Loading

cjcenizal left a comment •

edited

Loading

cjcenizal Apr 4, 2017 •

edited

Loading