[Discover] Move total hits counter from histogram to grid area. New controls in histogram. #171638

jughosta · 2023-11-21T14:01:19Z

Closes [Discover] Migrate number of hits from UnifiedHistogram to UnifiedDataTable #168825
Closes [Discover] Align Fieldlist and Histogram hide / show buttons #171610
Closes [UnifiedHistogram] Redesign Buttons, Breakdown and Interval #167427
Partially addresses [Unified histogram][ES|QL] Extra fetches in specific scenarios #165192

Summary

This PR moves the total hits counter closer to the grid, updates histogram controls and introduces new panel toggle buttons for toggling fields sidebar and histogram.

Testing

When testing, please check collapsing/expanding the fields sidebar and histogram. Also for ES|QL mode with suggestions, legacy table, no results and error prompt, Field Statistics tab, data views without a time field, light/dark themes.

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

…fix'

…825-move-total-hits

…fix'

…825-move-total-hits

…fix'

…825-move-total-hits

davismcphee

Finished my initial code review, and the changes are looking great so far! I'll follow up tomorrow with some local testing and another review.

davismcphee · 2024-01-09T02:25:49Z

...st/src/containers/unified_field_list_sidebar/sidebar_toggle_button/sidebar_toggle_button.tsx

Thanks for adding the aria labels here, it's great to keep a11y in mind 👍

davismcphee · 2024-01-09T04:03:00Z

src/plugins/unified_histogram/public/chart/breakdown_field_selector.test.tsx

It's great that you converted this to RTL, thanks!

kertal · 2024-01-09T07:37:23Z

@kertal: FWIW, a term that came up a few times in recent usability tests was "Events" (rather than "Documents" or "Hits"). Perhaps this is a more recognizable and generic term? CCing @alexmarhaba.

IMO this would apply to time based documents. But in Discover we also support Non-time based documents searches

andreadelrio

@jughosta Looking great! Nit, can we remove the 8px gap from here?

From:

To:

andreadelrio

🚀 Design changes LGTM!

- Fix double EuiSelectable focus ring styles - Fix issue where toolbar buttons get cut off on smaller screen sizes - Add tooltips to toolbar buttons so users can see the full labels when truncated - Tweak toolbar button menu designs to more closely match mockups (full-width menu items, compressed search bar, show all time interval menu items without scrolling) - Add middle truncation to toolbar button menus for long values, and update search bar placeholder to "Search" to match mockups - Autofocus on toolbar button menu selectables when popover is opened - Update toolbar button menu "no results" display to put message and search term on one line - Remove extra 2 pixels below Lens suggestion selector caused by `display: inline-block`, and update tooltip delay to "long" to match the toolbar button tooltips

davismcphee

Awesome work on this! It's such an improvement over what we had before, and makes the entire design feel more complete and cohesive 🎉 I've tested this PR pretty thoroughly at this point, including the listed test scenarios (thanks a lot for including those), and I haven't encountered any bugs 🙌

I did have a few UX concerns, but rather than just listing them out here, I figured I'd have a shot at addressing them to help move things along with this PR 🙂 I included all of the changes in the PR description for reference, so feel free to merge or adjust as needed if you think any of the changes don't make sense: jughosta#9.

The only other concern I had that I couldn't address since it would first require discussion and collaboration with Shared UX is that it would be nice if we used EUI tooltips for the toolbar buttons to match our other UI elements and the data grid toolbar buttons. But this doesn't need to be addressed in this PR of course, and we can talk to them to see what they think and create a followup PR later:

I'll be out tomorrow morning, but I'll do another review in the afternoon once I'm back and you've gotten a chance to check out the touchups PR. Thanks for all the work on this, and it's looking very close to being ready to merge!

Total hits and histogram control touchups

davismcphee

I did a final quick round of testing and code review, and it LGTM! Excellent job on this, all of the design refresh work really makes Discover feel like a much cleaner and more modern app. I especially like how much space the new collapsible section designs provide for the data grid (almost makes fullscreen mode feel unnecessary now) 🎉

davismcphee · 2024-01-11T19:57:07Z

src/plugins/discover/public/application/main/components/layout/discover_histogram_layout.tsx

