[Discover] [Unified Data Table] Add document comparison mode by davismcphee · Pull Request #166577 · elastic/kibana

davismcphee · 2023-09-18T01:16:16Z

Summary

This PR adds a new document comparison mode to Discover and Unified Data Table:

Flaky test runner x50: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5590.

Resolves #93567.

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

andreadelrio · 2024-02-07T04:54:23Z

@davismcphee this is looking amazing! I'm sending you a first round of design-related comments and ideas I would like us to consider:

Highlight the "Selected" button more by using the success notification badge.

Use buttons in the style of the ones we have for the document table data grid. The buttons would be: Show diff, Diff settings, Exit comparison mode.
Have a text indicating the mode we're in and the number of documents under comparison (i.e. Comparing 3 documents). This would follow the same format we have planned for the 1,245 documents text in the main view.
Add a pinFilled icon to the header of the first column.

Use a toggle button for the Show diff toggle. See screenshot below for the on styling.
Use a different background color for the pinned column. Maybe subdued?

Have a consolidated Diff settings menu (see below). Where each badge shows a tooltip explaining what Advanced mode means.

Are we planning on having the option to disable the zebra stripping?

davismcphee · 2024-03-25T00:54:16Z

/ci

davismcphee · 2024-03-26T14:39:37Z

/ci

davismcphee

Thanks for the feedback! I've made some changes based on the suggestions and updated mockups.

What should we do with smart fields?

I think we should treat this as a known bug and address it when we integrate smart fields as a native Discover feature, similar to what we're planning for the field statistics tab. But I can definitely create a separate issue to track it so we don't forget.

From what I understand we pick documents by their id when rendering columns for comparison, right? The thing is that documents can have same ids but come from different indices. We had such bug in DocViewer in the past.

The comparison mode is based on our existing selected documents logic, which depends on the generated ID in DataTableRecord to guarantee uniqueness. The only place we use the raw document ID is for displaying in the comparison table headers (so we could technically see a duplicate ID in the headers, although I think the chances of that happening are quite low, and the functionality will still work as expected):

kibana/packages/kbn-discover-utils/src/utils/get_doc_id.ts

Lines 10 to 17 in 001f24c

    
           /** 
        
            * Returning a generated id of a given ES document, since `_id` can be the same 
        
            * when using different indices and shard routing 
        
            */ 
        
           export const getDocId = (doc: EsHitRecord & { _routing?: string }) => { 
        
             const routing = doc._routing ? doc._routing : ''; 
        
             return [doc._index, doc._id, routing].join('::'); 
        
           };

...ges/kbn-unified-data-table/src/components/compare_documents/hooks/use_comparison_columns.tsx

packages/kbn-unified-data-table/src/components/data_table_document_selection.tsx

packages/kbn-unified-data-table/src/components/compare_documents/comparison_controls.tsx

amyjtechwriter

LGTM, labels and docs side. Hopefully I have answered all the comments tagged in but if I've missed anything please let me know!

jughosta

Looking great!

I like the separate "Compare" button! The new highlight on both buttons seem a bit distracting though.

I also tested a case when user selects documents for comparison, then changes the time range and those documents are gone. We could probably improve UI by showing a message that documents are not available anymore or something like it.

Currently, when 1 of 2 documents is gone after refetch:

When 2 out of 2 documents are gone:

jughosta · 2024-04-08T09:14:43Z

packages/kbn-unified-data-table/src/components/compare_documents/compare_documents.tsx

