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.

Sign up for GitHub

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

Jump to bottom

[Uptime] Expand synthetic journey step thumbnail on hover #89179

Merged
merged 19 commits into from
Feb 2, 2021
Merged

[Uptime] Expand synthetic journey step thumbnail on hover #89179

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
d6c5170
Add desired hover functionality and a test.
justinkambic Jan 25, 2021
0b2341f
Switch render from img to EuiImage for step view.
justinkambic Jan 25, 2021
3e44990
Merge branch 'master' into 80173-expand-thumbnail-on-hover
justinkambic Jan 26, 2021
0b4f4cd
Create new module for ping_timestamp. Extract a function. Add a test.
justinkambic Jan 26, 2021
872c75f
Extract nav buttons, translations. Add tests.
justinkambic Jan 26, 2021
bd595f7
Fix a typo.
justinkambic Jan 26, 2021
efbb775
Extract caption to own file. Add tests.
justinkambic Jan 26, 2021
519565e
Extract no image display to dedicated file. Add aria label. Add tests.
justinkambic Jan 26, 2021
c934044
Make import path more explicit.
justinkambic Jan 27, 2021
b518415
Move step image popover to dedicated file. Add tests.
justinkambic Jan 27, 2021
2808ff3
Merge branch 'master' into 80173-expand-thumbnail-on-hover
justinkambic Jan 27, 2021
1a14d65
Clean up inline code in timestamp component.
justinkambic Jan 27, 2021
d519c26
Explicit var names.
justinkambic Jan 27, 2021
73cf9ad
Simplicity.
justinkambic Jan 27, 2021
349db31
Fix refactoring issues in test files.
justinkambic Jan 27, 2021
0a52f97
Merge branch 'master' into 80173-expand-thumbnail-on-hover
justinkambic Jan 29, 2021
44c5bb3
Merge branch 'master' into 80173-expand-thumbnail-on-hover
justinkambic Feb 1, 2021
ad1ebbd
Move translations to central file.
justinkambic Feb 1, 2021
0dfd62d
Rename test for better accuracy.
justinkambic Feb 1, 2021
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
23 changes: 23 additions & 0 deletions x-pack/plugins/uptime/public/components/monitor/ping_list/columns/ping_timestamp.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/

import React from 'react';
import { fireEvent, waitFor } from '@testing-library/react';
import { PingTimestamp } from './ping_timestamp';
import { mockReduxHooks } from '../../../../lib/helper/test_helpers';
import { render } from '../../../../lib/helper/rtl_helpers';
Expand Down Expand Up @@ -92,4 +93,26 @@ describe('Ping Timestamp component', () => {
const { container } = render(<PingTimestamp ping={response} timestamp={response.timestamp} />);
expect(container.querySelector('img')?.src).toBe(src);
});

it('displays popover image when mouse enters img caption, and hides onLeave', async () => {
const src = 'http://sample.com/sampleImageSrc.png';
jest.spyOn(observabilityPublic, 'useFetcher').mockReturnValue({
status: FETCH_STATUS.SUCCESS,
data: { src },
refetch: () => null,
});
const { getByAltText, getByText, queryByAltText } = render(
<PingTimestamp ping={response} timestamp={response.timestamp} />
);
const caption = getByText('Nov 26, 2020 10:28:56 AM');
fireEvent.mouseEnter(caption);

const altText = `A full-size screenshot for this journey step's thumbnail.`;

await waitFor(() => getByAltText(altText));

fireEvent.mouseLeave(caption);

await waitFor(() => expect(queryByAltText(altText)).toBeNull());
});
});
60 changes: 45 additions & 15 deletions x-pack/plugins/uptime/public/components/monitor/ping_list/columns/ping_timestamp.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,13 @@ import {
EuiButtonIcon,
EuiFlexGroup,
EuiFlexItem,
EuiLoadingSpinner,
EuiImage,
EuiPopover,
EuiSpacer,
EuiText,
EuiLoadingSpinner,
} from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import useIntersection from 'react-use/lib/useIntersection';
import moment from 'moment';
import styled from 'styled-components';
Expand Down Expand Up @@ -60,13 +62,21 @@ const StepDiv = styled.div`
}
`;

const POPOVER_IMG_HEIGHT = 360;
const POPOVER_IMG_WIDTH = 640;

interface Props {
timestamp: string;
ping: Ping;
}

const nextAriaLabel = i18n.translate('xpack.uptime.synthetics.nextButton.ariaLabel', {
defaultMessage: 'Next',
});

