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.

Sign up for GitHub

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

Jump to bottom

[7.x] [Discover] Add EUIDataGrid to surrounding documents (#99447) #101063

Merged
merged 5 commits into from
Jun 4, 2021
Merged

[7.x] [Discover] Add EUIDataGrid to surrounding documents (#99447) #101063

merged 5 commits into from
Jun 4, 2021

Conversation

dimaanj
Copy link
Contributor

@dimaanj dimaanj commented Jun 1, 2021

Backports the following commits to 7.x:

@dimaanj @kibanamachine
[Discover] Add EUIDataGrid to surrounding documents (elastic#99447)
c6e4ddb 
* [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>
@dimaanj dimaanj enabled auto-merge (squash) June 1, 2021 15:41
@timroes
Copy link
Contributor

timroes commented Jun 1, 2021

@elasticmachine run elasticsearch-ci/docs

@dimaanj
Copy link
Contributor Author

dimaanj commented Jun 2, 2021

@elasticmachine merge upstream

@dimaanj
Copy link
Contributor Author

dimaanj commented Jun 2, 2021

@elasticmachine merge upstream

@kertal
Copy link
Member

kertal commented Jun 2, 2021

Well, this is odd, it should work. you could try the following what fails is:

kibana/test/functional/services/data_grid.ts

Line 209 in 69883de

await this.find.clickByButtonText(sortText);

It's failing because for some reason the header action popover isn't displayed

kibana/test/functional/services/data_grid.ts

Line 207 in 69883de

await this.find.clickByCssSelector('.euiDataGridHeaderCell__button');

So opening this menu could be ensured by implementing a retry similar like this:

kibana/test/functional/services/data_grid.ts

Lines 195 to 200 in 69883de

public async openColMenuByField(field: string) {
await this.retry.waitFor('header cell action being displayed', async () => {
// to prevent flakiness
await this.testSubjects.click(`dataGridHeaderCell-${field}`);
return await this.testSubjects.exists(`dataGridHeaderCellActionGroup-${field}`);
});

If this would help here in 7.x, it should also be implemented in master

@dimaanj dimaanj disabled auto-merge June 3, 2021 06:57
@dimaanj
Copy link
Contributor Author

dimaanj commented Jun 3, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

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 410.9KB 420.0KB +9.1KB
Unknown metric groups

References to deprecated APIs

id before after diff
discover 31 27 -4

History

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

dimaanj
dimaanj commented Jun 3, 2021
View reviewed changes
src/plugins/discover/public/application/components/discover_grid/discover_grid.scss
@@ -47,6 +47,8 @@

// We only truncate if the cell is not a control column.
.euiDataGridHeader {
display: flex; // fixing row header in firefox
Copy link
Contributor Author

@dimaanj dimaanj Jun 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

First three builds of this backport failed due to Firefox smoke test failure which not reproduced locally. For some reason header action popover had not opened.

One of the suggestion is header row position, picture below shows gap (only in firefox). So, after applying display: flex to a data grid, in this way overwritingdisplay: inline-flex, everything seems ok.

8C56E89A-B8C4-41F3-A80F-EC6B0CBFB32C_4_5005_c

Copy link
Member

Choose a reason for hiding this comment

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

Great debugging! Could you also apply this change in a seperate PR to master? thx!

@dimaanj dimaanj merged commit b09aeb5 into elastic:7.x Jun 4, 2021
@dimaanj dimaanj mentioned this pull request Jun 4, 2021
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kertal kertal kertal left review comments

Assignees
No one assigned
Labels
backport
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dimaanj @timroes @kertal @kibanamachine