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.

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

[Discover] Add EUIDataGrid to surrounding documents #99447

Merged
merged 34 commits into from
Jun 1, 2021

Conversation

dimaanj
Copy link
Contributor

@dimaanj dimaanj commented May 6, 2021

Closes #97126

Checklist

Delete any items that are not applicable to this PR.

C69634EB-15F4-4D25-8172-ADDC6DF965D5_1_105_c

dimaanj and others added 8 commits May 3, 2021 10:55
…ts-migration

# Conflicts:
#	src/plugins/discover/public/application/angular/context/query_parameters/actions.test.ts
#	src/plugins/discover/public/application/components/context_error_message/context_error_message.test.tsx
#	src/plugins/discover/public/application/components/context_error_message/context_error_message.tsx
@kertal kertal added the Feature:Discover Discover Application label May 6, 2021
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Great that it already works, first look, note that you need to disable sorting, since there is just one type of sorting in Context, that the user shouldn't be able to change

@dimaanj
Copy link
Contributor Author

dimaanj commented May 7, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

merge conflict between base and head

columns,
rows: allRowsLoaded && rows,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check needs to prevent first loaded anchor document to be colored. Otherwise see the folowing behaviour. Checkbox from the first row is highlighted

9413657A-D110-4E74-B998-4F229FC23510_1_105_c

@dimaanj dimaanj self-assigned this May 7, 2021
@kertal
Copy link
Member

kertal commented May 10, 2021

@elasticmachine merge upstream

@andreadelrio
Copy link
Contributor

Thanks for taking this on @dmitriynj
Now that we're in that code we might as well take the opportunity to implement some design improvements. Hopefully this is nothing too big or else we can do a follow up PR (cc @kertal). Here's what I'm thinking:

a) Add a header
image

b) Add a button that lets the user go straight to the page where the document is in. Think of it like when you're in a calendar app and you quickly want to go back to 'Today'. Something like this:

Screen Recording 2021-05-11 at 5 19 26 PM

@kertal
Copy link
Member

kertal commented May 12, 2021

@elasticmachine merge upstream

@kertal

This comment has been minimized.

@kertal
Copy link
Member

kertal commented May 29, 2021

@elasticmachine merge upstream

@dimaanj
Copy link
Contributor Author

dimaanj commented May 31, 2021

@elasticmachine merge upstream

@kertal
Copy link
Member

kertal commented Jun 1, 2021

@elasticmachine merge upstream

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Code LGTM, tested locally in Safari, Chrome, Firefox, MacOS, works as expected, great work! 👍

@kertal
Copy link
Member

kertal commented Jun 1, 2021

ah, one last thing , I think the title of this PR could be

[Discover] Add EUIDataGrid to surrounding documents

since it doesn't change the default (currently), but adds it as additional doc table

@dimaanj dimaanj changed the title [Discover] Change surrounding documents to use EUIDataGrid [Discover] Add EUIDataGrid to surrounding documents Jun 1, 2021
@kibanamachine
Copy link
Contributor

💛 Build succeeded, but was flaky


Test Failures

Kibana Pipeline / general / X-Pack API Integration Tests.x-pack/test/api_integration/apis/management/index_management/indices·js.apis management index management indices list should list all the indices with the expected properties and data enrichers

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has failed 16 times on tracked branches: https://github.com/elastic/kibana/issues/64473

