[Discover] Fix doc viewer table columns by dimaanj · Pull Request #113124 · elastic/kibana

dimaanj · 2021-09-27T14:15:56Z

Summary

Fixes #112966

Checklist

This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

Risk Matrix

Delete this section if it is not applicable to this PR.

Before

After

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2021-09-27T14:15:59Z

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

kertal · 2021-09-28T05:59:53Z

ACK, will review

kertal · 2021-09-29T08:48:25Z

@elasticmachine merge upstream

kertal

this would fix the issue 👍 . however there's one thing I'd like to discuss. So when you have just short filenames and a very broad screen, there's lots of whitespace with this approach:

the alternative solution would be to assign a min width to the fieldname column like this:

.kbnDocViewer__tableRow td:nth-child(2) {
    min-width: 108px;
 }

Then it would look like this:

More space for the value which I think is more imortant in this case

WDYT dear @elastic/kibana-design

Here's some testdata

POST testdata/_doc/1
{
  "veryvery.very.very.very.veryvery.veryveryvery.very.veryvery.veryvery.very.veryvery.very.very.very.very.very.veryveryvery.long.key": "shortvalue"
}

POST testdata/_doc/2
{
  "shortkey": "veryvery.very.very.very.veryvery.veryveryvery.very.veryvery.veryvery.very.veryvery.very.very.very.very.very.veryveryvery.long.keyveryvery.very.very.very.veryvery.veryveryvery.very.veryvery.veryvery.very.veryvery.very.very.very.very.very.veryveryvery.long.keyveryvery.very.very.very.veryvery.veryveryvery.very.veryvery.veryvery.very.veryvery.very.very.very.very.very.veryveryvery.long.keyveryvery.very.very.very.veryvery.veryveryvery.very.veryvery.veryvery.very.veryvery.very.very.very.very.very.veryveryvery.long.keyveryvery.very.very.very.veryvery.veryveryvery.very.veryvery.veryvery.very.veryvery.very.very.very.very.very.veryveryvery.long.keyveryvery.very.very.very.veryvery.veryveryvery.very.veryvery.veryvery.very.veryvery.very.very.very.very.very.veryveryvery.long.keyveryvery.very.very.very.veryvery.veryveryvery.very.veryvery.veryvery.very.veryvery.very.very.very.very.very.veryveryvery.long.key"
}

POST testdata/_doc/3
{
  "veryvery.very.very.very.veryvery.veryveryvery.very.veryvery.veryvery.very.veryvery.very.very.very.very.very.veryveryvery.long.key": "veryvery.very.very.very.veryvery.veryveryvery.very.veryvery.veryvery.very.veryvery.very.very.very.very.very.veryveryvery.long.keyveryvery.very.very.very.veryvery.veryveryvery.very.veryvery.veryvery.very.veryvery.very.very.very.very.very.veryveryvery.long.keyveryvery.very.very.very.veryvery.veryveryvery.very.veryvery.veryvery.very.veryvery.very.very.very.very.very.veryveryvery.long.keyveryvery.very.very.very.veryvery.veryveryvery.very.veryvery.veryvery.very.veryvery.very.very.very.very.very.veryveryvery.long.keyveryvery.very.very.very.veryvery.veryveryvery.very.veryvery.veryvery.very.veryvery.very.very.very.very.very.veryveryvery.long.keyveryvery.very.very.very.veryvery.veryveryvery.very.veryvery.veryvery.very.veryvery.very.very.very.very.very.veryveryvery.long.keyveryvery.very.very.very.veryvery.veryveryvery.very.veryvery.veryvery.very.veryvery.very.very.very.very.very.veryveryvery.long.key"
}

majagrubic · 2021-09-29T09:35:12Z

I like the solution of having min-width in there.

dimaanj · 2021-09-29T21:16:51Z

@elasticmachine merge upstream

andreadelrio · 2021-09-29T23:33:05Z

this would fix the issue 👍 . however there's one thing I'd like to discuss. So when you have just short filenames and a very broad screen, there's lots of whitespace with this approach:

the alternative solution would be to assign a min width to the fieldname column like this:
.kbnDocViewer__tableRow td:nth-child(2) {
    min-width: 108px;
 }
Then it would look like this: More space for the value which I think is more imortant in this case

WDYT dear @elastic/kibana-design

I also like the approach you're proposing here. Let's just remember to use Eui variables to get the pixel value $euiSizeM * 9 should get you there.

@dmitriynj if you could add some quick before/after screenshots to these PRs it would be super useful when reviewing.

kertal

I think the first column should have a fixed width, else it could look like this:

should work correctly when the change in table_columns.tsx was reverted. CSS change is fine 👍

dimaanj · 2021-09-30T15:59:14Z

@elasticmachine merge upstream