+  setSelectedDocs,
+  setIsCompareActive,
+}: CompareDocumentsProps) => {
+  const [showDiff, setShowDiff] = useLocalStorage(getStorageKey(consumer, 'ShowDiff'), true);


If user toggles "Show diff" off, exits the comparison mode, then next time they press "Compare" button, should not it get enabled again? Otherwise why to go to comparison mode if it's off.

I think we should keep this in local storage to be consistent with the other settings. In most cases users probably won't have a reason to disable diffing, but there are some legitimate use cases such as just wanting to view multiple documents in a vertical format similar to the doc viewer, or wanting to retain the original colours when viewing documents that use the colour field formatter instead of having them overwritten by the diff colours. This way they can exist comparison mode, select more documents, and return to comparison mode without having to keep disabling "Show diff".

No diff:

Diff:

andreadelrio · 2024-04-08T17:43:03Z

I like the separate "Compare" button! The new highlight on both buttons seem a bit distracting though.

@davismcphee I agree with Julia. I think with the new dedicated Compare button we can get rid of the success color in the buttons. This will also help us achieve consistency with the rest of the buttons.

andreadelrio

@davismcphee Thanks for implementing the changes to this menu. Adding some comments below.

I think leaving the Basic/Advanced options enabled when Show diff is set to off looks a bit odd. We could consider disabling them when Show diff is set to off and adding a tooltip explaining why such as "You need to enable Show diff".

Same goes for the Show diff decorations toggle. If Show diff is set to off, this toggle should be disabled.

…diff is off

…loses access to them

davismcphee

@jughosta @andreadelrio thanks both for the feedback!

I updated both of the button badges back to the original colour now:

I also updated the settings menu to disable the diff mode entries and decorations switch, and added a tooltip, when "Show diff" is off:

I also tested a case when user selects documents for comparison, then changes the time range and those documents are gone. We could probably improve UI by showing a message that documents are not available anymore or something like it.

@jughosta good catch! Rather than showing a message when the documents are no longer available, I instead made it so the columns remain visible in the comparison table until the user exists comparison mode: 8c5bcaa. I feel like this is a bit more friendly to users so they don't accidentally lose the documents they're comparing if they trigger a refetch or have auto refresh enabled. WDYT?

...ges/kbn-unified-data-table/src/components/compare_documents/hooks/use_comparison_columns.tsx

davismcphee · 2024-04-09T02:02:56Z

packages/kbn-unified-data-table/src/components/compare_documents/compare_documents.tsx

+  setSelectedDocs,
+  setIsCompareActive,
+}: CompareDocumentsProps) => {
+  const [showDiff, setShowDiff] = useLocalStorage(getStorageKey(consumer, 'ShowDiff'), true);


I think we should keep this in local storage to be consistent with the other settings. In most cases users probably won't have a reason to disable diffing, but there are some legitimate use cases such as just wanting to view multiple documents in a vertical format similar to the doc viewer, or wanting to retain the original colours when viewing documents that use the colour field formatter instead of having them overwritten by the diff colours. This way they can exist comparison mode, select more documents, and return to comparison mode without having to keep disabling "Show diff".

No diff:

Diff:

jughosta

Great work, Davis! My favourite feature now! 👍 🎉
And thanks, @kertal, for proposing it!

andreadelrio

Superb job on this Davis! I'm approving now but I would like to confirm with @amyjtechwriter whether the text in the new tooltip looks good to her.

amyjtechwriter · 2024-04-10T11:54:10Z

Hey hey, we can keep "You need to enable Show Diff" or if you think it's just as clear we could just use "Enable Show diff". I don't have a super strong preference here, so I think go with the one you like best :)

kertal

Such a great feature, so much great fine tuning! Really great work 🥳 Let's 🚢 !

Great work, Davis! My favourite feature now! 👍 🎉
And thanks, @kertal, for proposing it!

Thx to @andreadelrio for the mockups in 2021 built by just a very basic description in the issue we resolve. Something like this is so important showing a potential useful concept in action. One can write a lot of good description how something should work, but an animated mock is always better 👍

kibana-ci · 2024-04-10T15:11:06Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 5e021ba

Failed CI Steps

FTR Configs #94

Test Failures