export const PingTimestamp = ({ timestamp, ping }: Props) => {
const [stepNo, setStepNo] = useState(1);
const [isImagePopoverOpen, setIsImagePopoverOpen] = useState(false);

const [stepImages, setStepImages] = useState<string[]>([]);

Expand Down Expand Up @@ -113,7 +123,7 @@ export const PingTimestamp = ({ timestamp, ping }: Props) => {
setStepNo(stepNo - 1);
}}
iconType="arrowLeft"
aria-label="Next"
aria-label={nextAriaLabel}
/>
</EuiFlexItem>
<EuiFlexItem grow={false}>
Expand All @@ -127,7 +137,7 @@ export const PingTimestamp = ({ timestamp, ping }: Props) => {
setStepNo(stepNo + 1);
}}
iconType="arrowRight"
aria-label="Next"
aria-label={nextAriaLabel}
/>
</EuiFlexItem>
</EuiFlexGroup>
Expand All @@ -140,17 +150,36 @@ export const PingTimestamp = ({ timestamp, ping }: Props) => {
);

return (
<StepDiv ref={intersectionRef}>
<StepDiv
onMouseEnter={() => setIsImagePopoverOpen(true)}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This solution results in the popover displaying the larger image when the mouse enters the figcaption element that the image contains as well. I wasn't sure if this functionality would be an issue. I did explore a few avenues around this, but they weren't as simple as this. I also considered the notion that the caption should be semantically linked to the img itself as further justification.

I've included a GIF in the description illustrating the caption hover triggering the display.

onMouseLeave={() => setIsImagePopoverOpen(false)}
ref={intersectionRef}
>
{imgSrc ? (
<StepImage
allowFullScreen={true}
size="s"
hasShadow
caption={ImageCaption}
alt={captionContent}
url={imgSrc}
data-test-subj="pingTimestampImage"
/>
<EuiPopover
anchorPosition="rightCenter"
button={
<StepImage
allowFullScreen={true}
alt={captionContent}
caption={ImageCaption}
data-test-subj="pingTimestampImage"
hasShadow
url={imgSrc}
size="s"
/>
}
closePopover={() => setIsImagePopoverOpen(false)}
isOpen={isImagePopoverOpen}
>
<EuiImage
alt={i18n.translate('xpack.uptime.synthetics.thumbnail.fullSize.alt', {
defaultMessage: `A full-size screenshot for this journey step's thumbnail.`,
})}
url={imgSrc}
style={{ height: POPOVER_IMG_HEIGHT, width: POPOVER_IMG_WIDTH, objectFit: 'contain' }}
/>
</EuiPopover>
Copy link
Contributor

@shahzad31 shahzad31 Jan 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

seems like this ping_timestamp component has become bulky, i feel it deserver some asbtraction.

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure I can refactor. This code was somewhat new to me as it was contributed in my absence, so I didn't want to do any significant restructuring. I'm happy to try to break it up a bit though!

) : (
<EuiFlexGroup gutterSize="s" alignItems="center">
<EuiFlexItem>
Expand All @@ -167,6 +196,7 @@ export const PingTimestamp = ({ timestamp, ping }: Props) => {
className="stepArrows"
gutterSize="s"
alignItems="center"
onMouseEnter={() => setIsImagePopoverOpen(true)}
style={{ position: 'absolute', bottom: 0, left: 30 }}
>
<EuiFlexItem grow={false}>
Expand All @@ -178,7 +208,7 @@ export const PingTimestamp = ({ timestamp, ping }: Props) => {
setStepNo(stepNo - 1);
}}
iconType="arrowLeft"
aria-label="Next"
aria-label={nextAriaLabel}
/>
</EuiFlexItem>
<EuiFlexItem grow={false}>
Expand All @@ -190,7 +220,7 @@ export const PingTimestamp = ({ timestamp, ping }: Props) => {
setStepNo(stepNo + 1);
}}
iconType="arrowRight"
aria-label="Next"
aria-label={nextAriaLabel}
/>
</EuiFlexItem>
</EuiFlexGroup>
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/uptime/public/components/monitor/synthetics/step_screenshot_display.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ export const StepScreenshotDisplay: FC<StepScreenshotDisplayProps> = ({
closePopover={() => setIsImagePopoverOpen(false)}
isOpen={isImagePopoverOpen}
>
<img
<EuiImage
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

alt={
stepName
? i18n.translate('xpack.uptime.synthetics.screenshotDisplay.thumbnailAltText', {
Expand All @@ -114,7 +114,7 @@ export const StepScreenshotDisplay: FC<StepScreenshotDisplayProps> = ({
}
)
}
src={imgSrc}
url={imgSrc}
style={{ width: POPOVER_IMG_WIDTH, height: POPOVER_IMG_HEIGHT, objectFit: 'contain' }}
/>
</EuiPopover>
Expand Down