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] Redesign for the grid, panels and sidebar v1 #165866

Merged
merged 76 commits into from
Sep 12, 2023

Conversation

jughosta
Copy link
Contributor

@jughosta jughosta commented Sep 6, 2023

Summary

Part 1

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 [UnifiedDataTable] Redesign Document column #164634)
  • updates Surrounding Documents side paddings

Part 2

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

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
Screenshot 2023-09-07 at 14 39 48

Please note that "auto" row height is in a separate PR which is also ready for review #164218

jughosta and others added 30 commits August 17, 2023 16:46
# Conflicts:
#	packages/kbn-unified-data-table/src/components/data_table.scss
#	packages/kbn-unified-data-table/src/constants.ts
@andreadelrio
Copy link
Contributor

@jughosta I noticed that there is something off when Row height is set to Single.
image

Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

Thanks for the latest changes! They look good for the most part, but I noticed that because we added the padding back to the list item when dragging, it meant we needed to add it to the regular list item too, which doesn't match the designs. It also seems like the white background will reappear when a list item is active, which looks a bit strange:
list_item

I think I found a fix for it though where we can add a custom class to style the list item while dragging without having to change the regular list items too. I did some testing across browsers and it seems to work consistently. I opened up a PR for it here, let me know what you think and if it makes sense to merge: jughosta#6.

I also noticed that when viewing the documents column in single line mode, the line below is partially visible:
single_line

@jughosta
Copy link
Contributor Author

jughosta commented Sep 9, 2023

@andreadelrio

I noticed that there is something off when Row height is set to Single.

@davismcphee

I also noticed that when viewing the documents column in single line mode, the line below is partially visible

Addressed these issues via a93943c

[Discover Redesign] Improve Unified Field List item drag styles
@jughosta jughosta requested a review from a team as a code owner September 11, 2023 07:42
Comment on lines +313 to +324

// Apply an optional class to the element being dragged so the ghost
// can be styled. We must add it to the actual element for a single
// frame before removing it so the ghost picks up the styling.
const current = e.currentTarget;