[00:00:00]       │
[00:00:00]         └-: apis
[00:00:00]           └-> "before all" hook in "apis"
[00:05:19]           └-: management
[00:05:19]             └-> "before all" hook in "management"
[00:05:36]             └-: index management
[00:05:36]               └-> "before all" hook in "index management"
[00:05:36]               └-: indices
[00:05:36]                 └-> "before all" hook in "indices"
[00:05:38]                 └-: list
[00:05:38]                   └-> "before all" hook for "should list all the indices with the expected properties and data enrichers"
[00:05:38]                   └-> should list all the indices with the expected properties and data enrichers
[00:05:38]                     └-> "before each" hook: global before each for "should list all the indices with the expected properties and data enrichers"
[00:05:38]                     └- ✖ fail: apis management index management indices list should list all the indices with the expected properties and data enrichers
[00:05:38]                     │       Error: expected [ 'aliases',
[00:05:38]                     │   'data_stream',
[00:05:38]                     │   'documents',
[00:05:38]                     │   'health',
[00:05:38]                     │   'hidden',
[00:05:38]                     │   'ilm',
[00:05:38]                     │   'isFollowerIndex',
[00:05:38]                     │   'isFrozen',
[00:05:38]                     │   'isRollupIndex',
[00:05:38]                     │   'name',
[00:05:38]                     │   'primary',
[00:05:38]                     │   'replica',
[00:05:38]                     │   'size',
[00:05:38]                     │   'status',
[00:05:38]                     │   'uuid' ] to sort of equal [ 'aliases',
[00:05:38]                     │   'documents',
[00:05:38]                     │   'health',
[00:05:38]                     │   'hidden',
[00:05:38]                     │   'ilm',
[00:05:38]                     │   'isFollowerIndex',
[00:05:38]                     │   'isFrozen',
[00:05:38]                     │   'isRollupIndex',
[00:05:38]                     │   'name',
[00:05:38]                     │   'primary',
[00:05:38]                     │   'replica',
[00:05:38]                     │   'size',
[00:05:38]                     │   'status',
[00:05:38]                     │   'uuid' ]
[00:05:38]                     │       + expected - actual
[00:05:38]                     │ 
[00:05:38]                     │        [
[00:05:38]                     │          "aliases"
[00:05:38]                     │       -  "data_stream"
[00:05:38]                     │          "documents"
[00:05:38]                     │          "health"
[00:05:38]                     │          "hidden"
[00:05:38]                     │          "ilm"
[00:05:38]                     │       
[00:05:38]                     │       at Assertion.assert (/dev/shm/workspace/parallel/4/kibana/node_modules/@kbn/expect/expect.js:100:11)
[00:05:38]                     │       at Assertion.eql (/dev/shm/workspace/parallel/4/kibana/node_modules/@kbn/expect/expect.js:244:8)
[00:05:38]                     │       at Context.<anonymous> (test/api_integration/apis/management/index_management/indices.js:206:39)
[00:05:38]                     │       at Object.apply (/dev/shm/workspace/parallel/4/kibana/node_modules/@kbn/test/src/functional_test_runner/lib/mocha/wrap_function.js:73:16)
[00:05:38]                     │ 
[00:05:38]                     │ 

Stack Trace

Error: expected [ 'aliases',
  'data_stream',
  'documents',
  'health',
  'hidden',
  'ilm',
  'isFollowerIndex',
  'isFrozen',
  'isRollupIndex',
  'name',
  'primary',
  'replica',
  'size',
  'status',
  'uuid' ] to sort of equal [ 'aliases',
  'documents',
  'health',
  'hidden',
  'ilm',
  'isFollowerIndex',
  'isFrozen',
  'isRollupIndex',
  'name',
  'primary',
  'replica',
  'size',
  'status',
  'uuid' ]
    at Assertion.assert (/dev/shm/workspace/parallel/4/kibana/node_modules/@kbn/expect/expect.js:100:11)
    at Assertion.eql (/dev/shm/workspace/parallel/4/kibana/node_modules/@kbn/expect/expect.js:244:8)
    at Context.<anonymous> (test/api_integration/apis/management/index_management/indices.js:206:39)
    at Object.apply (/dev/shm/workspace/parallel/4/kibana/node_modules/@kbn/test/src/functional_test_runner/lib/mocha/wrap_function.js:73:16) {
  actual: '[\n' +
    '  "aliases"\n' +
    '  "data_stream"\n' +
    '  "documents"\n' +
    '  "health"\n' +
    '  "hidden"\n' +
    '  "ilm"\n' +
    '  "isFollowerIndex"\n' +
    '  "isFrozen"\n' +
    '  "isRollupIndex"\n' +
    '  "name"\n' +
    '  "primary"\n' +
    '  "replica"\n' +
    '  "size"\n' +
    '  "status"\n' +
    '  "uuid"\n' +
    ']',
  expected: '[\n' +
    '  "aliases"\n' +
    '  "documents"\n' +
    '  "health"\n' +
    '  "hidden"\n' +
    '  "ilm"\n' +
    '  "isFollowerIndex"\n' +
    '  "isFrozen"\n' +
    '  "isRollupIndex"\n' +
    '  "name"\n' +
    '  "primary"\n' +
    '  "replica"\n' +
    '  "size"\n' +
    '  "status"\n' +
    '  "uuid"\n' +
    ']',
  showDiff: true
}

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
discover 359 369 +10

Async chunks

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

id before after diff
discover 411.1KB 420.0KB +8.9KB
Unknown metric groups

References to deprecated APIs

id before after diff
canvas 29 25 -4
crossClusterReplication 8 6 -2
discover 31 27 -4
fleet 22 20 -2
globalSearch 4 2 -2
indexManagement 12 7 -5
infra 261 149 -112
lens 67 45 -22
licensing 18 15 -3
lists 239 236 -3
maps 286 208 -78
ml 121 115 -6
monitoring 109 56 -53
securitySolution 390 346 -44
stackAlerts 101 95 -6
total -346

History

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

cc @dmitriynj

@dimaanj dimaanj merged commit 69883de into elastic:master Jun 1, 2021
@kertal kertal added this to In progress in Discover via automation Jun 1, 2021
dimaanj added a commit to dimaanj/kibana that referenced this pull request Jun 1, 2021
* [Discover] migrate remaining context files from js to ts

* [Discover] get rid of any types

* [Discover] replace constants with enums, update imports

* [Discover] use unknown instead of any, correct types

* [Discover] skip any type for tests

* [Discover] add euiDataGrid view

* [Discover] add support dataGrid columns, provide ability to do not change sorting, highlight anchor doc, rename legacy variables

* [Discover] update context_legacy test and types

* [Discover] update unit tests, add context header

* [Discover] update unit and functional tests

* [Discover] remove docTable from context test which uses new data grid

* [Discover] update EsHitRecord type, use it for context app. add no pagination support

* [Discover] resolve type error in test

* [Discover] add disabling control columns option, change loading feedback

* [Discover] clean up, update functional tests

* [Discover] remove invalid translations

* [Discover] support both no results found and loading feedback

* [Discover] provide loading status for discover

* [Discover] fix functional test

* [Discover] add useDataGridColumns test, update by comments

* [Discover] fix types

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jun 3, 2021
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

dimaanj added a commit that referenced this pull request Jun 4, 2021
…101063)

