[Discover] New responsive layout using EUI components #83633

kertal · 2020-11-18T14:02:55Z

Summary

Responsiveness of Discover was powered by Bootstrap classes, with this PR it's replaced by EUI. The ways it works is a bit different to before, there is a Flyout containing the field selector in the mobile version. This prepares Discover for #67259 aka Data Grid (Basically it's work, extracted from there).

The layout has changed, moved to EuiPage components, sidebar and results are now scrollable separately:

You can hide the date histogram to gain more vertical screen space (Histogram query is still submitted, but this will change).

And here's the new responsiveness in action:

Other changes

Doc Viewer Table action icons moved to the right side of the table.

Changes in this PR were mainly done by @andreadelrio 👍
FYI @gchaps we need to check the docs if there are screens to update

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
~~- [ ] Documentation was added for features that require explanation or tutorials~~
-[x] Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

~~- [ ] This was checked for breaking API changes and was labeled appropriately~~

…discover-data-grid

flash1293

All my previous comments have been addressed, LGTM from my side.

This is probably a personal thing, but I don't really like the new style of the index pattern picker, especially along with the other elements in that area - everything looks slightly different (colors, font weights, edges, ...), the hits count is not aligned to the elements in the sidebar, the sidebar collapse button feels a little misplaced with its different color floating in between the other elements. Biggest part of that is probably me being used to the old style :)

I found one minor thing, but it's not blocking this IMHO:
If the table is scrollable horizontally, the "End of page" message at the end of the table is not taking the full width (not sure how important):

This definitely needs another pair of eyes before merging as there are so many changes going on.

flash1293 · 2020-12-01T16:41:13Z

src/plugins/discover/public/application/components/sidebar/discover_sidebar_responsive.tsx

+  /**
+   * hits fetched from ES, displayed in the doc table
+   */
+  hits: Array<Record<string, unknown>>;


nit: Could this be ElasticSearchHit[]?

of course it could, and now it is! thx!

kertal · 2020-12-01T17:18:11Z

Thx @flash1293 for your valuable feedback!

This definitely needs another pair of eyes before merging as there are so many changes going on.

Dear @andreadelrio & @cchaos could your eyes have another look? Many thx!

src/plugins/discover/public/application/angular/directives/uninitialized.tsx

src/plugins/discover/public/application/components/skip_bottom_button/skip_bottom_button.tsx

src/plugins/discover/public/application/components/timechart_header/timechart_header.tsx

src/plugins/discover/public/application/components/doc/doc.tsx

cchaos · 2020-12-02T17:17:51Z

This is probably a personal thing, but I don't really like the new style of the index pattern picker, especially along with the other elements in that area - everything looks slightly different (colors, font weights, edges, ...), the hits count is not aligned to the elements in the sidebar, the sidebar collapse button feels a little misplaced with its different color floating in between the other elements. Biggest part of that is probably me being used to the old style :)

I agree that there are some great inconsistencies here but most of that will be mitigated when we switch to the new K8 theme. Example:

The main change here really was making the index pattern selection more prominent by making it the border-style button. This button control the entire data of this screen and it was getting lost. The second update was actually removing the compressed style of the field search box so it lines up better size-wise with the rest of the controls.

And the mobile version (both being bordered buttons) is actually more consistent.

andreadelrio · 2020-12-02T18:02:43Z

@kertal filed issue #84805 so we can tackle the footer problem Joe brought up in a follow up PR. From the design side this should be ready to be merged.

flash1293 · 2020-12-02T18:09:22Z

Thanks for the explanation, @cchaos - the K8 theme mockup looks great

…-responsive-layout

kertal · 2020-12-03T07:57:58Z

@elasticmachine merge upstream

majagrubic

Just a few quick code comments while I'm testing this.

majagrubic · 2020-12-03T08:32:01Z

src/plugins/discover/public/application/components/discover_legacy.tsx

-                            onRemoveColumn={onRemoveColumn}
-                            onSort={onSort}
+                )}
+                {resultState === 'uninitialized' && <DiscoverUninitialized onRefresh={fetch} />}


Can we use ternary statements here, I think it would improve readability?
resultState === 'uninitialized' ? <DiscoverUninitialized onRefresh={fetch} : null

But I think then we should do it the same way throughout the file? for consistency. the readability is subjective, but what's an argument for ternary is, that it's explicit. issue with && are very well described here: https://kentcdodds.com/blog/use-ternaries-rather-than-and-and-in-jsx
However I'd prefer changes here later on (since it's not a single line)