if (dragClassName && !current.classList.contains(dragClassName)) {
current.classList.add(dragClassName);
requestAnimationFrame(() => {
current.classList.remove(dragClassName);
});
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davismcphee helped here to improve the view of a dragging item on Discover jughosta#6

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
discover 632 649 +17
eventAnnotation 428 445 +17
lens 1139 1156 +17
logExplorer 319 336 +17
total +68

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-field-list 275 276 +1

Async chunks

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

id before after diff
discover 551.0KB 556.4KB +5.4KB
lens 1.4MB 1.4MB +265.0B
unifiedHistogram 48.5KB 48.4KB -154.0B
total +5.5KB
Unknown metric groups

API count

id before after diff
@kbn/unified-field-list 302 303 +1

History

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

cc @jughosta

Copy link
Contributor

@andreadelrio andreadelrio left a comment

Choose a reason for hiding this comment

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

Design changes for v1 LGTM! Nice job @jughosta.

Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes! I did a final round of testing and confirmed the latest issues were fixed, and I didn't notice anything else. Awesome work on this, it's such a big improvement to Discover's UI, LGTM 👍

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

This is indeed awesome Julia! LGTM!

@jughosta jughosta merged commit 6cb937a into elastic:main Sep 12, 2023
@jughosta jughosta deleted the discover-redesign-v1 branch September 12, 2023 06:51
jughosta added a commit that referenced this pull request Sep 30, 2023
A follow up for #165866

## Summary

This PR replaces icons for the sidebar toggle button.

<img width="200" alt="Screenshot 2023-09-29 at 13 00 39"
src="https://github.com/elastic/kibana/assets/1415710/6ed10562-9a50-48ce-b6b2-030ab7b11e11">
<img width="200" alt="Screenshot 2023-09-29 at 13 00 47"
src="https://github.com/elastic/kibana/assets/1415710/e286e152-a6bb-4b21-a97a-44419757dafb">
jughosta added a commit that referenced this pull request Oct 5, 2023
## Summary

Since we are not merging #167179
yet, I cherry-picked a fix for the issue with the tabs border above the
grid which was introduced in
#165866 and can be backported to
8.11.

Before:
<img width="400" alt="Screenshot 2023-10-04 at 11 01 13"
src="https://github.com/elastic/kibana/assets/1415710/68b85c1d-c1f5-4955-8a33-35d140157744">
<img width="400" alt="Screenshot 2023-10-04 at 11 01 52"
src="https://github.com/elastic/kibana/assets/1415710/3a401fef-1f05-4a1e-8a4d-b84bd6f00be9">

After:
<img width="400" alt="Screenshot 2023-10-04 at 11 02 55"
src="https://github.com/elastic/kibana/assets/1415710/94907e5a-f9dc-476b-9c65-dc655ded8caa">
<img width="400" alt="Screenshot 2023-10-04 at 11 03 13"
src="https://github.com/elastic/kibana/assets/1415710/a0bf0fa0-bffb-494f-ba20-aea9f566ac07">

---------

Co-authored-by: Michael Marcialis <[email protected]>
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 5, 2023
## Summary

Since we are not merging elastic#167179
yet, I cherry-picked a fix for the issue with the tabs border above the
grid which was introduced in
elastic#165866 and can be backported to
8.11.

Before:
<img width="400" alt="Screenshot 2023-10-04 at 11 01 13"
src="https://github.com/elastic/kibana/assets/1415710/68b85c1d-c1f5-4955-8a33-35d140157744">
<img width="400" alt="Screenshot 2023-10-04 at 11 01 52"
src="https://github.com/elastic/kibana/assets/1415710/3a401fef-1f05-4a1e-8a4d-b84bd6f00be9">

After:
<img width="400" alt="Screenshot 2023-10-04 at 11 02 55"
src="https://github.com/elastic/kibana/assets/1415710/94907e5a-f9dc-476b-9c65-dc655ded8caa">
<img width="400" alt="Screenshot 2023-10-04 at 11 03 13"
src="https://github.com/elastic/kibana/assets/1415710/a0bf0fa0-bffb-494f-ba20-aea9f566ac07">

---------

Co-authored-by: Michael Marcialis <[email protected]>
(cherry picked from commit 6990b1a)
kibanamachine added a commit that referenced this pull request Oct 5, 2023
# Backport

This will backport the following commits from `main` to `8.11`:
- [[Discover] Fix tabs border above the grid
(#167967)](#167967)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Julia
Rechkunova","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-10-05T09:49:37Z","message":"[Discover]
Fix tabs border above the grid (#167967)\n\n## Summary\r\n\r\nSince we
are not merging https://github.com/elastic/kibana/pull/167179\r\nyet, I
cherry-picked a fix for the issue with the tabs border above the\r\ngrid
which was introduced in\r\nhttps://github.com//pull/165866
and can be backported to\r\n8.11.\r\n\r\nBefore:\r\n<img width=\"400\"
alt=\"Screenshot 2023-10-04 at 11 01
13\"\r\nsrc=\"https://github.com/elastic/kibana/assets/1415710/68b85c1d-c1f5-4955-8a33-35d140157744\">\r\n<img
width=\"400\" alt=\"Screenshot 2023-10-04 at 11 01
52\"\r\nsrc=\"https://github.com/elastic/kibana/assets/1415710/3a401fef-1f05-4a1e-8a4d-b84bd6f00be9\">\r\n\r\nAfter:\r\n<img
width=\"400\" alt=\"Screenshot 2023-10-04 at 11 02
55\"\r\nsrc=\"https://github.com/elastic/kibana/assets/1415710/94907e5a-f9dc-476b-9c65-dc655ded8caa\">\r\n<img
width=\"400\" alt=\"Screenshot 2023-10-04 at 11 03
13\"\r\nsrc=\"https://github.com/elastic/kibana/assets/1415710/a0bf0fa0-bffb-494f-ba20-aea9f566ac07\">\r\n\r\n---------\r\n\r\nCo-authored-by:
Michael Marcialis
<[email protected]>","sha":"6990b1a38c9b264d8c251a2cce83697a060600fa","branchLabelMapping":{"^v8.12.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Discover","release_note:skip","Team:DataDiscovery","backport:prev-minor","v8.12.0"],"number":167967,"url":"https://github.com/elastic/kibana/pull/167967","mergeCommit":{"message":"[Discover]
Fix tabs border above the grid (#167967)\n\n## Summary\r\n\r\nSince we
are not merging https://github.com/elastic/kibana/pull/167179\r\nyet, I
cherry-picked a fix for the issue with the tabs border above the\r\ngrid
which was introduced in\r\nhttps://github.com//pull/165866
and can be backported to\r\n8.11.\r\n\r\nBefore:\r\n<img width=\"400\"
alt=\"Screenshot 2023-10-04 at 11 01
13\"\r\nsrc=\"https://github.com/elastic/kibana/assets/1415710/68b85c1d-c1f5-4955-8a33-35d140157744\">\r\n<img
width=\"400\" alt=\"Screenshot 2023-10-04 at 11 01
52\"\r\nsrc=\"https://github.com/elastic/kibana/assets/1415710/3a401fef-1f05-4a1e-8a4d-b84bd6f00be9\">\r\n\r\nAfter:\r\n<img
width=\"400\" alt=\"Screenshot 2023-10-04 at 11 02
55\"\r\nsrc=\"https://github.com/elastic/kibana/assets/1415710/94907e5a-f9dc-476b-9c65-dc655ded8caa\">\r\n<img
width=\"400\" alt=\"Screenshot 2023-10-04 at 11 03
13\"\r\nsrc=\"https://github.com/elastic/kibana/assets/1415710/a0bf0fa0-bffb-494f-ba20-aea9f566ac07\">\r\n\r\n---------\r\n\r\nCo-authored-by:
Michael Marcialis
<[email protected]>","sha":"6990b1a38c9b264d8c251a2cce83697a060600fa"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.12.0","labelRegex":"^v8.12.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/167967","number":167967,"mergeCommit":{"message":"[Discover]
Fix tabs border above the grid (#167967)\n\n## Summary\r\n\r\nSince we
are not merging https://github.com/elastic/kibana/pull/167179\r\nyet, I
cherry-picked a fix for the issue with the tabs border above the\r\ngrid
which was introduced in\r\nhttps://github.com//pull/165866
and can be backported to\r\n8.11.\r\n\r\nBefore:\r\n<img width=\"400\"
alt=\"Screenshot 2023-10-04 at 11 01
13\"\r\nsrc=\"https://github.com/elastic/kibana/assets/1415710/68b85c1d-c1f5-4955-8a33-35d140157744\">\r\n<img
width=\"400\" alt=\"Screenshot 2023-10-04 at 11 01
52\"\r\nsrc=\"https://github.com/elastic/kibana/assets/1415710/3a401fef-1f05-4a1e-8a4d-b84bd6f00be9\">\r\n\r\nAfter:\r\n<img
width=\"400\" alt=\"Screenshot 2023-10-04 at 11 02
55\"\r\nsrc=\"https://github.com/elastic/kibana/assets/1415710/94907e5a-f9dc-476b-9c65-dc655ded8caa\">\r\n<img
width=\"400\" alt=\"Screenshot 2023-10-04 at 11 03
13\"\r\nsrc=\"https://github.com/elastic/kibana/assets/1415710/a0bf0fa0-bffb-494f-ba20-aea9f566ac07\">\r\n\r\n---------\r\n\r\nCo-authored-by:
Michael Marcialis
<[email protected]>","sha":"6990b1a38c9b264d8c251a2cce83697a060600fa"}}]}]
BACKPORT-->

Co-authored-by: Julia Rechkunova <[email protected]>
dej611 pushed a commit to dej611/kibana that referenced this pull request Oct 17, 2023
## Summary

Since we are not merging elastic#167179
yet, I cherry-picked a fix for the issue with the tabs border above the
grid which was introduced in
elastic#165866 and can be backported to
8.11.

Before:
<img width="400" alt="Screenshot 2023-10-04 at 11 01 13"
src="https://github.com/elastic/kibana/assets/1415710/68b85c1d-c1f5-4955-8a33-35d140157744">
<img width="400" alt="Screenshot 2023-10-04 at 11 01 52"
src="https://github.com/elastic/kibana/assets/1415710/3a401fef-1f05-4a1e-8a4d-b84bd6f00be9">

After:
<img width="400" alt="Screenshot 2023-10-04 at 11 02 55"
src="https://github.com/elastic/kibana/assets/1415710/94907e5a-f9dc-476b-9c65-dc655ded8caa">
<img width="400" alt="Screenshot 2023-10-04 at 11 03 13"
src="https://github.com/elastic/kibana/assets/1415710/a0bf0fa0-bffb-494f-ba20-aea9f566ac07">

---------

Co-authored-by: Michael Marcialis <[email protected]>
@qd-danh
Copy link

qd-danh commented Nov 17, 2023

not liking the look of the data grid without vertical lines. Now the data columns just seem to fall off into the empty space. Is this a global change and will remain going forward?

Wish this was configurable based on a setting somewhere.

before
image

after
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Discover Discover Application release_note:enhancement Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. v8.11.0
Projects
None yet
10 participants