+  const renderCustomChartToggleActions = useCallback(
+    () =>
+      React.isValidElement(panelsToggle)
+        ? React.cloneElement(panelsToggle, { renderedFor: 'histogram' })
+        : panelsToggle,
+    [panelsToggle]
+  );


This seems to work well for what we're doing here, but I think the use of cloneElement in this PR may be a bit of a code smell. It seems the React team now considers it a "legacy" API and is recommending alternatives like render props instead, which might be an option for us. That said, I have no problem merging this as is, just something to consider as potential followup work for cleanup.

davismcphee · 2024-01-11T20:08:52Z

src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts

Thanks for the cleanup in here btw 👍 It still feels more complicated than it should be to manage our refetches, but this definitely removes some of the complexity.

davismcphee · 2024-01-11T20:28:38Z

src/plugins/unified_histogram/public/layout/layout.tsx

+  // eslint-disable-next-line react-hooks/exhaustive-deps
+  const chartMemoized = useMemo(() => chart, [chart?.title, chart?.timeInterval, chart?.hidden]);
+  // eslint-disable-next-line react-hooks/exhaustive-deps
+  const hitsMemoized = useMemo(() => hits, [hits?.status, hits?.total]);


Just curious if the lack of memoization before was causing any issues in this PR, or if this was just added for convenience to prevent consumers having to do their own memoization? It's not an issue to do it here IMO (although I don't love having to disable ESLint rules personally), I'm just wondering. In retrospect, given how Unified Histogram has evolved I probably should have done this differently from the start and skipped the context objects altogether (I guess hindsight is 20/20).

@davismcphee That was something I added when trying to reduce the number of Chart rerenderings. I can't prove now that it's still relevant so I'm removing this code.

kibana-ci · 2024-01-12T10:36:27Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: f927aee

Failed CI Steps

Investigations - Security Solution Cypress Tests #5

Test Failures

[job] [logs] Investigations - Security Solution Cypress Tests #5 / [ESS] Save Timeline Prompts "before each" hook for "should NOT prompt when navigating with a changed & saved timeline within security solution where timelines are disabled" "before each" hook for "should NOT prompt when navigating with a changed & saved timeline within security solution where timelines are disabled"

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`discover`	694	698	+4
`unifiedHistogram`	68	109	+41
total			+45

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`canvas`	1013.7KB	1013.8KB	+38.0B
`discover`	559.5KB	564.1KB	+4.6KB
`unifiedHistogram`	51.0KB	69.3KB	+18.2KB
total			+22.9KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`@kbn/unified-field-list`	9	10	+1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`core`	375.2KB	375.4KB	+198.0B
`unifiedHistogram`	6.3KB	6.4KB	+54.0B
total			+252.0B

Unknown metric groups

API count

id	before	after	diff
`@kbn/shared-ux-button-toolbar`	26	30	+4

async chunk count

id	before	after	diff
`unifiedHistogram`	2	3	+1

ESLint disabled line counts

id	before	after	diff
`discover`	24	25	+1

Total ESLint disabled count

id	before	after	diff
`discover`	24	25	+1

History

💚 Build #186946 succeeded 3cc96c3
💛 Build #186885 was flaky 5d10a7d
💚 Build #186861 succeeded 125e9d7
💚 Build #186752 succeeded 6b0976e
💚 Build #186297 succeeded 5b1ea10
💛 Build #185820 was flaky b501606

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

cc @jughosta @andreadelrio

