Use push flyout for Discover document flyout #166406
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| .dscGridFlyout { | ||
| .euiFlyoutBody__overflowContent { | ||
| padding: $euiSize; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,7 @@ import { | |
| keys, | ||
| EuiButtonEmpty, | ||
| useEuiTheme, | ||
| useIsWithinMinBreakpoint, | ||
| } from '@elastic/eui'; | ||
| import type { Filter, Query, AggregateQuery } from '@kbn/es-query'; | ||
| import type { DataTableRecord } from '@kbn/discover-utils/types'; | ||
|
|
@@ -37,6 +38,7 @@ import { isTextBasedQuery } from '../../application/main/utils/is_text_based_que | |
| import { useFlyoutActions } from './use_flyout_actions'; | ||
| import { useDiscoverCustomization } from '../../customizations'; | ||
| import { DiscoverGridFlyoutActions } from './discover_grid_flyout_actions'; | ||
| import './_discover_grid_flyout.scss'; | ||
|
|
||
| export interface DiscoverGridFlyoutProps { | ||
| savedSearchId?: string; | ||
|
|
@@ -82,8 +84,8 @@ export function DiscoverGridFlyout({ | |
| const services = useDiscoverServices(); | ||
| const flyoutCustomization = useDiscoverCustomization('flyout'); | ||
| const { euiTheme } = useEuiTheme(); | ||
| const isXlScreen = useIsWithinMinBreakpoint('xl'); | ||
| const defaultWidth = flyoutCustomization?.size ?? euiTheme.base * 34; // 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; | ||
|
|
@@ -230,6 +232,10 @@ export function DiscoverGridFlyout({ | |
| minWidth={minWidth} | ||
| maxWidth={maxWidth} | ||
| onResize={setFlyoutWidth} | ||
| css={{ | ||
| maxWidth: `${isXlScreen ? `calc(100vw - ${defaultWidth}px)` : '90vw'} !important`, | ||
| }} | ||
| className="dscGridFlyout" | ||
|
There was a problem hiding this comment. Rather than using a class with a new style, we should be able to instead use |
||
| > | ||
| <EuiFlyoutHeader hasBorder> | ||
| <EuiFlexGroup | ||
|
|
@@ -241,7 +247,7 @@ export function DiscoverGridFlyout({ | |
| > | ||
| <EuiFlexItem grow={false}> | ||
| <EuiTitle | ||
| size="xs" | ||
| data-test-subj="docTableRowDetailsTitle" | ||
| css={css` | ||
| white-space: nowrap; | ||
|
|
@@ -274,15 +280,11 @@ export function DiscoverGridFlyout({ | |
| </EuiFlyoutHeader> | ||
| <EuiFlyoutBody>{bodyContent}</EuiFlyoutBody> | ||
|
There was a problem hiding this comment. @lukasolson this comment is referring to the element being rendered in the let's bring this over the finishing line by adjusting the responsiveness of the action column of the 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 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 Thx for the feedback @andreadelrio and There was a problem hiding this comment. 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.
We could use a preset breakpoint, but it might even be better to just use |
||
| <EuiFlyoutFooter> | ||
|
There was a problem hiding this comment. From the EUI docs (https://elastic.github.io/eui/#/layout/flyout#push-versus-overlay):
|
||
| <EuiButtonEmpty iconType="cross" onClick={onClose} flush="left"> | ||
| {i18n.translate('discover.grid.flyout.close', { | ||
| defaultMessage: 'Close', | ||
| })} | ||
| </EuiButtonEmpty> | ||
| </EuiFlyoutFooter> | ||
| </EuiFlyoutResizable> | ||
| </EuiPortal> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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'); | ||
| }); | ||
|
|
||
| it('should not display a log level badge when not available', async () => { | ||
|
There was a problem hiding this comment. 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 There was a problem hiding this comment. 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'); | ||
| }); | ||
|
|
||
| it('should not display a message code block when not available', async () => { | ||
| await dataGrid.clickRowToggle({ rowIndex: 1, columnIndex: 4 }); | ||
| await testSubjects.missingOrFail('logsExplorerFlyoutLogMessage'); | ||
| }); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't realize this when I originally opened my touchups PR, but there's going to be an issue with using
defaultWidthhere since it could be a string value when overridden by a customization (e.g. Logs Explorer uses60%). I think we can just split outeuiTheme.base * 34into a separate const to use here and we should be safe.