[Discover] Update data table grid styles #164187

jughosta · 2023-08-17T14:49:46Z

Resolves [Discover] Update data table styles #164287

Summary

This PRs:

~~swaps checkbox and row selection~~
removes vertical borders
adds rows highlight
increases cell padding
adds row stripes
updates header background
removes grey background from field name and makes it bolder (part of [UnifiedDataTable] Redesign Document column #164634)

Before:

After:

elasticmachine · 2023-08-17T19:48:16Z

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

MichaelMarcialis

Thanks for putting this together, @jughosta! Looks great. Sending some initial questions/comments for your initial review below. Also CCing @andreadelrio, in case she has additional input.

MichaelMarcialis · 2023-08-22T21:15:44Z

src/plugins/discover/public/components/discover_grid/constants.ts

  fontSize: 's',
-  cellPadding: 's',
-  rowHover: 'none',
+  cellPadding: 'm',


If the intent here is to begin adopting some aspects of the newly proposed Discover design refresh, this padding will need to be increased to match the designs.

Suggested change

cellPadding: 'm',

cellPadding: 'l',

MichaelMarcialis · 2023-08-22T21:20:32Z

src/plugins/discover/public/components/discover_grid/constants.ts

@@ -11,10 +11,10 @@ import { EuiDataGridStyle } from '@elastic/eui';
 // data types
 export const kibanaJSON = 'kibana-json';
 export const GRID_STYLE = {
-  border: 'all',
+  border: 'horizontal',


We may want to hold off on removal of vertical borders until we add the proposed column highlighting, as it helps to indicate to users where the column boundaries are located when vertical borders are absent. Thoughts?

I don't think we should hold off on this change until column highlighting is available. Padding included in the cells should produce enough space between columns for users to tell when a column begins and ends. See:

Also, note that the column highlighting is not currently considered part of the MVP we're aiming for.

Agreed we don't need to wait, IMO it's already looking nice 👍

MichaelMarcialis · 2023-08-22T21:25:53Z

src/plugins/discover/public/components/discover_grid/constants.ts

-  cellPadding: 's',
-  rowHover: 'none',
+  cellPadding: 'm',
+  rowHover: 'highlight',


Similarly, we may want to wait to enable row highlighting until we alter the contents of the "Document" column to no longer use the EUI description list component styling. I say this because field names within the "Document" column to appear in a gray badge that is very difficult to see when combined with the new highlight (but will be addressed when we switch to bolding the field names and prepending them with field type tokens). Thoughts?

I agree. @kertal do we have an issue tracking that? I couldn't find one and #164287 doesn't cover it, maybe we should update it to include:

Remove grey background from EuiDescriptionList's title in table and make it bolder.

working on an issue for that

@MichaelMarcialis here's the issue #164634

MichaelMarcialis · 2023-08-22T21:35:09Z

src/plugins/discover/public/components/discover_grid/discover_grid.scss

+  .euiDataGrid__controls {
+    border: none;
+    border-bottom: $euiBorderThin;
+  }


Assuming we do proceed with using the border="horizontal" prop for the data grid now, I believe you can actually get rid of these styles entirely (as the data grid is already styled this way with that prop configuration).

Suggested change

.euiDataGrid__controls {

border: none;

border-bottom: $euiBorderThin;

}

Removed via de53757

MichaelMarcialis · 2023-08-22T21:49:00Z

src/plugins/discover/public/components/discover_grid/discover_grid.scss

+  .dscDiscoverGrid__table .euiDataGridRowCell:first-of-type {
+    border-left: none;
+    border-right: none;
+  }


Out of curiosity, is this styling necessary? If it's to prevent the checkbox from being cut-off by 2px, would it be better to just change the width of the column to account for the additional 2px of transparent border, rather than overriding EUI styles? Or are the transparent borders an EUI bug that needs to be addressed?

Thanks for the idea, @MichaelMarcialis!
Updated via de53757

# Conflicts: # packages/kbn-unified-data-table/src/components/data_table.scss # packages/kbn-unified-data-table/src/constants.ts

kertal

Looking pretty 👍 and works as expected.
2 things

@andreadelrio suggested for now to keep the order of checkbox and expand icon last week
I think we need a line on top of the grid when being displayed on a dashboard?

jughosta · 2023-09-05T13:48:34Z

@kertal

@andreadelrio suggested for now to keep the order of checkbox and expand icon last week

Swapped the buttons back:

jughosta · 2023-09-05T14:24:54Z

@kertal

I think we need a line on top of the grid when being displayed on a dashboard?

Added top border and also adjusted side paddings on Surrounding Documents page.

andreadelrio · 2023-09-06T03:18:17Z

@jughosta Looking very nice! As part of this PR, I would also like to see the background of rows for the tabs for Documents | Field Statistics removed (given the agreement in today's team sync to keep those tabs for now).

andreadelrio · 2023-09-06T04:10:55Z

@jughosta Looking very nice! As part of this PR, I would also like to see the background of rows for the tabs for Documents | Field Statistics removed (given the agreement in today's team sync to keep those tabs for now).

@jughosta you can disregard the above as I've seen you already have it as part of #165687

kibana-ci · 2023-09-06T08:40:38Z

💔 Build Failed

Failed CI Steps

Test Failures

[job] [logs] Serverless Security Cypress Tests #8 / Add endpoint exception from rule details edits an endpoint exception item edits an endpoint exception item
[job] [logs] Serverless Security Explore Cypress Tests #1 / Enable risk scores "before all" hook for "shows enable host risk button" "before all" hook for "shows enable host risk button"
[job] [logs] Serverless Observability Examples Tests / serverless examples UI Unified Field List Examples Field stats field distribution "before all" hook for "should return an auto histogram for numbers and top values"
[job] [logs] Serverless Observability Tests / serverless observability UI Configure Closure options change closure option successfully

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`	550.3KB	550.8KB	+451.0B

History

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

cc @jughosta

kertal

Code + Rendering LGTM. Pretty it is already, and more prettier it will be soon 🪞 . Tested locally and worked as expected. ✅

jughosta · 2023-09-06T15:26:26Z

Closing in favour of a single PR with all redesign changes #165866

## Summary ### Part 1 - Resolves #164287 - Closes #146339 - Previously separate PR #164187 Changes: - ~~swaps checkbox and row selection~~ - removes vertical borders - adds rows highlight - increases cell padding - adds row stripes - updates header background - removes grey background from field name and makes it bolder (part of #164634) - updates Surrounding Documents side paddings ### Part 2 - Resolves #164661 - Previously separate PR #165687 Changes: - removes background from panels, tabs and sidebar - updates "Add a field" button style - removes shadow from field list items - makes field search compact ### Part 3 - Resolves #164662 Changes: - wraps "Add a field" button in its own container with a top border - ~~adds a drag handle to sidebar items~~ - ~~adds new Show/Hide buttons to toggle sidebar~~ moves sidebar toggle button from discover plugin to unified field list - reduces spaces between sidebar items from 4px to 2px - reduces padding on Single Document page - removes border above grid tabs <img width="600" alt="Screenshot 2023-09-07 at 14 39 48" src="https://github.com/elastic/kibana/assets/1415710/976db247-fd70-4c9b-8634-552ece45b522"> Please note that "auto" row height is in a separate PR which is also ready for review #164218 --------- Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Davis McPhee <[email protected]> Co-authored-by: Davis McPhee <[email protected]>

[Discover] Update grid styles

6be6936

jughosta added release_note:enhancement Feature:Discover Discover Application backport:skip This commit does not require backporting Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. labels Aug 17, 2023

jughosta self-assigned this Aug 17, 2023

jughosta added 5 commits August 17, 2023 18:03

[Discover] Update tests

7c27938

[Discover] Update tests

72bc5b2

[Discover] Add a comment

0ac8342

[Discover] Update tests

69ed036

[Discover] Update tests

680641b

jughosta marked this pull request as ready for review August 17, 2023 19:48

jughosta requested review from a team as code owners August 17, 2023 19:48

kertal changed the title ~~[Discover] Update grid styles~~ [Discover] Update data table grid styles Aug 21, 2023

MichaelMarcialis reviewed Aug 22, 2023

View reviewed changes

jughosta marked this pull request as draft September 4, 2023 13:20

jughosta added 6 commits September 4, 2023 16:31

[Discover] Address review comments

de53757

[Discover] Enable row stripes

aca35f3

Merge remote-tracking branch 'upstream/main' into redesign-checkboxes

2c65d30

# Conflicts: # packages/kbn-unified-data-table/src/components/data_table.scss # packages/kbn-unified-data-table/src/constants.ts

[Discover] Update header background

bdcbf1d

[Discover] Update document view

1c71502

Merge branch 'main' into redesign-checkboxes

159dcbb

jughosta marked this pull request as ready for review September 4, 2023 15:58

jughosta requested review from kertal, MichaelMarcialis and andreadelrio September 4, 2023 15:59

jughosta added 2 commits September 4, 2023 20:45

[Discover] Update jest snapshots

fa58fa3

Merge branch 'main' into redesign-checkboxes

e575c01

kertal reviewed Sep 5, 2023

View reviewed changes

[Discover] Swap checkbox and arrow back

604d722

jughosta added 2 commits September 5, 2023 16:09

[Discover] Update context app layout to remove extra horizontal margin

7e59c76

[Discover] Add top border to grid controls

46d813c

jughosta added 2 commits September 5, 2023 17:31

[Discover] Fix tests

2e783c3

[Discover] Remove comment

1f86e50

Merge branch 'main' into redesign-checkboxes

22637f5

kertal self-requested a review September 6, 2023 10:33

kertal approved these changes Sep 6, 2023

View reviewed changes

jughosta closed this Sep 6, 2023

jughosta mentioned this pull request Sep 6, 2023

[Discover] Redesign for the grid, panels and sidebar v1 #165866

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] Update data table grid styles #164187

[Discover] Update data table grid styles #164187

jughosta commented Aug 17, 2023 •

edited

Loading

elasticmachine commented Aug 17, 2023

MichaelMarcialis left a comment

MichaelMarcialis Aug 22, 2023

MichaelMarcialis Aug 22, 2023

andreadelrio Aug 22, 2023

kertal Aug 23, 2023

MichaelMarcialis Aug 22, 2023

andreadelrio Aug 22, 2023 •

edited

Loading

kertal Aug 23, 2023

kertal Aug 24, 2023

MichaelMarcialis Aug 22, 2023

jughosta Sep 4, 2023

MichaelMarcialis Aug 22, 2023

jughosta Sep 4, 2023

kertal left a comment

jughosta commented Sep 5, 2023

jughosta commented Sep 5, 2023 •

edited

Loading

andreadelrio commented Sep 6, 2023 •

edited

Loading

andreadelrio commented Sep 6, 2023

kibana-ci commented Sep 6, 2023 •

edited

Loading

kertal left a comment

jughosta commented Sep 6, 2023

[Discover] Update data table grid styles #164187

[Discover] Update data table grid styles #164187

Conversation

jughosta commented Aug 17, 2023 • edited Loading

Summary

elasticmachine commented Aug 17, 2023

MichaelMarcialis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreadelrio Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kertal left a comment

Choose a reason for hiding this comment

jughosta commented Sep 5, 2023

jughosta commented Sep 5, 2023 • edited Loading

andreadelrio commented Sep 6, 2023 • edited Loading

andreadelrio commented Sep 6, 2023

kibana-ci commented Sep 6, 2023 • edited Loading

💔 Build Failed

Failed CI Steps

Test Failures

Metrics [docs]

Async chunks

History

kertal left a comment

Choose a reason for hiding this comment

jughosta commented Sep 6, 2023

jughosta commented Aug 17, 2023 •

edited

Loading

andreadelrio Aug 22, 2023 •

edited

Loading

jughosta commented Sep 5, 2023 •

edited

Loading

andreadelrio commented Sep 6, 2023 •

edited

Loading

kibana-ci commented Sep 6, 2023 •

edited

Loading