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] Optimize popover size for grid cell expansion #152480

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,30 @@
}

.dscDiscoverGrid__cellPopover {
min-height: 140px;
max-width: 900px !important;

&.dscDiscoverGrid__cellPopover--withJson {
resize: horizontal;
min-width: 516px;
width: 516px;
}
Comment on lines +46 to +50
Copy link
Member

Choose a reason for hiding this comment

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

I definitely think, even if we decide not to allow resizing, having the JSON view in a larger dimension makes much sense


&:not(.dscDiscoverGrid__cellPopover--withJson) {
resize: both;
min-width: 300px;
width: 400px;
max-height: 390px !important;
}

& > div {
height: 100%;
}
}

.dscDiscoverGrid__cellPopoverValueContainer {
height: 100%;

// Fixes https://github.com/elastic/kibana/issues/145216 in Chrome
.lines-content.monaco-editor-background {
overflow: unset !important;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import { getSchemaDetectors } from './discover_grid_schema';
import { DiscoverGridFlyout } from './discover_grid_flyout';
import { DiscoverGridContext } from './discover_grid_context';
import { getRenderCellValueFn } from './get_render_cell_value';
import { getRenderCellPopoverFn } from './get_render_cell_popover';
import { DiscoverGridSettings } from './types';
import {
getEuiGridColumns,
Expand Down Expand Up @@ -380,6 +381,8 @@ export const DiscoverGrid = ({
[dataView, displayedRows, useNewFieldsApi, shouldShowFieldHandler, services.uiSettings]
);

const renderCellPopover = useMemo(() => getRenderCellPopoverFn(), []);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: it seems like it would be simpler to pass in something like a CellPopover component directly instead of wrapping it with getRenderCellPopoverFn since we're not using the closure for anything.


/**
* Render variables
*/
Expand Down Expand Up @@ -601,6 +604,7 @@ export const DiscoverGrid = ({
onColumnResize={onResize}
pagination={paginationObj}
renderCellValue={renderCellValue}
renderCellPopover={renderCellPopover}
ref={dataGridRef}
rowCount={rowCount}
schemaDetectors={schemaDetectors}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import React from 'react';
import { mount } from 'enzyme';
import { getRenderCellPopoverFn } from './get_render_cell_popover';
import { EuiDataGridCellPopoverElementProps } from '@elastic/eui';

describe('Discover grid cell popover rendering', function () {
const DiscoverGridCellPopover = getRenderCellPopoverFn() as React.FC<
Pick<EuiDataGridCellPopoverElementProps, 'cellActions' | 'setCellPopoverProps'>
>;
const CellValue = ({ schema }: { schema: string }) => {
return <div>{schema}</div>;
};

it('renders correctly', () => {
const setMock = jest.fn();
const component = mount(
<DiscoverGridCellPopover
cellActions={<button>{'test'}</button>}
setCellPopoverProps={setMock}
>
<CellValue schema="string" />
</DiscoverGridCellPopover>
);
expect(component.html()).toMatchInlineSnapshot(
`"<div class=\\"euiFlexGroup css-4b375e\\"><div class=\\"euiFlexItem css-9sbomz-euiFlexItem-grow-1\\"><div>string</div></div><div class=\\"euiFlexItem css-kpsrin-euiFlexItem-growZero\\"><button>test</button></div></div>"`
);
expect(setMock).toHaveBeenCalledWith({
panelClassName: 'dscDiscoverGrid__cellPopover',
});
});

it('renders correctly for json view', () => {
const setMock = jest.fn();
const component = mount(
<DiscoverGridCellPopover
cellActions={<button>{'test'}</button>}
setCellPopoverProps={setMock}
>
<CellValue schema="kibana-json" />
</DiscoverGridCellPopover>
);
expect(component.html()).toMatchInlineSnapshot(
`"<div class=\\"euiFlexGroup css-4b375e\\"><div class=\\"euiFlexItem css-9sbomz-euiFlexItem-grow-1\\"><div>kibana-json</div></div><div class=\\"euiFlexItem css-kpsrin-euiFlexItem-growZero\\"><button>test</button></div></div>"`
);
expect(setMock).toHaveBeenCalledWith({
panelClassName: 'dscDiscoverGrid__cellPopover dscDiscoverGrid__cellPopover--withJson',
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import React, { useEffect } from 'react';
import { css } from '@emotion/react';
import classnames from 'classnames';
import { EuiDataGridProps, EuiFlexGroup, EuiFlexItem } from '@elastic/eui';

const containerStyles = css`
height: 100%;
`;

export const getRenderCellPopoverFn =
(): EuiDataGridProps['renderCellPopover'] =>
({ cellActions, setCellPopoverProps, children }) => {
useEffect(() => {
setCellPopoverProps({
panelClassName: classnames('dscDiscoverGrid__cellPopover', {
'dscDiscoverGrid__cellPopover--withJson':
children &&
typeof children === 'object' &&
'props' in children &&
children.props?.schema === 'kibana-json',
}),
});
}, [setCellPopoverProps, children]);

return (
<EuiFlexGroup responsive={false} direction="column" gutterSize="none" css={containerStyles}>
<EuiFlexItem grow={true}>{children}</EuiFlexItem>
<EuiFlexItem grow={false}>{cellActions}</EuiFlexItem>
</EuiFlexGroup>
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ describe('Discover grid cell rendering', function () {
/>
);
expect(component.html()).toMatchInlineSnapshot(
`"<div class=\\"euiFlexGroup css-1h68cm-euiFlexGroup-none-flexStart-stretch-row\\"><div class=\\"euiFlexItem css-9sbomz-euiFlexItem-grow-1\\"><span class=\\"dscDiscoverGrid__cellPopoverValue eui-textBreakWord\\">100</span></div><div class=\\"euiFlexItem css-kpsrin-euiFlexItem-growZero\\"><button class=\\"euiButtonIcon euiButtonIcon--xSmall css-1q7ycil-euiButtonIcon-empty-primary-hoverStyles\\" type=\\"button\\" aria-label=\\"Close popover\\" data-test-subj=\\"docTableClosePopover\\"><span data-euiicon-type=\\"cross\\" class=\\"euiButtonIcon__icon\\" aria-hidden=\\"true\\" color=\\"inherit\\"></span></button></div></div>"`
`"<div class=\\"euiFlexGroup css-1h68cm-euiFlexGroup-none-flexStart-stretch-row\\"><div class=\\"euiFlexItem css-9sbomz-euiFlexItem-grow-1\\"><div class=\\"euiText css-f0maoj-euiText-m\\"><span class=\\"dscDiscoverGrid__cellPopoverValue eui-textBreakWord\\">100</span></div></div><div class=\\"euiFlexItem css-kpsrin-euiFlexItem-growZero\\"><button class=\\"euiButtonIcon euiButtonIcon--xSmall css-1q7ycil-euiButtonIcon-empty-primary-hoverStyles\\" type=\\"button\\" aria-label=\\"Close popover\\" data-test-subj=\\"docTableClosePopover\\"><span data-euiicon-type=\\"cross\\" class=\\"euiButtonIcon__icon\\" aria-hidden=\\"true\\" color=\\"inherit\\"></span></button></div></div>"`
);
});

Expand All @@ -146,7 +146,7 @@ describe('Discover grid cell rendering', function () {
/>
);
expect(component.html()).toMatchInlineSnapshot(
`"<div class=\\"euiFlexGroup css-1h68cm-euiFlexGroup-none-flexStart-stretch-row\\"><div class=\\"euiFlexItem css-9sbomz-euiFlexItem-grow-1\\"><span class=\\"dscDiscoverGrid__cellPopoverValue eui-textBreakWord\\">100</span></div><div class=\\"euiFlexItem css-kpsrin-euiFlexItem-growZero\\"><button class=\\"euiButtonIcon euiButtonIcon--xSmall css-1q7ycil-euiButtonIcon-empty-primary-hoverStyles\\" type=\\"button\\" aria-label=\\"Close popover\\" data-test-subj=\\"docTableClosePopover\\"><span data-euiicon-type=\\"cross\\" class=\\"euiButtonIcon__icon\\" aria-hidden=\\"true\\" color=\\"inherit\\"></span></button></div></div>"`
`"<div class=\\"euiFlexGroup css-1h68cm-euiFlexGroup-none-flexStart-stretch-row\\"><div class=\\"euiFlexItem css-9sbomz-euiFlexItem-grow-1\\"><div class=\\"euiText css-f0maoj-euiText-m\\"><span class=\\"dscDiscoverGrid__cellPopoverValue eui-textBreakWord\\">100</span></div></div><div class=\\"euiFlexItem css-kpsrin-euiFlexItem-growZero\\"><button class=\\"euiButtonIcon euiButtonIcon--xSmall css-1q7ycil-euiButtonIcon-empty-primary-hoverStyles\\" type=\\"button\\" aria-label=\\"Close popover\\" data-test-subj=\\"docTableClosePopover\\"><span data-euiicon-type=\\"cross\\" class=\\"euiButtonIcon__icon\\" aria-hidden=\\"true\\" color=\\"inherit\\"></span></button></div></div>"`
);
findTestSubject(component, 'docTableClosePopover').simulate('click');
expect(closePopoverMockFn).toHaveBeenCalledTimes(1);
Expand Down Expand Up @@ -248,7 +248,7 @@ describe('Discover grid cell rendering', function () {
);
expect(component).toMatchInlineSnapshot(`
<EuiFlexGroup
className="dscDiscoverGrid__cellPopover"
className="dscDiscoverGrid__cellPopoverValueContainer"
direction="column"
gutterSize="none"
justifyContent="flexEnd"
Expand All @@ -275,9 +275,21 @@ describe('Discover grid cell rendering', function () {
</EuiFlexItem>
</EuiFlexGroup>
</EuiFlexItem>
<EuiFlexItem>
<EuiFlexItem
css={
Object {
"map": undefined,
"name": "1eiwchg",
"next": undefined,
"styles": "
min-width: 370;
",
"toString": [Function],
}
}
>
<JsonCodeEditor
height={200}
height={300}
json={
Object {
"_id": "1",
Expand All @@ -294,7 +306,6 @@ describe('Discover grid cell rendering', function () {
},
}
}
width={370}
/>
</EuiFlexItem>
</EuiFlexGroup>
Expand Down Expand Up @@ -480,7 +491,7 @@ describe('Discover grid cell rendering', function () {
);
expect(component).toMatchInlineSnapshot(`
<EuiFlexGroup
className="dscDiscoverGrid__cellPopover"
className="dscDiscoverGrid__cellPopoverValueContainer"
direction="column"
gutterSize="none"
justifyContent="flexEnd"
Expand All @@ -507,9 +518,21 @@ describe('Discover grid cell rendering', function () {
</EuiFlexItem>
</EuiFlexGroup>
</EuiFlexItem>
<EuiFlexItem>
<EuiFlexItem
css={
Object {
"map": undefined,
"name": "1eiwchg",
"next": undefined,
"styles": "
min-width: 370;
",
"toString": [Function],
}
}
>
<JsonCodeEditor
height={200}
height={300}
json={
Object {
"_id": "1",
Expand All @@ -531,7 +554,6 @@ describe('Discover grid cell rendering', function () {
},
}
}
width={370}
/>
</EuiFlexItem>
</EuiFlexGroup>
Expand Down Expand Up @@ -644,7 +666,7 @@ describe('Discover grid cell rendering', function () {
);
expect(component).toMatchInlineSnapshot(`
<EuiFlexGroup
className="dscDiscoverGrid__cellPopover"
className="dscDiscoverGrid__cellPopoverValueContainer"
direction="column"
gutterSize="none"
justifyContent="flexEnd"
Expand All @@ -671,17 +693,28 @@ describe('Discover grid cell rendering', function () {
</EuiFlexItem>
</EuiFlexGroup>
</EuiFlexItem>
<EuiFlexItem>
<EuiFlexItem
css={
Object {
"map": undefined,
"name": "1eiwchg",
"next": undefined,
"styles": "
min-width: 370;
",
"toString": [Function],
}
}
>
<JsonCodeEditor
height={200}
height={300}
json={
Object {
"object.value": Array [
100,
],
}
}
width={370}
/>
</EuiFlexItem>
</EuiFlexGroup>
Expand Down Expand Up @@ -865,16 +898,18 @@ describe('Discover grid cell rendering', function () {
responsive={false}
>
<EuiFlexItem>
<span
className="dscDiscoverGrid__cellPopoverValue eui-textBreakWord"
dangerouslySetInnerHTML={
Object {
"__html": Array [
".gz",
],
<EuiText>
<span
className="dscDiscoverGrid__cellPopoverValue eui-textBreakWord"
dangerouslySetInnerHTML={
Object {
"__html": Array [
".gz",
],
}
}
}
/>
/>
</EuiText>
</EuiFlexItem>
<EuiFlexItem
grow={false}
Expand Down
Loading