* [Discover] Add EUIDataGrid to surrounding documents (#99447)

* [Discover] migrate remaining context files from js to ts

* [Discover] get rid of any types

* [Discover] replace constants with enums, update imports

* [Discover] use unknown instead of any, correct types

* [Discover] skip any type for tests

* [Discover] add euiDataGrid view

* [Discover] add support dataGrid columns, provide ability to do not change sorting, highlight anchor doc, rename legacy variables

* [Discover] update context_legacy test and types

* [Discover] update unit tests, add context header

* [Discover] update unit and functional tests

* [Discover] remove docTable from context test which uses new data grid

* [Discover] update EsHitRecord type, use it for context app. add no pagination support

* [Discover] resolve type error in test

* [Discover] add disabling control columns option, change loading feedback

* [Discover] clean up, update functional tests

* [Discover] remove invalid translations

* [Discover] support both no results found and loading feedback

* [Discover] provide loading status for discover

* [Discover] fix functional test

* [Discover] add useDataGridColumns test, update by comments

* [Discover] fix types

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* [Discover] fix data grid row header in firefox

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jun 4, 2021
@timroes timroes moved this from In progress to Done in Discover Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Discover Discover Application release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.14.0 v8.0.0
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

[Discover] Change surrounding documents to use EUIDataGrid
8 participants