[job] [logs] FTR Configs #94 / dashboard app - group 2 dashboard filtering adding a filter that excludes all data "before all" hook for "filters on pie charts"

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`cloudSecurityPosture`	416	432	+16
`discover`	778	788	+10
`lens`	1385	1401	+16
`logsExplorer`	710	726	+16
`securitySolution`	5301	5316	+15
`slo`	673	689	+16
total			+89

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/unified-data-table`	78	83	+5

Async chunks

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

id	before	after	diff
`cloudSecurityPosture`	402.1KB	449.7KB	+47.6KB
`discover`	592.3KB	636.7KB	+44.3KB
`securitySolution`	17.0MB	17.1MB	+60.7KB
`slo`	611.0KB	660.9KB	+49.9KB
total			+202.6KB

Page load bundle

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

id	before	after	diff
`cloudSecurityPosture`	15.9KB	16.0KB	+69.0B

Unknown metric groups

API count

id	before	after	diff
`@kbn/unified-data-table`	144	151	+7

async chunk count

id	before	after	diff
`cloudSecurityPosture`	5	6	+1
`discover`	21	22	+1
`securitySolution`	66	67	+1
`slo`	25	26	+1
total			+4

ESLint disabled line counts

id	before	after	diff
`@kbn/unified-data-table`	8	10	+2

Total ESLint disabled count

id	before	after	diff
`@kbn/unified-data-table`	8	10	+2

History

💚 Build #201995 succeeded 8c5bcaa
💚 Build #201982 succeeded 76cb00f
💔 Build #201647 failed 740856b
💔 Build #201645 failed bc24f98
💔 Build #201607 failed 243a8ca
💔 Build #201606 failed 9e63710

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

cc @andreadelrio @davismcphee

davismcphee · 2024-04-10T16:15:32Z

Thanks all for helping bring this over the finish line! It seems like we're good with the current tooltip text, so I'm going to merge this now 🙂

davismcphee added Feature:Discover Discover Application release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover, search (data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. t// labels Sep 18, 2023

davismcphee self-assigned this Sep 18, 2023

davismcphee force-pushed the discover-doc-compare branch from 3bbbc44 to 5fad331 Compare November 27, 2023 01:11

kertal requested a review from andreadelrio December 5, 2023 10:55

andreadelrio mentioned this pull request Feb 9, 2024

[EuiIcon] Add diff glyph elastic/eui#7520

Merged

4 tasks

davismcphee force-pushed the discover-doc-compare branch 2 times, most recently from 175d1f6 to 6304bab Compare March 25, 2024 00:53

amyjtechwriter self-requested a review March 26, 2024 14:03

davismcphee mentioned this pull request Mar 27, 2024

[EuiDataGrid] Allow sticky columns elastic/eui#2839

Open

davismcphee added 16 commits March 31, 2024 01:12

Add initial implementation of document comparison

ee654f1

Fixing some bugs and display issues

55bc9a4

Add support for pinning and removing docs

c4a5e7a

Add advanced diffing

6378993

Add support for selecting diff modes

9ae16ec

Add support for character and word diffs

3108dd2

Improve line diff indicators

2f58b14

Use local storage for diff options

91cb6c7

Fine tuning diff display

f591305

Separate CompareDocuments into a separate component

649269e

Cleanup

e1cf7eb

More cleanup

05f319b

Add i18n strings

dd49b22

Create useComparisonColumns

76214ee

Create useComparisonFields hook

339df3c

Add ComparisonControls component

82c517e

davismcphee added 2 commits April 6, 2024 02:34

Update i18n values

243a8ca

Update functional test table headers

bc24f98

davismcphee commented Apr 7, 2024

View reviewed changes

Merge branch 'main' into discover-doc-compare

740856b

amyjtechwriter approved these changes Apr 8, 2024

View reviewed changes

jughosta reviewed Apr 8, 2024

View reviewed changes

andreadelrio requested changes Apr 8, 2024

View reviewed changes

davismcphee added 5 commits April 8, 2024 19:45

Merge branch 'main' into discover-doc-compare

76cb00f

Remove 'Document' prefix from column headers

f8e9a71

Remove custom badge styles

da20ce9

Disable diff mode buttons and show diff decorations switch when show …

4050791

…diff is off

Fix issue where docs are removed from comparison when the main table …

8c5bcaa

…loses access to them

davismcphee commented Apr 9, 2024

View reviewed changes

jughosta approved these changes Apr 9, 2024

View reviewed changes

davismcphee mentioned this pull request Apr 10, 2024

[Discover] Split functional tests in more groups #180046

Merged

andreadelrio self-requested a review April 10, 2024 02:02

andreadelrio approved these changes Apr 10, 2024

View reviewed changes

kertal approved these changes Apr 10, 2024

View reviewed changes

Merge branch 'main' into discover-doc-compare

5e021ba

davismcphee merged commit 339379e into elastic:main Apr 10, 2024

davismcphee deleted the discover-doc-compare branch April 10, 2024 19:49

kibanamachine added v8.14.0 backport:skip This PR does not require backporting labels Apr 10, 2024

kertal mentioned this pull request Apr 12, 2024

[Discover] Copy rows as text to clipboard UX / UI enhancements #179731

Closed

davismcphee mentioned this pull request Apr 24, 2024

[Discover] [Logs Explorer] Document comparison mode does not support smart fields #181621

Closed

kertal mentioned this pull request Jun 18, 2024

[Discover] Allow to select/deselect all rows in the grid at once #184241

Merged

9 tasks

	/**
	* Returning a generated id of a given ES document, since `_id` can be the same
	* when using different indices and shard routing
	*/
	export const getDocId = (doc: EsHitRecord & { _routing?: string }) => {
	const routing = doc._routing ? doc._routing : '';
	return [doc._index, doc._id, routing].join('::');
	};

Conversation

davismcphee commented Sep 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

andreadelrio commented Feb 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davismcphee commented Mar 25, 2024

Uh oh!

davismcphee commented Mar 26, 2024

Uh oh!

davismcphee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amyjtechwriter left a comment

Choose a reason for hiding this comment

Uh oh!

jughosta left a comment

Choose a reason for hiding this comment

Uh oh!

jughosta Apr 8, 2024

Choose a reason for hiding this comment

Uh oh!

davismcphee Apr 9, 2024

Choose a reason for hiding this comment

Uh oh!

andreadelrio commented Apr 8, 2024

Uh oh!

andreadelrio left a comment

Choose a reason for hiding this comment

Uh oh!

davismcphee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

davismcphee Apr 9, 2024

Choose a reason for hiding this comment

Uh oh!

jughosta left a comment

Choose a reason for hiding this comment

Uh oh!

andreadelrio left a comment

Choose a reason for hiding this comment

Uh oh!

amyjtechwriter commented Apr 10, 2024

Uh oh!

kertal left a comment

Choose a reason for hiding this comment

Uh oh!

kibana-ci commented Apr 10, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

async chunk count

ESLint disabled line counts

Total ESLint disabled count

History

Uh oh!

davismcphee commented Apr 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

davismcphee commented Sep 18, 2023 •

edited

Loading

andreadelrio commented Feb 7, 2024 •

edited

Loading