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

Use push flyout for Discover document flyout #166406

Merged
merged 51 commits into from
Apr 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
51 commits
Select commit Hold shift + click to select a range
cadd8d8
Use push flyout for Discover document flyout
lukasolson Sep 13, 2023
23dc630
Merge branch 'main' into unified-doc-viewer/push-flyout
lukasolson Sep 25, 2023
58cc26e
Merge branch 'main' into unified-doc-viewer/push-flyout
lukasolson Sep 26, 2023
b164831
Merge branch 'main' into unified-doc-viewer/push-flyout
lukasolson Sep 27, 2023
cea1288
Add export
lukasolson Sep 27, 2023
deaa9ee
Merge branch 'unified-doc-viewer/push-flyout' of github.com:lukasolso…
lukasolson Sep 27, 2023
70d7127
Merge branch 'main' into unified-doc-viewer/push-flyout
kibanamachine Sep 29, 2023
083921c
Merge branch 'main' into unified-doc-viewer/push-flyout
lukasolson Jan 12, 2024
54d24b6
Use resizable flyout
lukasolson Jan 12, 2024
a9072e4
Merge branch 'unified-doc-viewer/push-flyout' of github.com:lukasolso…
lukasolson Jan 12, 2024
b583a9d
Merge branch 'main' into unified-doc-viewer/push-flyout
lukasolson Jan 12, 2024
ab29999
Merge branch 'main' into unified-doc-viewer/push-flyout
lukasolson Jan 16, 2024
8e70fb3
Add i18n
lukasolson Jan 16, 2024
0b97736
Add min/max & use ownFocus=true
lukasolson Jan 17, 2024
3b4cb11
Merge branch 'unified-doc-viewer/push-flyout' of github.com:lukasolso…
lukasolson Jan 17, 2024
28483cd
Merge branch 'main' into unified-doc-viewer/push-flyout
lukasolson Jan 17, 2024
5e9b816
Merge branch 'main' into unified-doc-viewer/push-flyout
lukasolson Jan 23, 2024
e8028d9
Text & size changes
lukasolson Jan 23, 2024
2d2eb32
Merge branch 'unified-doc-viewer/push-flyout' of github.com:lukasolso…
lukasolson Jan 23, 2024
ddf3694
Merge branch 'main' into unified-doc-viewer/push-flyout
stratoula Jan 26, 2024
dfebc40
Undo text changes
lukasolson Jan 30, 2024
9942d54
Merge branch 'unified-doc-viewer/push-flyout' of github.com:lukasolso…
lukasolson Jan 30, 2024
6ca57ec
Merge branch 'main' into unified-doc-viewer/push-flyout
lukasolson Jan 30, 2024
a83c461
Merge branch 'main' of github.com:elastic/kibana into unified-doc-vie…
lukasolson Jan 31, 2024
46d4424
Persist flyout width on resize
lukasolson Jan 31, 2024
8c1bdec
Merge branch 'main' into unified-doc-viewer/push-flyout
lukasolson Apr 3, 2024
865fed9
Merge branch 'main' into unified-doc-viewer/push-flyout
lukasolson Apr 5, 2024
785c75f
Responsive table/flyout actions
lukasolson Apr 5, 2024
a5d409a
Merge branch 'main' into unified-doc-viewer/push-flyout
lukasolson Apr 8, 2024
8b3d9f8
Use breakpoints for mobile screen detection
lukasolson Apr 8, 2024
1de9a94
Add some minor touchups around the Unified Doc Viewer push flyout
davismcphee Apr 10, 2024
da63a18
Merge pull request #20 from davismcphee/unified-doc-viewer-push-flyou…
lukasolson Apr 10, 2024
f488e70
Merge branch 'main' into unified-doc-viewer/push-flyout
lukasolson Apr 10, 2024
816fafc
Merge branch 'unified-doc-viewer/push-flyout' of github.com:lukasolso…
lukasolson Apr 10, 2024
0156206
Review feedback
lukasolson Apr 10, 2024
8ab3fd7
Merge branch 'main' into unified-doc-viewer/push-flyout
lukasolson Apr 11, 2024
b268e75
Design updates
lukasolson Apr 11, 2024
88c7b05
Merge branch 'main' into unified-doc-viewer/push-flyout
lukasolson Apr 12, 2024
ec72196
Update tests
lukasolson Apr 15, 2024
e1f6667
Merge branch 'main' into unified-doc-viewer/push-flyout
lukasolson Apr 15, 2024
8e362f3
Fix functional test
lukasolson Apr 15, 2024
42d6612
Remove .only
lukasolson Apr 15, 2024
4e995f7
Merge branch 'main' into unified-doc-viewer/push-flyout
lukasolson Apr 16, 2024
8cfa359
Break tests into separate cases
lukasolson Apr 16, 2024
40be3e2
Merge branch 'main' into unified-doc-viewer/push-flyout
lukasolson Apr 22, 2024
8866baf
Merge branch 'main' into unified-doc-viewer/push-flyout
lukasolson Apr 22, 2024
f86a39d
Remove closing flyout
lukasolson Apr 22, 2024
9abcf27
Merge branch 'unified-doc-viewer/push-flyout' of github.com:lukasolso…
lukasolson Apr 22, 2024
8d69a43
Remove close flyout
lukasolson Apr 23, 2024
6f462be
Merge branch 'main' into unified-doc-viewer/push-flyout
lukasolson Apr 24, 2024
f48a840
Review feedback
lukasolson Apr 24, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ export const DiscoverResizableLayout = ({
const { euiTheme } = useEuiTheme();
const minSidebarWidth = euiTheme.base * 13;
const defaultSidebarWidth = euiTheme.base * 19;
const minMainPanelWidth = euiTheme.base * 30;
const minMainPanelWidth = euiTheme.base * 24;

const [sidebarWidth, setSidebarWidth] = useLocalStorage(SIDEBAR_WIDTH_KEY, defaultSidebarWidth);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ jest.mock('@elastic/eui', () => {

return original.useIsWithinBreakpoints(breakpoints);
}),
useResizeObserver: jest.fn(() => ({ width: 1000, height: 1000 })),
};
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,20 +14,25 @@ import type { DataView } from '@kbn/data-views-plugin/public';
import {
EuiFlexGroup,
EuiFlexItem,
EuiFlyout,
EuiFlyoutResizable,
EuiFlyoutBody,
EuiFlyoutFooter,
EuiFlyoutHeader,
EuiTitle,
EuiSpacer,
EuiPortal,
EuiPagination,
keys,
EuiButtonEmpty,
useEuiTheme,
useIsWithinMinBreakpoint,
} from '@elastic/eui';
import type { Filter, Query, AggregateQuery } from '@kbn/es-query';
import type { DataTableRecord } from '@kbn/discover-utils/types';
import type { DocViewFilterFn } from '@kbn/unified-doc-viewer/types';
import type { DataTableColumnsMeta } from '@kbn/unified-data-table';
import { UnifiedDocViewer } from '@kbn/unified-doc-viewer-plugin/public';
import useLocalStorage from 'react-use/lib/useLocalStorage';
import { useDiscoverServices } from '../../hooks/use_discover_services';
import { isTextBasedQuery } from '../../application/main/utils/is_text_based_query';
import { useFlyoutActions } from './use_flyout_actions';
Expand Down Expand Up @@ -55,6 +60,8 @@ function getIndexByDocId(hits: DataTableRecord[], id: string) {
return h.id === id;
});
}

