diff --git a/public/app/features/explore/Logs/Logs.test.tsx b/public/app/features/explore/Logs/Logs.test.tsx index 7e472a801b0..dd38faa5daf 100644 --- a/public/app/features/explore/Logs/Logs.test.tsx +++ b/public/app/features/explore/Logs/Logs.test.tsx @@ -365,12 +365,12 @@ describe('Logs', () => { const row = screen.getAllByRole('row'); await userEvent.hover(row[0]); - const linkButtons = row[1].querySelectorAll('button'); - await userEvent.click(linkButtons[2]); + const linkButton = screen.getByLabelText('Copy shortlink'); + await userEvent.click(linkButton); expect(reportInteraction).toHaveBeenCalledWith('grafana_explore_logs_permalink_clicked', { datasourceType: 'unknown', - logRowUid: '2', + logRowUid: '1', logRowLevel: 'debug', }); }); @@ -382,11 +382,11 @@ describe('Logs', () => { const row = screen.getAllByRole('row'); await userEvent.hover(row[0]); - const linkButtons = row[1].querySelectorAll('button'); - await userEvent.click(linkButtons[2]); + const linkButton = screen.getByLabelText('Copy shortlink'); + await userEvent.click(linkButton); expect(createAndCopyShortLink).toHaveBeenCalledWith( - 'http://localhost:3000/explore?left=%7B%22datasource%22:%22%22,%22queries%22:%5B%7B%22refId%22:%22A%22,%22expr%22:%22%22,%22queryType%22:%22range%22,%22datasource%22:%7B%22type%22:%22loki%22,%22uid%22:%22id%22%7D%7D%5D,%22range%22:%7B%22from%22:%222019-01-01T10:00:00.000Z%22,%22to%22:%222019-01-01T16:00:00.000Z%22%7D,%22panelsState%22:%7B%22logs%22:%7B%22id%22:%222%22%7D%7D%7D' + 'http://localhost:3000/explore?left=%7B%22datasource%22:%22%22,%22queries%22:%5B%7B%22refId%22:%22A%22,%22expr%22:%22%22,%22queryType%22:%22range%22,%22datasource%22:%7B%22type%22:%22loki%22,%22uid%22:%22id%22%7D%7D%5D,%22range%22:%7B%22from%22:%222019-01-01T10:00:00.000Z%22,%22to%22:%222019-01-01T16:00:00.000Z%22%7D,%22panelsState%22:%7B%22logs%22:%7B%22id%22:%221%22%7D%7D%7D' ); }); }); diff --git a/public/app/features/logs/components/LogRow.test.tsx b/public/app/features/logs/components/LogRow.test.tsx index dd5a2ac3dba..7956e7a7bea 100644 --- a/public/app/features/logs/components/LogRow.test.tsx +++ b/public/app/features/logs/components/LogRow.test.tsx @@ -110,4 +110,14 @@ describe('LogRow', () => { expect(scrollIntoView).not.toHaveBeenCalled(); }); }); + + it('should render the menu cell on mouse over', async () => { + setup({ showContextToggle: jest.fn().mockReturnValue(true) }); + + expect(screen.queryByLabelText('Show context')).not.toBeInTheDocument(); + + await userEvent.hover(screen.getByText('test123')); + + expect(screen.getByLabelText('Show context')).toBeInTheDocument(); + }); }); diff --git a/public/app/features/logs/components/LogRow.tsx b/public/app/features/logs/components/LogRow.tsx index 2872ed72753..c6a1ba86930 100644 --- a/public/app/features/logs/components/LogRow.tsx +++ b/public/app/features/logs/components/LogRow.tsx @@ -50,6 +50,7 @@ interface Props extends Themeable2 { interface State { highlightBackround: boolean; showDetails: boolean; + mouseIsOver: boolean; } /** @@ -63,6 +64,7 @@ class UnThemedLogRow extends PureComponent { state: State = { highlightBackround: false, showDetails: false, + mouseIsOver: false, }; logLineRef: React.RefObject; @@ -108,12 +110,14 @@ class UnThemedLogRow extends PureComponent { } onMouseEnter = () => { + this.setState({ mouseIsOver: true }); if (this.props.onLogRowHover) { this.props.onLogRowHover(this.props.row); } }; onMouseLeave = () => { + this.setState({ mouseIsOver: false }); if (this.props.onLogRowHover) { this.props.onLogRowHover(undefined); } @@ -249,6 +253,7 @@ class UnThemedLogRow extends PureComponent { onPinLine={this.props.onPinLine} onUnpinLine={this.props.onUnpinLine} pinned={this.props.pinned} + mouseIsOver={this.state.mouseIsOver} /> )} diff --git a/public/app/features/logs/components/LogRowMenuCell.tsx b/public/app/features/logs/components/LogRowMenuCell.tsx index 871b8b3e290..e54ea208d21 100644 --- a/public/app/features/logs/components/LogRowMenuCell.tsx +++ b/public/app/features/logs/components/LogRowMenuCell.tsx @@ -15,6 +15,7 @@ interface Props { onUnpinLine?: (row: LogRowModel) => void; pinned?: boolean; styles: LogRowStyles; + mouseIsOver: boolean; } export const LogRowMenuCell = React.memo( @@ -28,6 +29,7 @@ export const LogRowMenuCell = React.memo( row, showContextToggle, styles, + mouseIsOver, }: Props) => { const shouldShowContextToggle = showContextToggle ? showContextToggle(row) : false; const onLogRowClick = useCallback((e: SyntheticEvent) => { @@ -42,79 +44,77 @@ export const LogRowMenuCell = React.memo( ); const getLogText = useCallback(() => logText, [logText]); return ( - <> - {pinned && ( - // TODO: fix keyboard a11y - // eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-static-element-interactions - - onUnpinLine && onUnpinLine(row)} - tooltip="Unpin line" - tooltipPlacement="top" - aria-label="Unpin line" - /> - - )} - {/* TODO: fix keyboard a11y */} - {/* eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-static-element-interactions */} - - {shouldShowContextToggle && ( - - )} - + {pinned && !mouseIsOver && ( + onUnpinLine && onUnpinLine(row)} + tooltip="Unpin line" tooltipPlacement="top" + aria-label="Unpin line" /> - {pinned && onUnpinLine && ( - + {shouldShowContextToggle && ( + + )} + onUnpinLine && onUnpinLine(row)} - tooltip="Unpin line" + getText={getLogText} + tooltip="Copy to clipboard" tooltipPlacement="top" - aria-label="Unpin line" /> - )} - {!pinned && onPinLine && ( - onPinLine && onPinLine(row)} - tooltip="Pin line" - tooltipPlacement="top" - aria-label="Pin line" - /> - )} - {onPermalinkClick && row.uid && ( - onPermalinkClick(row)} - /> - )} - - + {pinned && onUnpinLine && ( + onUnpinLine && onUnpinLine(row)} + tooltip="Unpin line" + tooltipPlacement="top" + aria-label="Unpin line" + /> + )} + {!pinned && onPinLine && ( + onPinLine && onPinLine(row)} + tooltip="Pin line" + tooltipPlacement="top" + aria-label="Pin line" + /> + )} + {onPermalinkClick && row.uid && ( + onPermalinkClick(row)} + /> + )} + + )} + ); } ); diff --git a/public/app/features/logs/components/LogRowMessage.test.tsx b/public/app/features/logs/components/LogRowMessage.test.tsx index a7133bdc999..0b1b791d9fc 100644 --- a/public/app/features/logs/components/LogRowMessage.test.tsx +++ b/public/app/features/logs/components/LogRowMessage.test.tsx @@ -1,4 +1,4 @@ -import { render, screen } from '@testing-library/react'; +import { render, screen, fireEvent } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import React, { ComponentProps } from 'react'; @@ -18,6 +18,7 @@ const setup = (propOverrides?: Partial>, ro prettifyLogMessage: false, app: CoreApp.Explore, styles, + mouseIsOver: true, ...(propOverrides || {}), }; @@ -41,8 +42,9 @@ describe('LogRowMessage', () => { }); describe('with show context', () => { - it('should show context button', () => { + it('should show context button', async () => { setup({ showContextToggle: () => true }); + await userEvent.hover(screen.getByText('test123')); expect(screen.queryByLabelText('Show context')).toBeInTheDocument(); }); @@ -54,6 +56,7 @@ describe('LogRowMessage', () => { it('should call `onOpenContext` with row on click', async () => { const showContextToggle = jest.fn(); const props = setup({ showContextToggle: () => true, onOpenContext: showContextToggle }); + await userEvent.hover(screen.getByText('test123')); const button = screen.getByLabelText('Show context'); await userEvent.click(button); @@ -63,8 +66,9 @@ describe('LogRowMessage', () => { }); describe('with permalinking', () => { - it('should show permalinking button when `onPermalinkClick` is defined', () => { + it('should show permalinking button when `onPermalinkClick` is defined', async () => { setup({ onPermalinkClick: jest.fn() }); + await userEvent.hover(screen.getByText('test123')); expect(screen.queryByLabelText('Copy shortlink')).toBeInTheDocument(); }); @@ -76,6 +80,7 @@ describe('LogRowMessage', () => { it('should call `onPermalinkClick` with row on click', async () => { const permalinkClick = jest.fn(); const props = setup({ onPermalinkClick: permalinkClick }); + await userEvent.hover(screen.getByText('test123')); const button = screen.getByLabelText('Copy shortlink'); await userEvent.click(button); @@ -86,8 +91,9 @@ describe('LogRowMessage', () => { describe('with pinning', () => { describe('for `onPinLine`', () => { - it('should show pinning button when `onPinLine` is defined', () => { + it('should show pinning button when `onPinLine` is defined', async () => { setup({ onPinLine: jest.fn() }); + await userEvent.hover(screen.getByText('test123')); expect(screen.queryByLabelText('Pin line')).toBeInTheDocument(); }); @@ -104,6 +110,7 @@ describe('LogRowMessage', () => { it('should call `onPinLine` on click', async () => { const onPinLine = jest.fn(); setup({ onPinLine }); + await userEvent.hover(screen.getByText('test123')); const button = screen.getByLabelText('Pin line'); await userEvent.click(button); @@ -118,10 +125,10 @@ describe('LogRowMessage', () => { expect(screen.queryByLabelText('Unpin line')).not.toBeInTheDocument(); }); - it('should show 2 pinning buttons when `onUnpinLine` and `pinned` is defined', () => { - // we show 2 because we now have an "always visible" menu, and a "hover" menu + it('should show 1 pinning buttons when `onUnpinLine` and `pinned` is defined', () => { + // we show 1 because we now have an "always visible" menu, the others are rendered on mouse over setup({ onUnpinLine: jest.fn(), pinned: true }); - expect(screen.queryAllByLabelText('Unpin line').length).toBe(2); + expect(screen.queryAllByLabelText('Unpin line').length).toBe(1); }); it('should not show pinning button when `onUnpinLine` is not defined', () => { @@ -132,9 +139,10 @@ describe('LogRowMessage', () => { it('should call `onUnpinLine` on click', async () => { const onUnpinLine = jest.fn(); setup({ onUnpinLine, pinned: true }); - const button = screen.getAllByLabelText('Unpin line')[0]; + const button = screen.getByLabelText('Unpin line'); - await userEvent.click(button); + // There's an issue with userEvent and this button, so we use fireEvent instead + fireEvent.click(button); expect(onUnpinLine).toHaveBeenCalledTimes(1); }); diff --git a/public/app/features/logs/components/LogRowMessage.tsx b/public/app/features/logs/components/LogRowMessage.tsx index 206cd636968..e2a23b4e4d8 100644 --- a/public/app/features/logs/components/LogRowMessage.tsx +++ b/public/app/features/logs/components/LogRowMessage.tsx @@ -21,6 +21,7 @@ interface Props { onUnpinLine?: (row: LogRowModel) => void; pinned?: boolean; styles: LogRowStyles; + mouseIsOver: boolean; } interface LogMessageProps { @@ -73,9 +74,11 @@ export const LogRowMessage = React.memo((props: Props) => { onUnpinLine, onPinLine, pinned, + mouseIsOver, } = props; const { hasAnsi, raw } = row; const restructuredEntry = useMemo(() => restructureLog(raw, prettifyLogMessage), [raw, prettifyLogMessage]); + const shouldShowMenu = useMemo(() => mouseIsOver || pinned, [mouseIsOver, pinned]); return ( <> { @@ -90,17 +93,20 @@ export const LogRowMessage = React.memo((props: Props) => { - + {shouldShowMenu && ( + + )} ); diff --git a/public/app/features/logs/components/LogRowMessageDisplayedFields.tsx b/public/app/features/logs/components/LogRowMessageDisplayedFields.tsx index d8ed4f29ea4..6c74cd81445 100644 --- a/public/app/features/logs/components/LogRowMessageDisplayedFields.tsx +++ b/public/app/features/logs/components/LogRowMessageDisplayedFields.tsx @@ -1,5 +1,5 @@ import { css } from '@emotion/css'; -import React from 'react'; +import React, { useCallback, useMemo, useState } from 'react'; import { LogRowModel, Field, LinkModel, DataFrame } from '@grafana/data'; @@ -22,37 +22,52 @@ export interface Props { } export const LogRowMessageDisplayedFields = React.memo((props: Props) => { - const { row, detectedFields, getFieldLinks, wrapLogMessage, styles, ...rest } = props; + const [hover, setHover] = useState(false); + const { row, detectedFields, getFieldLinks, wrapLogMessage, styles, pinned, ...rest } = props; const fields = getAllFields(row, getFieldLinks); const wrapClassName = wrapLogMessage ? '' : displayedFieldsStyles.noWrap; // only single key/value rows are filterable, so we only need the first field key for filtering - const line = detectedFields - .map((parsedKey) => { - const field = fields.find((field) => { - const { keys } = field; - return keys[0] === parsedKey; - }); + const line = useMemo( + () => + detectedFields + .map((parsedKey) => { + const field = fields.find((field) => { + const { keys } = field; + return keys[0] === parsedKey; + }); - if (field !== undefined && field !== null) { - return `${parsedKey}=${field.values}`; - } + if (field !== undefined && field !== null) { + return `${parsedKey}=${field.values}`; + } - if (row.labels[parsedKey] !== undefined && row.labels[parsedKey] !== null) { - return `${parsedKey}=${row.labels[parsedKey]}`; - } + if (row.labels[parsedKey] !== undefined && row.labels[parsedKey] !== null) { + return `${parsedKey}=${row.labels[parsedKey]}`; + } - return null; - }) - .filter((s) => s !== null) - .join(' '); + return null; + }) + .filter((s) => s !== null) + .join(' '), + [detectedFields, fields, row.labels] + ); + + const showMenu = useCallback(() => { + setHover(true); + }, []); + const hideMenu = useCallback(() => { + setHover(false); + }, []); + const shouldShowMenu = useMemo(() => hover || pinned, [hover, pinned]); return ( <> - +
{line}
- - + + {shouldShowMenu && ( + + )} ); diff --git a/public/app/features/logs/components/getLogRowStyles.ts b/public/app/features/logs/components/getLogRowStyles.ts index c586473e257..ff1d29d4f13 100644 --- a/public/app/features/logs/components/getLogRowStyles.ts +++ b/public/app/features/logs/components/getLogRowStyles.ts @@ -85,14 +85,9 @@ export const getLogRowStyles = memoizeOne((theme: GrafanaTheme2) => { &:hover { .log-row-menu { - visibility: visible; z-index: 1; } - .log-row-menu-visible { - visibility: hidden; - } - background: ${hoverBgColor}; } diff --git a/public/app/features/logs/components/log-context/LogRowContextModal.test.tsx b/public/app/features/logs/components/log-context/LogRowContextModal.test.tsx index 74221c7c16d..f36f51b8ad3 100644 --- a/public/app/features/logs/components/log-context/LogRowContextModal.test.tsx +++ b/public/app/features/logs/components/log-context/LogRowContextModal.test.tsx @@ -1,4 +1,4 @@ -import { screen, waitFor } from '@testing-library/react'; +import { screen, waitFor, fireEvent } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import React from 'react'; import { render } from 'test/redux-rtl'; @@ -475,8 +475,10 @@ describe('LogRowContextModal', () => { expect(rows).toHaveStyle('position: sticky'); }); const unpinButtons = screen.getAllByLabelText('Unpin line')[0]; - await userEvent.click(unpinButtons); - const rows = screen.getByTestId('entry-row'); - expect(rows).not.toHaveStyle('position: sticky'); + fireEvent.click(unpinButtons); + await waitFor(() => { + const rows = screen.getByTestId('entry-row'); + expect(rows).not.toHaveStyle('position: sticky'); + }); }); });