src/plugins/discover/public/application/components/discover_legacy.tsx

majagrubic · 2020-12-03T08:34:35Z

src/plugins/discover/public/application/components/discover_legacy.tsx


  return (
    <I18nProvider>
-      <div className="dscAppContainer" data-fetch-counter={fetchCounter}>
-        <h1 className="euiScreenReaderOnly">{savedSearch.title}</h1>
+      <EuiPage className="dscPage" data-fetch-counter={fetchCounter}>


I think we could break down the render function into smaller functions, to improve readability. Eg. renderSidebar, renderRows etc.

If you don't mind we could do that later on ⏳

majagrubic · 2020-12-03T08:39:04Z

test/functional/apps/discover/_discover.js

@@ -114,7 +114,7 @@ export default function ({ getService, getPageObjects }) {
        await PageObjects.discover.waitUntilSearchingHasFinished();

        const newDurationHours = await PageObjects.timePicker.getTimeDurationInHours();
-        expect(Math.round(newDurationHours)).to.be(24);
+        expect(Math.round(newDurationHours)).to.be(26);


why this change?

The layout changed, and so the width of the date histogram, but what didn't change is the width of the histogram brushing with the mouse. Because the date histogram is a bit smaller, a longer time range is selected. that's why more hours were selected. (It's a good question, also I've wondered why this test was failing initially)

…al/kibana into kertal-2020-11-18-responsive-layout

majagrubic

Tested in Chrome on Mac OS, looks great!

Co-authored-by: Andrea Del Rio <delrio.andre@gmail.com> Co-authored-by: cchaos <caroline.horn@elastic.co> Co-authored-by: Michail Yasonik <michail.yasonik@elastic.co> Co-authored-by: Dave Snider <dave.snider@gmail.com> Co-authored-by: Marta Bondyra <marta.bondyra@elastic.co> # Conflicts: # src/plugins/discover/public/application/angular/directives/fixed_scroll.test.js

Co-authored-by: Andrea Del Rio <delrio.andre@gmail.com> Co-authored-by: cchaos <caroline.horn@elastic.co> Co-authored-by: Michail Yasonik <michail.yasonik@elastic.co> Co-authored-by: Dave Snider <dave.snider@gmail.com> Co-authored-by: Marta Bondyra <marta.bondyra@elastic.co>

kibanamachine · 2020-12-04T11:27:37Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: c687048

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`discover`	279	312	+33

Async chunks

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

id	before	after	diff
`discover`	432.2KB	450.3KB	+18.1KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`discover`	82.1KB	82.0KB	-93.0B

Unknown metric groups

async chunk count

id	before	after	diff
`discover`	7	6	-1

History

💚 Build #91690 succeeded c687048
💚 Build #91663 succeeded 6a8acc8
💚 Build #91629 succeeded ee164b0
💔 Build #91548 failed 9fa4f3f
💚 Build #90847 succeeded 9ab6fd6

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

Michail Yasonik and others added 30 commits January 14, 2020 16:53

initial discover DataGrid proof of concept

e6671e5

fixing scss import

cd000cf

Merge remote-tracking branch 'upstream/master' into discover-data-grid

581fe40

bringing back old table with advanced setting

c13b349

fixing old tests

5f19bf2

removing hide fields action and fixing field add/sub bug

6543d26

fixing old tests

f4fd8c2

adding tests

472e240

added new test

6105c05

Merge remote-tracking branch 'upstream/master' into discover-data-grid

5721112

json field formatter work

4d4b63f

fixing old test

179ee06

futzing with tests

9cf8c75

i18n fix

a81dc9a

only show _source when no other columns are shown

977c637

more test fixes

887bf3a

Merge remote-tracking branch 'upstream/master' into discover-data-grid

61422fd

Merge remote-tracking branch 'upstream/master' into discover-data-grid

d6c156c

Merge remote-tracking branch 'upstream/master' into discover-data-grid

44c4319

add docs for new legacyTable advanced setting

70c6a33

basic layout

7ac7f9b

design changes to dg

b50c90b

Merge remote-tracking branch 'upstream/master' into discover-data-grid

9eb57e6

perf improvements

156a558

Merge branch 'discover-data-grid' of github.com:myasonik/kibana into …

8761909

…discover-data-grid

basic layout and scrolling now works

f6b2cee

attempting to get tests to pass again

78a7405

fixed infinite scroll of legacy table

2bc70e9

fixing sorting and schema detection and other bugs

1702235

Merge remote-tracking branch 'upstream/master' into discover-data-grid

b3b873d

flash1293 approved these changes Dec 1, 2020

View reviewed changes

gchaps reviewed Dec 1, 2020

View reviewed changes

andreadelrio mentioned this pull request Dec 2, 2020

[Discover] Doc table footer not going full width #84805

Closed

andreadelrio self-requested a review December 2, 2020 18:03

andreadelrio approved these changes Dec 2, 2020

View reviewed changes

kertal added 2 commits December 2, 2020 23:21

Address wording review comments

27779c1

Merge remote-tracking branch 'upstream/master' into kertal-2020-11-18…

9fa4f3f

…-responsive-layout

Merge branch 'master' into kertal-2020-11-18-responsive-layout

ee164b0

majagrubic reviewed Dec 3, 2020

View reviewed changes

kertal added 3 commits December 3, 2020 11:09

Add i18 for showing, hiding the chart

0ebf5a7

Merge branch 'kertal-2020-11-18-responsive-layout' of github.com:kert…

6a8acc8

…al/kibana into kertal-2020-11-18-responsive-layout

Migrate types to use ElasticSearchHit

c687048

majagrubic approved these changes Dec 3, 2020

View reviewed changes

kertal merged commit 95f8d9d into elastic:master Dec 3, 2020

kertal mentioned this pull request Dec 3, 2020

[7.x] [Discover] New responsive layout using EUI components (#83633) #84911

Merged

This was referenced Dec 16, 2020

Design adaptions of Discover's chart #45629

Closed

[Discover] Improve "hide/show chart" to be persistable #86293

Closed

timroes mentioned this pull request Dec 17, 2020

Hide histogram on Discover page #6164

Closed

kertal mentioned this pull request Jan 27, 2021

[Discover] Add "Hide chart" / "Show chart" persistence #88603

Merged

1 task

kertal mentioned this pull request Dec 2, 2021

Page Up/Down keys stopped working in Discover app in recent versions #120180

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] New responsive layout using EUI components #83633

[Discover] New responsive layout using EUI components #83633

kertal commented Nov 18, 2020 •

edited

Loading

flash1293 left a comment •

edited

Loading

flash1293 Dec 1, 2020

kertal Dec 3, 2020

kertal commented Dec 1, 2020

cchaos commented Dec 2, 2020

andreadelrio commented Dec 2, 2020

flash1293 commented Dec 2, 2020

kertal commented Dec 3, 2020

majagrubic left a comment

majagrubic Dec 3, 2020

kertal Dec 3, 2020

majagrubic Dec 3, 2020

kertal Dec 3, 2020

majagrubic Dec 3, 2020

kertal Dec 3, 2020 •

edited

Loading

majagrubic left a comment

kibanamachine commented Dec 4, 2020

async chunk count

[Discover] New responsive layout using EUI components #83633

[Discover] New responsive layout using EUI components #83633

Conversation

kertal commented Nov 18, 2020 • edited Loading

Summary

Other changes

Checklist

For maintainers

flash1293 left a comment • edited Loading

Choose a reason for hiding this comment

flash1293 Dec 1, 2020

Choose a reason for hiding this comment

kertal Dec 3, 2020

Choose a reason for hiding this comment

kertal commented Dec 1, 2020

cchaos commented Dec 2, 2020

andreadelrio commented Dec 2, 2020

flash1293 commented Dec 2, 2020

kertal commented Dec 3, 2020

majagrubic left a comment

Choose a reason for hiding this comment

majagrubic Dec 3, 2020

Choose a reason for hiding this comment

kertal Dec 3, 2020

Choose a reason for hiding this comment

majagrubic Dec 3, 2020

Choose a reason for hiding this comment

kertal Dec 3, 2020

Choose a reason for hiding this comment

majagrubic Dec 3, 2020

Choose a reason for hiding this comment

kertal Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

majagrubic left a comment

Choose a reason for hiding this comment

kibanamachine commented Dec 4, 2020

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

async chunk count

History

kertal commented Nov 18, 2020 •

edited

Loading

flash1293 left a comment •

edited

Loading

kertal Dec 3, 2020 •

edited

Loading