export const FLYOUT_WIDTH_KEY = 'discover:flyoutWidth';
/**
* Flyout displaying an expanded Elasticsearch document
*/
Expand All @@ -75,6 +82,13 @@ export function DiscoverGridFlyout({
}: DiscoverGridFlyoutProps) {
const services = useDiscoverServices();
const flyoutCustomization = useDiscoverCustomization('flyout');
const { euiTheme } = useEuiTheme();
const isXlScreen = useIsWithinMinBreakpoint('xl');
const DEFAULT_WIDTH = euiTheme.base * 34;
const defaultWidth = flyoutCustomization?.size ?? DEFAULT_WIDTH; // Give enough room to search bar to not wrap
const [flyoutWidth, setFlyoutWidth] = useLocalStorage(FLYOUT_WIDTH_KEY, defaultWidth);
const minWidth = euiTheme.base * 24;
const maxWidth = euiTheme.breakpoint.xl;

const isPlainRecord = isTextBasedQuery(query);
// Get actual hit with updated highlighted searches
Expand Down Expand Up @@ -204,16 +218,24 @@ export function DiscoverGridFlyout({
defaultMessage: 'Document',
});
const flyoutTitle = flyoutCustomization?.title ?? defaultFlyoutTitle;
const flyoutSize = flyoutCustomization?.size ?? 'm';

return (
<EuiPortal>
<EuiFlyout
<EuiFlyoutResizable
onClose={onClose}
size={flyoutSize}
type="push"
size={flyoutWidth}
pushMinBreakpoint="xl"
data-test-subj="docTableDetailsFlyout"
onKeyDown={onKeyDown}
ownFocus={false}
ownFocus={true}
minWidth={minWidth}
maxWidth={maxWidth}
onResize={setFlyoutWidth}
css={{
maxWidth: `${isXlScreen ? `calc(100vw - ${DEFAULT_WIDTH}px)` : '90vw'} !important`,
}}
paddingSize="m"
>
<EuiFlyoutHeader hasBorder>
<EuiFlexGroup
Expand All @@ -225,7 +247,7 @@ export function DiscoverGridFlyout({
>
<EuiFlexItem grow={false}>
<EuiTitle
size="s"
size="xs"
data-test-subj="docTableRowDetailsTitle"
css={css`
white-space: nowrap;
Expand Down Expand Up @@ -257,7 +279,14 @@ export function DiscoverGridFlyout({
)}
</EuiFlyoutHeader>
<EuiFlyoutBody>{bodyContent}</EuiFlyoutBody>
Copy link
Member

@kertal kertal Apr 3, 2024

Choose a reason for hiding this comment

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

@lukasolson this comment is referring to the element being rendered in the EuiFlyoutBody

let's bring this over the finishing line by adjusting the responsiveness of the action column of the DocViewerTable. Something like this should work with a change at the beginning of the DocViewerTable component.

export const DocViewerTable = ({
columns,
columnsMeta,
hit,
dataView,
hideActionsColumn,
filter,
onAddColumn,
onRemoveColumn,
}: DocViewRenderProps) => {
const showActionsInsideTableCell = useIsWithinBreakpoints(['xl'], true);
const { fieldFormats, storage, uiSettings } = getUnifiedDocViewerServices();
const showMultiFields = uiSettings.get(SHOW_MULTIFIELDS);
const currentDataViewId = dataView.id!;

  const { euiTheme } = useEuiTheme();
  const [ref, setRef] = useState<HTMLDivElement | HTMLSpanElement | null>(null);
  const dimensions = useResizeObserver(ref);
  const showActionsInsideTableCell = dimensions?.width
    ? dimensions.width > euiTheme.breakpoint.m
    : false;

With setRef being used at the EuiFlexGroup wrapper

Then the action column would no longer be responsive to the window width, but to the flyout width, which is a better pattern anyway. Should we use euiTheme.breakpoint.m or euiTheme.breakpoint.l? Both look good to me.

Thx for the feedback @andreadelrio and useResizeObserver hint @davismcphee

Copy link
Contributor

Choose a reason for hiding this comment

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

One other note: we should also make the single document and surrounding documents buttons responsive based on the flyout width since they currently use the window width too. We should be able to use the same resize observer approach for both the buttons and table actions.

Should we use euiTheme.breakpoint.m or euiTheme.breakpoint.l?

We could use a preset breakpoint, but it might even be better to just use euiTheme.base * X and get a value close to when the UI elements start to "break".

</EuiFlyout>
<EuiFlyoutFooter>
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

<EuiButtonEmpty iconType="cross" onClick={onClose} flush="left">
{i18n.translate('discover.grid.flyout.close', {
defaultMessage: 'Close',
})}
</EuiButtonEmpty>
</EuiFlyoutFooter>
</EuiFlyoutResizable>
</EuiPortal>
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,14 @@ import {
EuiContextMenuPanel,
EuiContextMenuItem,
EuiContextMenuItemIcon,
useIsWithinBreakpoints,
EuiText,
EuiButtonEmpty,
EuiButtonIcon,
EuiPopoverProps,
EuiToolTip,
useEuiTheme,
useResizeObserver,
useIsWithinBreakpoints,
} from '@elastic/eui';
import type { FlyoutActionItem } from '../../customizations';

Expand All @@ -34,11 +35,34 @@ export interface DiscoverGridFlyoutActionsProps {
}

export function DiscoverGridFlyoutActions({ flyoutActions }: DiscoverGridFlyoutActionsProps) {
const { euiTheme } = useEuiTheme();
const [ref, setRef] = useState<HTMLDivElement | HTMLSpanElement | null>(null);
const dimensions = useResizeObserver(ref);
const isMobileScreen = useIsWithinBreakpoints(['xs', 's']);
const isLargeFlyout = dimensions?.width ? dimensions.width > euiTheme.base * 30 : false;
return (
<div ref={setRef}>
<FlyoutActions
flyoutActions={flyoutActions}
isMobileScreen={isMobileScreen}
isLargeFlyout={isLargeFlyout}
/>
</div>
);
}

function FlyoutActions({
flyoutActions,
isMobileScreen,
isLargeFlyout,
}: {
flyoutActions: DiscoverGridFlyoutActionsProps['flyoutActions'];
isMobileScreen: boolean;
isLargeFlyout: boolean;
}) {
const { euiTheme } = useEuiTheme();
const [isMoreFlyoutActionsPopoverOpen, setIsMoreFlyoutActionsPopoverOpen] =
useState<boolean>(false);
const isMobileScreen = useIsWithinBreakpoints(['xs', 's']);
const isLargeScreen = useIsWithinBreakpoints(['xl']);

if (isMobileScreen) {
return (
Expand Down Expand Up @@ -72,7 +96,7 @@ export function DiscoverGridFlyoutActions({ flyoutActions }: DiscoverGridFlyoutA
flyoutActions.length
);
const showFlyoutIconsOnly =
remainingFlyoutActions.length > 0 || (!isLargeScreen && visibleFlyoutActions.length > 1);
remainingFlyoutActions.length > 0 || (!isLargeFlyout && visibleFlyoutActions.length > 1);

return (
<EuiFlexGroup
Expand All @@ -91,12 +115,10 @@ export function DiscoverGridFlyoutActions({ flyoutActions }: DiscoverGridFlyoutA
>
<EuiFlexItem grow={false}>
<EuiText size="s">
<strong>
{i18n.translate('discover.grid.tableRow.actionsLabel', {
defaultMessage: 'Actions',
})}
:
</strong>
{i18n.translate('discover.grid.tableRow.actionsLabel', {
defaultMessage: 'Actions',
})}
:
</EuiText>
</EuiFlexItem>
{visibleFlyoutActions.map((action) => (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ import {
EuiTablePagination,
EuiSelectableMessage,
EuiI18n,
useIsWithinBreakpoints,
useEuiTheme,
useResizeObserver,
} from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import { FormattedMessage } from '@kbn/i18n-react';
Expand Down Expand Up @@ -118,7 +119,12 @@ export const DocViewerTable = ({
onAddColumn,
onRemoveColumn,
}: DocViewRenderProps) => {
const showActionsInsideTableCell = useIsWithinBreakpoints(['xl'], true);
const { euiTheme } = useEuiTheme();
const [ref, setRef] = useState<HTMLDivElement | HTMLSpanElement | null>(null);
const dimensions = useResizeObserver(ref);
const showActionsInsideTableCell = dimensions?.width
? dimensions.width > euiTheme.breakpoint.m
: false;

const { fieldFormats, storage, uiSettings } = getUnifiedDocViewerServices();
const showMultiFields = uiSettings.get(SHOW_MULTIFIELDS);
Expand Down Expand Up @@ -407,7 +413,7 @@ export const DocViewerTable = ({
];

return (
<EuiFlexGroup direction="column" gutterSize="none" responsive={false}>
<EuiFlexGroup direction="column" gutterSize="none" responsive={false} ref={setRef}>
<EuiFlexItem grow={false}>
<EuiSpacer size="s" />
</EuiFlexItem>
Expand Down
5 changes: 4 additions & 1 deletion test/accessibility/apps/discover.ts
Original file line number Diff line number Diff line change
Expand Up @@ -133,11 +133,14 @@ 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
} else {
await testSubjects.existOrFail('fieldActionsGroup-Cancelled');
}
await a11y.testAppSnapshot();
if (await testSubjects.exists('openFieldActionsButton-Cancelled')) {
await testSubjects.click('openFieldActionsButton-Cancelled'); // Close the actions
}
});

it('a11y test for data-grid table with columns', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,17 +78,19 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
it('should display a log level badge when available', async () => {
await dataGrid.clickRowToggle({ columnIndex: 4 });
await testSubjects.existOrFail('logsExplorerFlyoutLogLevel');
await dataGrid.closeFlyout();
});

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

await dataGrid.clickRowToggle({ rowIndex: 1, columnIndex: 4 });
await testSubjects.missingOrFail('logsExplorerFlyoutLogLevel');
});

it('should display a message code block when available', async () => {
await dataGrid.clickRowToggle({ columnIndex: 4 });
await testSubjects.existOrFail('logsExplorerFlyoutLogMessage');
await dataGrid.closeFlyout();
});

it('should not display a message code block when not available', async () => {
await dataGrid.clickRowToggle({ rowIndex: 1, columnIndex: 4 });
await testSubjects.missingOrFail('logsExplorerFlyoutLogMessage');
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,17 +80,19 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
it('should display a log level badge when available', async () => {
await dataGrid.clickRowToggle({ columnIndex: 4 });
await testSubjects.existOrFail('logsExplorerFlyoutLogLevel');
await dataGrid.closeFlyout();
});
Copy link
Member

Choose a reason for hiding this comment

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

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


it('should not display a log level badge when not available', async () => {
await dataGrid.clickRowToggle({ rowIndex: 1, columnIndex: 4 });
await testSubjects.missingOrFail('logsExplorerFlyoutLogLevel');
});

it('should display a message code block when available', async () => {
await dataGrid.clickRowToggle({ columnIndex: 4 });
await testSubjects.existOrFail('logsExplorerFlyoutLogMessage');
await dataGrid.closeFlyout();
});

it('should not display a message code block when not available', async () => {
await dataGrid.clickRowToggle({ rowIndex: 1, columnIndex: 4 });
await testSubjects.missingOrFail('logsExplorerFlyoutLogMessage');
});
Expand Down
Loading