…ontrols in histogram. (elastic#171638) - Closes elastic#168825 - Closes elastic#171610 - Closes elastic#167427 - Partially addresses elastic#165192 This PR moves the total hits counter closer to the grid, updates histogram controls and introduces new panel toggle buttons for toggling fields sidebar and histogram. <img width="500" alt="Screenshot 2023-12-05 at 15 37 20" src="https://github.com/elastic/kibana/assets/1415710/5b9bd771-1052-4205-849f-18c21cc299b8"> <img width="500" alt="Screenshot 2023-12-05 at 15 37 29" src="https://github.com/elastic/kibana/assets/1415710/e5941b27-c497-4d7e-b461-68b66931475a"> <img width="500" alt="Screenshot 2023-12-05 at 15 37 37" src="https://github.com/elastic/kibana/assets/1415710/97abd32e-9ff2-4d9a-b7e7-b9d6d9cf64db"> <img width="500" alt="Screenshot 2023-12-05 at 15 37 50" src="https://github.com/elastic/kibana/assets/1415710/10f2b4f4-ec37-41c3-b78b-78c64e14d655"> <img width="400" alt="Screenshot 2023-12-05 at 15 37 59" src="https://github.com/elastic/kibana/assets/1415710/ef2e28b2-f6ba-4ccb-aea4-3946ba2d5839"> <img width="300" alt="Screenshot 2023-12-05 at 15 38 05" src="https://github.com/elastic/kibana/assets/1415710/07901ede-0bcb-46a6-a398-4562189fd54f"> <img width="500" alt="Screenshot 2023-12-05 at 15 40 38" src="https://github.com/elastic/kibana/assets/1415710/17830115-2111-4b8f-ae40-7b5875c06879"> <img width="500" alt="Screenshot 2023-12-05 at 15 40 56" src="https://github.com/elastic/kibana/assets/1415710/975d475b-280b-495a-b7b7-31c7ade5f21e"> <img width="500" alt="Screenshot 2023-12-05 at 15 43 08" src="https://github.com/elastic/kibana/assets/1415710/38b6053a-e260-48d8-9591-3f3409df2876"> When testing, please check collapsing/expanding the fields sidebar and histogram. Also for ES|QL mode with suggestions, legacy table, no results and error prompt, Field Statistics tab, data views without a time field, light/dark themes. - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co> Co-authored-by: Davis McPhee <davis.mcphee@elastic.co>

## Summary Following this PR #171638, this renames the histogram query to use the word results instead of rows. <img width="1672" alt="image" src="https://github.com/elastic/kibana/assets/17003240/9b917bd2-e961-43eb-9aad-6fefd5e38f9c"> ### Checklist - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

…ontrols in histogram. (elastic#171638) - Closes elastic#168825 - Closes elastic#171610 - Closes elastic#167427 - Partially addresses elastic#165192 ## Summary This PR moves the total hits counter closer to the grid, updates histogram controls and introduces new panel toggle buttons for toggling fields sidebar and histogram. <img width="500" alt="Screenshot 2023-12-05 at 15 37 20" src="https://github.com/elastic/kibana/assets/1415710/5b9bd771-1052-4205-849f-18c21cc299b8"> <img width="500" alt="Screenshot 2023-12-05 at 15 37 29" src="https://github.com/elastic/kibana/assets/1415710/e5941b27-c497-4d7e-b461-68b66931475a"> <img width="500" alt="Screenshot 2023-12-05 at 15 37 37" src="https://github.com/elastic/kibana/assets/1415710/97abd32e-9ff2-4d9a-b7e7-b9d6d9cf64db"> <img width="500" alt="Screenshot 2023-12-05 at 15 37 50" src="https://github.com/elastic/kibana/assets/1415710/10f2b4f4-ec37-41c3-b78b-78c64e14d655"> <img width="400" alt="Screenshot 2023-12-05 at 15 37 59" src="https://github.com/elastic/kibana/assets/1415710/ef2e28b2-f6ba-4ccb-aea4-3946ba2d5839"> <img width="300" alt="Screenshot 2023-12-05 at 15 38 05" src="https://github.com/elastic/kibana/assets/1415710/07901ede-0bcb-46a6-a398-4562189fd54f"> <img width="500" alt="Screenshot 2023-12-05 at 15 40 38" src="https://github.com/elastic/kibana/assets/1415710/17830115-2111-4b8f-ae40-7b5875c06879"> <img width="500" alt="Screenshot 2023-12-05 at 15 40 56" src="https://github.com/elastic/kibana/assets/1415710/975d475b-280b-495a-b7b7-31c7ade5f21e"> <img width="500" alt="Screenshot 2023-12-05 at 15 43 08" src="https://github.com/elastic/kibana/assets/1415710/38b6053a-e260-48d8-9591-3f3409df2876"> ## Testing When testing, please check collapsing/expanding the fields sidebar and histogram. Also for ES|QL mode with suggestions, legacy table, no results and error prompt, Field Statistics tab, data views without a time field, light/dark themes. ### Checklist - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co> Co-authored-by: Davis McPhee <davis.mcphee@elastic.co>

## Summary Following this PR elastic#171638, this renames the histogram query to use the word results instead of rows. <img width="1672" alt="image" src="https://github.com/elastic/kibana/assets/17003240/9b917bd2-e961-43eb-9aad-6fefd5e38f9c"> ### Checklist - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

…ontrols in histogram. (elastic#171638) - Closes elastic#168825 - Closes elastic#171610 - Closes elastic#167427 - Partially addresses elastic#165192 ## Summary This PR moves the total hits counter closer to the grid, updates histogram controls and introduces new panel toggle buttons for toggling fields sidebar and histogram. <img width="500" alt="Screenshot 2023-12-05 at 15 37 20" src="https://github.com/elastic/kibana/assets/1415710/5b9bd771-1052-4205-849f-18c21cc299b8"> <img width="500" alt="Screenshot 2023-12-05 at 15 37 29" src="https://github.com/elastic/kibana/assets/1415710/e5941b27-c497-4d7e-b461-68b66931475a"> <img width="500" alt="Screenshot 2023-12-05 at 15 37 37" src="https://github.com/elastic/kibana/assets/1415710/97abd32e-9ff2-4d9a-b7e7-b9d6d9cf64db"> <img width="500" alt="Screenshot 2023-12-05 at 15 37 50" src="https://github.com/elastic/kibana/assets/1415710/10f2b4f4-ec37-41c3-b78b-78c64e14d655"> <img width="400" alt="Screenshot 2023-12-05 at 15 37 59" src="https://github.com/elastic/kibana/assets/1415710/ef2e28b2-f6ba-4ccb-aea4-3946ba2d5839"> <img width="300" alt="Screenshot 2023-12-05 at 15 38 05" src="https://github.com/elastic/kibana/assets/1415710/07901ede-0bcb-46a6-a398-4562189fd54f"> <img width="500" alt="Screenshot 2023-12-05 at 15 40 38" src="https://github.com/elastic/kibana/assets/1415710/17830115-2111-4b8f-ae40-7b5875c06879"> <img width="500" alt="Screenshot 2023-12-05 at 15 40 56" src="https://github.com/elastic/kibana/assets/1415710/975d475b-280b-495a-b7b7-31c7ade5f21e"> <img width="500" alt="Screenshot 2023-12-05 at 15 43 08" src="https://github.com/elastic/kibana/assets/1415710/38b6053a-e260-48d8-9591-3f3409df2876"> ## Testing When testing, please check collapsing/expanding the fields sidebar and histogram. Also for ES|QL mode with suggestions, legacy table, no results and error prompt, Field Statistics tab, data views without a time field, light/dark themes. ### Checklist - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co> Co-authored-by: Davis McPhee <davis.mcphee@elastic.co>

## Summary Following this PR elastic#171638, this renames the histogram query to use the word results instead of rows. <img width="1672" alt="image" src="https://github.com/elastic/kibana/assets/17003240/9b917bd2-e961-43eb-9aad-6fefd5e38f9c"> ### Checklist - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

[Discover] Move total hits counter from histogram to grid area

de2700e

jughosta added Feature:Discover Discover Application release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Graph) labels Nov 21, 2023

jughosta self-assigned this Nov 21, 2023

kibanamachine and others added 24 commits November 21, 2023 14:14

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

885c3da

…fix'

[Discover] Update translations

8fed9c5

Merge remote-tracking branch 'origin/168825-move-total-hits' into 168…

6e16afa

…825-move-total-hits

Merge branch 'main' into 168825-move-total-hits

8fecdfc

[Discover] Update tests

c93b8ea

[Discover] Update tests

86e305d

[Discover] Update styles of histogram actions

75af937

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

fc1e428

…fix'

[Discover] Add "Hide chart" button

188a58b

Merge remote-tracking branch 'origin/168825-move-total-hits' into 168…

62841a4

…825-move-total-hits

[Discover] Add "Show chart" button

3863e00

Merge branch 'main' into 168825-move-total-hits

82649fe

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

7a171a0

…fix'

[Discover] Add "Show sidebar" button

19966ab

Merge remote-tracking branch 'origin/168825-move-total-hits' into 168…

d4bce02

…825-move-total-hits

[Discover] Update toggle button

24fc062

[Discover] Improve panel toggles

ee93092

[Discover] Add "Time interval" selector

4985f16

[Discover] Update tests

deca5a0

[Discover] Update tests

a59b504

[Discover] Clean up translations

6d998aa

[Discover] Update tests

cc5c199

[Discover] Fix syntax

36d39bb

[Discover] Update tests

186e1ba

davismcphee reviewed Jan 9, 2024

View reviewed changes

andreadelrio self-requested a review January 10, 2024 02:08

andreadelrio requested changes Jan 10, 2024

View reviewed changes

jughosta added 2 commits January 10, 2024 10:35

[Discover] Fix margin

eef8bd0

Merge branch 'main' into 168825-move-total-hits

6b0976e

andreadelrio self-requested a review January 10, 2024 17:24

andreadelrio approved these changes Jan 10, 2024

View reviewed changes

Merge branch 'main' into 168825-move-total-hits

125e9d7

jughosta enabled auto-merge (squash) January 10, 2024 17:39

jughosta disabled auto-merge January 10, 2024 19:20

jughosta and others added 2 commits January 10, 2024 21:47

Merge branch 'main' into 168825-move-total-hits

5d10a7d

davismcphee reviewed Jan 11, 2024

View reviewed changes

stratoula mentioned this pull request Jan 11, 2024

[ES|QL][Discover] Change rows to results in histogram #174665

Merged

1 task

Merge pull request #9 from davismcphee/move-total-hits-touchups

3cc96c3

Total hits and histogram control touchups

davismcphee approved these changes Jan 11, 2024

View reviewed changes

jughosta added 2 commits January 12, 2024 10:24

[Discover] Clean up

a691e1d

Merge branch 'main' into 168825-move-total-hits

f927aee

jughosta enabled auto-merge (squash) January 12, 2024 09:27

jughosta merged commit aa33843 into elastic:main Jan 12, 2024
39 checks passed

kibanamachine added the v8.13.0 label Jan 12, 2024

jughosta deleted the 168825-move-total-hits branch January 12, 2024 10:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] Move total hits counter from histogram to grid area. New controls in histogram. #171638

[Discover] Move total hits counter from histogram to grid area. New controls in histogram. #171638

jughosta commented Nov 21, 2023 •

edited

Loading

davismcphee left a comment

davismcphee Jan 9, 2024

davismcphee Jan 9, 2024

kertal commented Jan 9, 2024

andreadelrio left a comment

andreadelrio left a comment

davismcphee left a comment

davismcphee left a comment

davismcphee Jan 11, 2024

davismcphee Jan 11, 2024

davismcphee Jan 11, 2024

jughosta Jan 12, 2024

kibana-ci commented Jan 12, 2024

API count

async chunk count

ESLint disabled line counts

Total ESLint disabled count

[Discover] Move total hits counter from histogram to grid area. New controls in histogram. #171638

[Discover] Move total hits counter from histogram to grid area. New controls in histogram. #171638

Conversation

jughosta commented Nov 21, 2023 • edited Loading

Summary

Testing

Checklist

davismcphee left a comment

Choose a reason for hiding this comment

davismcphee Jan 9, 2024

Choose a reason for hiding this comment

davismcphee Jan 9, 2024

Choose a reason for hiding this comment

kertal commented Jan 9, 2024

andreadelrio left a comment

Choose a reason for hiding this comment

andreadelrio left a comment

Choose a reason for hiding this comment

davismcphee left a comment

Choose a reason for hiding this comment

davismcphee left a comment

Choose a reason for hiding this comment

davismcphee Jan 11, 2024

Choose a reason for hiding this comment

davismcphee Jan 11, 2024

Choose a reason for hiding this comment

davismcphee Jan 11, 2024

Choose a reason for hiding this comment

jughosta Jan 12, 2024

Choose a reason for hiding this comment

kibana-ci commented Jan 12, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Async chunks

Public APIs missing exports

Page load bundle

API count

async chunk count

ESLint disabled line counts

Total ESLint disabled count

History

jughosta commented Nov 21, 2023 •

edited

Loading