kibanamachine · 2021-09-30T18:00:19Z

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id	before	after	diff
`discover`	378.7KB	379.0KB	+288.0B

History

💔 Build #156774 failed 405e582
💚 Build #156791 succeeded 946db57
💚 Build #156653 succeeded fdf65c4
💛 Build #156596 was flaky 6fd913a
💔 Build #156548 failed aaa426b

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

cc @dmitriynj

kertal · 2021-10-01T07:51:30Z

src/plugins/discover/public/application/components/doc_viewer/doc_viewer.scss

  font-family: $euiCodeFontFamily;

+  // set min-width for each column except actions
+  .euiTableRowCell:nth-child(n+2) {


TIL you can do something like n+2 nice!

kertal

LGTM, tested a-la-carte with Firefox, Chrome, Safari. Thx for teaching me advanced CSS 🙏

andreadelrio

LGTM

* [Discover] fix doc viewer table columns * [Discover] apply suggestions * [Discover] apply suggestion * [Discover] fix action column * [Discover] do not apply min-width to actions column Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

byronhulcher · 2021-10-01T15:51:46Z

Is it possible this broke master? I'm seeing errors from CI for Quick commit checks after pulling master in with this code.

ERROR 
      src/plugins/discover/public/application/components/doc_viewer/doc_viewer.scss
       15:3  ✖  Cannot parse selector   parseError
       15:3  ✖  Cannot parse selector   parseError
       15:3  ✖  Cannot parse selector   parseError
       15:3  ✖  Cannot parse selector   parseError
       21:5  ✖  Cannot parse selector   parseError
       21:5  ✖  Cannot parse selector   parseError
       21:5  ✖  Cannot parse selector   parseError
       23:7  ✖  Cannot parse selector   parseError

See https://github.com/elastic/kibana/pull/113550/checks?check_run_id=3769108058

byronhulcher · 2021-10-01T16:04:48Z

Sorry, your file appearing may have been a false flag, currently suspecting #113443

* [Discover] fix doc viewer table columns * [Discover] apply suggestions * [Discover] apply suggestion * [Discover] fix action column * [Discover] do not apply min-width to actions column Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

[Discover] fix doc viewer table columns

15a63d0

dimaanj added Feature:Discover Discover Application release_note:fix v8.0.0 v7.16.0 v7.15.1 Team:DataDiscovery Discover, search (data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. t// labels Sep 27, 2021

dimaanj requested a review from a team September 27, 2021 14:15

dimaanj self-assigned this Sep 27, 2021

Merge branch 'master' into fix-doc-viewer-table-columns

c1c7c01

kertal reviewed Sep 29, 2021

View reviewed changes

[Discover] apply suggestions

aaa426b

dimaanj requested a review from a team as a code owner September 29, 2021 18:09

Merge branch 'master' into fix-doc-viewer-table-columns

6fd913a

[Discover] apply suggestion

fdf65c4

kertal reviewed Sep 30, 2021

View reviewed changes

dimaanj added 2 commits September 30, 2021 17:30

[Discover] fix action column

405e582

[Discover] do not apply min-width to actions column

946db57

dimaanj requested a review from kertal September 30, 2021 14:48

Merge branch 'master' into fix-doc-viewer-table-columns

2365a4b

kertal reviewed Oct 1, 2021

View reviewed changes

kertal approved these changes Oct 1, 2021

View reviewed changes

andreadelrio approved these changes Oct 1, 2021

View reviewed changes

dimaanj merged commit 88fa27d into elastic:master Oct 1, 2021

dimaanj mentioned this pull request Oct 1, 2021

[7.x] [Discover] Fix doc viewer table columns (#113124) #113637

Merged

dimaanj mentioned this pull request Oct 1, 2021

[7.15] [Discover] Fix doc viewer table columns (#113124) #113638

Merged

kertal mentioned this pull request Oct 15, 2021

DocViewer shrinks field column when large content in value column #113975

Closed

Conversation

dimaanj commented Sep 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Risk Matrix

For maintainers

Uh oh!

elasticmachine commented Sep 27, 2021

Uh oh!

kertal commented Sep 28, 2021

Uh oh!

kertal commented Sep 29, 2021

Uh oh!

kertal left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

majagrubic commented Sep 29, 2021

Uh oh!

dimaanj commented Sep 29, 2021

Uh oh!

andreadelrio commented Sep 29, 2021

Uh oh!

kertal left a comment

Choose a reason for hiding this comment

Uh oh!

dimaanj commented Sep 30, 2021

Uh oh!

kibanamachine commented Sep 30, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

Uh oh!

kertal Oct 1, 2021

Choose a reason for hiding this comment

Uh oh!

kertal 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!

byronhulcher commented Oct 1, 2021

Uh oh!

byronhulcher commented Oct 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dimaanj commented Sep 27, 2021 •

edited

Loading

kertal left a comment •

edited

Loading