Use push flyout for Discover document flyout #166406

lukasolson · 2023-09-13T23:46:41Z

Summary

Closes #163798.
Closes #160306

Uses "push" style for the document viewer flyout in Discover.

Before:

After:

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios

lukasolson · 2023-09-13T23:47:14Z

src/plugins/discover/public/components/discover_grid_flyout/discover_grid_flyout.tsx

@@ -251,6 +254,15 @@ export function DiscoverGridFlyout({
            textBasedHits={isPlainRecord ? hits : undefined}
          />
        </EuiFlyoutBody>
+        <EuiFlyoutFooter>


From the EUI docs (https://elastic.github.io/eui/#/layout/flyout#push-versus-overlay):

Also, it is good to include a close button in the footer for a larger hit target than the small close button provides.

…n/kibana into unified-doc-viewer/push-flyout

elasticmachine · 2023-09-29T05:17:18Z

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

kertal · 2023-09-29T05:19:36Z

src/plugins/discover/public/components/discover_grid_flyout/discover_grid_flyout.tsx

+          <EuiFlexGroup>
+            <EuiFlexItem grow={false}>
+              <EuiButtonEmpty iconType="cross" onClick={onClose} flush="left">
+                Close


yes, this make sense, one ask, pls add i18n for Close 🙏

kertal · 2023-09-29T05:20:06Z

@elasticmachine merge upstream

davismcphee

Looking good overall! A couple point of feedback from testing:

Now that the sidebar is resizable and the doc flyout pushes the content, the sidebar and main panels can become squished when the flyout is open and the screen is on the smaller side of XL (so still a push flyout). It seems like reducing the min width of the main panel improves this. I'd recommend updating discover_resizable_layout.tsx to use const minMainPanelWidth = euiTheme.base * 24; instead of the current value.

I also noticed that since the flyout pushes the content when it opens, you can lose sight of the selected document row as the data grid resizes. I wonder if it's possible to do something like auto-scroll to the selected doc after opening the flyout?

It also looks like after I close the push flyout, there are some rendering issues with the data grid. This is likely just a bug with EuiDataGrid, but I wonder if we can do something about it. It's possible that implementing some sort of auto-scroll functionality might force the data grid to render and fix this issue as well:

Otherwise I do find the flyout a bit wide overall. Even on a large screen it took up about half the space, but I think this could be reduced when we migrate to floating actions in the doc viewer table, so I wouldn't consider it blocking unless others feel strongly about it.

kertal · 2023-10-02T16:50:25Z

I feel like, given there were new changes under the hood, like the sizable field list, and also the potential vanishing of the actually selected row when the autosized document column is rendered, it should be reviewed again for design changes, and in the context of our ongoing redesign FYI @andreadelrio @MichaelMarcialis @ninoslavmiskovic
I feel like we should not rush in this significant change, here's a deployment to test:
deployment

andreadelrio · 2023-10-02T18:17:24Z

I feel like, given there were new changes under the hood, like the sizable field list, and also the potential vanishing of the actually selected row when the autosized document column is rendered, it should be reviewed again for design changes, and in the context of our ongoing redesign FYI @andreadelrio @MichaelMarcialis @ninoslavmiskovic I feel like we should not rush in this significant change, here's a deployment to test: deployment

Seeing this in action, I think I would not implement this change until we implement #164617. The left white space for me is a blocker to implement this push flyout. When we move to floating actions we'll be able to reduce the width of this flyout. I also think that when we implement the push flyout it should come with the ability to drag-to-resize, similar to what we now have for the field list.

ninoslavmiskovic · 2023-10-02T18:50:24Z

The vanishing row is a bug in my opinion.

Also I did not expect the push fly out to be that "aggressive" on resizing.

Could we e.g work with how it pushes the content or collapse the field list automatically?

Agree on next iteration with working on the re-sizing, however before see it as a blocker.

Is there anything we can work on how it renders ?

kertal · 2023-10-04T05:13:18Z

src/plugins/discover/public/components/discover_grid_flyout/discover_grid_flyout.tsx

        size="m"
+        pushMinBreakpoint="xl"
        data-test-subj="docTableDetailsFlyout"
        onKeyDown={onKeyDown}
        ownFocus={false}


I think ownFocus should be true now, this would not apply for the push, but for the overlay style

Suggested change

ownFocus={false}

ownFocus={true}

…n/kibana into unified-doc-viewer/push-flyout

lukasolson · 2024-04-16T23:23:13Z

x-pack/test/functional/apps/observability_logs_explorer/flyout.ts


+    it('should not display a log level badge when not available', async () => {


For some reason this test (and the others below) were failing because the web elements had become "stale", and were throwing errors. I'm not sure how the changes in this PR would affect this, but breaking these into separate it() statements seems to fix the issue and still test the same behavior.

This PR will actually remove most of these tests after moving the doc view implementation, I'll take care of the conflicts but this LGTM.

kertal · 2024-04-17T07:03:41Z

...k/test_serverless/functional/test_suites/observability/observability_logs_explorer/flyout.ts

@@ -81,7 +81,9 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
      await dataGrid.clickRowToggle({ columnIndex: 4 });
      await testSubjects.existOrFail('logsExplorerFlyoutLogLevel');
      await dataGrid.closeFlyout();
+    });


Nit: I think the flyouts no longer need to be closed in those tests, since we navigate to Discover before each test

kertal

LGTM, looking great and working snappy 👍
Tests locally with Safari, Firefox, Chrome, all fine and work as expected.
A note about the tests, that's pretty strange, it seems when using the push flyout our code in the test to get the row cells no longer works. Might be caused because when the push flyout is closed, this leads the table / layout to be pre-rendered. Not a blocker but we might need to investigate what's happening here , what also might have worked instead of creating separate tests, is just to not close the flout when showing another row, since this should work with the push flyout

…n/kibana into unified-doc-viewer/push-flyout

Addressed

davismcphee

Just about good to go on my end! A couple of minor points of feedback, but it's just about there IMO.

I did notice that my workaround to prevent breaking the Discover layout doesn't work when there's a nav sidebar like in serverless or the O11y solution nav, but I don't think there's much we can do about that unless we make all of Discover responsive based on container width, so I think it's good for now:

davismcphee · 2024-04-23T22:52:11Z

src/plugins/discover/public/components/discover_grid_flyout/discover_grid_flyout.tsx

@@ -230,6 +232,10 @@ export function DiscoverGridFlyout({
        minWidth={minWidth}
        maxWidth={maxWidth}
        onResize={setFlyoutWidth}
+        css={{
+          maxWidth: `${isXlScreen ? `calc(100vw - ${defaultWidth}px)` : '90vw'} !important`,


I didn't realize this when I originally opened my touchups PR, but there's going to be an issue with using defaultWidth here since it could be a string value when overridden by a customization (e.g. Logs Explorer uses 60%). I think we can just split out euiTheme.base * 34 into a separate const to use here and we should be safe.

davismcphee · 2024-04-23T22:59:37Z

src/plugins/discover/public/components/discover_grid_flyout/discover_grid_flyout.tsx

+        css={{
+          maxWidth: `${isXlScreen ? `calc(100vw - ${defaultWidth}px)` : '90vw'} !important`,
+        }}
+        className="dscGridFlyout"


Rather than using a class with a new style, we should be able to instead use paddingSize="m" to get the 16px padding. This will also apply the padding to the header which right now is still 24px.

davismcphee · 2024-04-23T23:03:16Z

test/accessibility/apps/discover.ts

@@ -133,7 +133,8 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
    it('a11y test for actions on a field', async () => {
      await PageObjects.discover.clickDocViewerTab('doc_view_table');
      if (await testSubjects.exists('openFieldActionsButton-Cancelled')) {
-        await testSubjects.click('openFieldActionsButton-Cancelled');
+        await testSubjects.click('openFieldActionsButton-Cancelled'); // Open the actions
+        await testSubjects.click('openFieldActionsButton-Cancelled'); // Close the actions


I think we should close the menu after we call await a11y.testAppSnapshot(); to make sure our a11y snapshot includes the open menu.

andreadelrio

LGTM, thanks!

tonyghiani

LGTM

kibana-ci · 2024-04-24T23:59:05Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: f48a840

Failed CI Steps

FTR Configs #53

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`	636.1KB	639.0KB	+2.8KB
`unifiedDocViewer`	60.8KB	60.9KB	+117.0B
total			+3.0KB

History

💛 Build #205388 was flaky 8d69a43
💛 Build #205113 was flaky 9abcf27
💚 Build #204245 succeeded 8cfa359
💔 Build #203866 failed 42d6612
💔 Build #203855 failed 8e362f3
💔 Build #203790 failed ec72196

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

cc @lukasolson @andreadelrio

davismcphee

Looks like my remaining concerns are addressed, so in that case this LGTM 👍 Great work on this, and I'm excited to get it merged!

I noticed one minor issue where the Logs Explorer flyout default width isn't being set now, but that's actually unrelated to these changes and is a separate bug with the customization framework, so I'll open up a separate PR to address it.

…er (#181788) ## Summary This PR fixes an issue where the `useCustomization` hook in Discover always returns `undefined` on the first render, which can have unintended consequences. This went unnoticed before, but the work in #166406 revealed the issue, resulting in the Logs Explorer flyout initializing to the wrong width. With this fix, the width initializes correctly, but the default value of `60%` causes the layout to become crammed, so I updated it to what seemed like a more reasonable default of `650px`. I can change this to something else if we feel there's a better default. **Before (`60%`):** <img width="1997" alt="before" src="https://github.com/elastic/kibana/assets/25592674/50994ca9-f232-4c5c-a948-46a0e9202864"> **Now (`650px`):** <img width="1997" alt="after" src="https://github.com/elastic/kibana/assets/25592674/dcc8f53f-0f17-4b2f-9e33-9b1ba134d4ae"> ### Checklist - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

## Summary This PR updates the bottom margin so the JSON view can be scrolled properly inside DocViewer flyout. It's a follow up for #183468 (fixes the scrolling to be limited to flyout height) and #166406 (taller footer was added with Close button in it).

Use push flyout for Discover document flyout

cadd8d8

lukasolson self-assigned this Sep 13, 2023

lukasolson commented Sep 13, 2023

View reviewed changes

lukasolson added 5 commits September 25, 2023 11:41

Merge branch 'main' into unified-doc-viewer/push-flyout

23dc630

Merge branch 'main' into unified-doc-viewer/push-flyout

58cc26e

Merge branch 'main' into unified-doc-viewer/push-flyout

b164831

Add export

cea1288

Merge branch 'unified-doc-viewer/push-flyout' of github.com:lukasolso…

deaa9ee

…n/kibana into unified-doc-viewer/push-flyout

lukasolson marked this pull request as ready for review September 27, 2023 22:50

lukasolson requested a review from a team as a code owner September 27, 2023 22:50

kertal added release_note:enhancement Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. labels Sep 29, 2023

kertal added the Feature:Discover Discover Application label Sep 29, 2023

kertal reviewed Sep 29, 2023

View reviewed changes

Merge branch 'main' into unified-doc-viewer/push-flyout

70d7127

davismcphee reviewed Sep 30, 2023

View reviewed changes

kertal requested a review from a team October 2, 2023 16:41

kertal reviewed Oct 4, 2023

View reviewed changes

lukasolson added 3 commits January 12, 2024 10:50

Merge branch 'main' into unified-doc-viewer/push-flyout

083921c

Use resizable flyout

54d24b6

Merge branch 'unified-doc-viewer/push-flyout' of github.com:lukasolso…

a9072e4

…n/kibana into unified-doc-viewer/push-flyout

lukasolson added the ci:cloud-deploy Create or update a Cloud deployment label Jan 12, 2024

Merge branch 'main' into unified-doc-viewer/push-flyout

b583a9d

lukasolson removed the ci:cloud-deploy Create or update a Cloud deployment label Jan 12, 2024

Merge branch 'main' into unified-doc-viewer/push-flyout

ab29999

lukasolson added 2 commits April 16, 2024 11:54

Merge branch 'main' into unified-doc-viewer/push-flyout

4e995f7

Break tests into separate cases

8cfa359

lukasolson requested a review from a team as a code owner April 16, 2024 23:21

lukasolson commented Apr 16, 2024

View reviewed changes

kertal reviewed Apr 17, 2024

View reviewed changes

kertal approved these changes Apr 17, 2024

View reviewed changes

lukasolson added 5 commits April 22, 2024 14:18

Merge branch 'main' into unified-doc-viewer/push-flyout

40be3e2

Merge branch 'main' into unified-doc-viewer/push-flyout

8866baf

Remove closing flyout

f86a39d

Merge branch 'unified-doc-viewer/push-flyout' of github.com:lukasolso…

9abcf27

…n/kibana into unified-doc-viewer/push-flyout

Remove close flyout

8d69a43

davismcphee reviewed Apr 23, 2024

View reviewed changes

andreadelrio approved these changes Apr 24, 2024

View reviewed changes

tonyghiani approved these changes Apr 24, 2024

View reviewed changes

lukasolson added 2 commits April 24, 2024 15:24

Merge branch 'main' into unified-doc-viewer/push-flyout

6f462be

Review feedback

f48a840

lukasolson requested a review from davismcphee April 25, 2024 17:52

davismcphee approved these changes Apr 25, 2024

View reviewed changes

davismcphee mentioned this pull request Apr 26, 2024

[Discover] Fix useCustomization returning undefined on first render #181788

Merged

10 tasks

lukasolson merged commit 9203342 into elastic:main Apr 26, 2024
18 checks passed

kibanamachine added v8.15.0 backport:skip This commit does not require backporting labels Apr 26, 2024

PhilippeOberti mentioned this pull request May 2, 2024

[8.14] [Security Solution][Alert details] - fix timeline ES|QL flyout shown behind (#182296) #182529

Merged

jughosta mentioned this pull request May 15, 2024

[Discover][DocViewer] Adjust bottom margin in flyout #183478

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use push flyout for Discover document flyout #166406

Use push flyout for Discover document flyout #166406

lukasolson commented Sep 13, 2023 •

edited by kertal

Loading

lukasolson Sep 13, 2023

elasticmachine commented Sep 29, 2023

kertal Sep 29, 2023

kertal commented Sep 29, 2023

davismcphee left a comment

kertal commented Oct 2, 2023

andreadelrio commented Oct 2, 2023

ninoslavmiskovic commented Oct 2, 2023

kertal Oct 4, 2023

lukasolson Apr 16, 2024

tonyghiani Apr 24, 2024

kertal Apr 17, 2024

kertal left a comment

davismcphee left a comment

davismcphee Apr 23, 2024

davismcphee Apr 23, 2024

davismcphee Apr 23, 2024

andreadelrio left a comment

tonyghiani left a comment

kibana-ci commented Apr 24, 2024

davismcphee left a comment


		it('should not display a log level badge when not available', async () => {

Use push flyout for Discover document flyout #166406

Use push flyout for Discover document flyout #166406

Conversation

lukasolson commented Sep 13, 2023 • edited by kertal Loading

Summary

Checklist

Choose a reason for hiding this comment

elasticmachine commented Sep 29, 2023

Choose a reason for hiding this comment

kertal commented Sep 29, 2023

davismcphee left a comment

Choose a reason for hiding this comment

kertal commented Oct 2, 2023

andreadelrio commented Oct 2, 2023

ninoslavmiskovic commented Oct 2, 2023

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

davismcphee 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

andreadelrio left a comment

Choose a reason for hiding this comment

tonyghiani left a comment

Choose a reason for hiding this comment

kibana-ci commented Apr 24, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Async chunks

History

davismcphee left a comment

Choose a reason for hiding this comment

lukasolson commented Sep 13, 2023 •

edited by kertal

Loading