[Uptime] Expand synthetic journey step thumbnail on hover #89179
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
justinkambic
added
release_note:enhancement
v8.0.0
Team:Uptime - DEPRECATED
|
Pinging @elastic/uptime (Team:uptime) |
justinkambic
commented
Jan 25, 2021
| @@ -140,17 +150,36 @@ export const PingTimestamp = ({ timestamp, ping }: Props) => { | |||
| ); | |||
|
|
|||
| return ( | |||
| <StepDiv ref={intersectionRef}> | |||
| <StepDiv | |||
| onMouseEnter={() => setIsImagePopoverOpen(true)} | |||
There was a problem hiding this comment.
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.
shahzad31
previously requested changes
Jan 26, 2021
Comment on lines
159
to
182
| <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> |
There was a problem hiding this comment.
seems like this ping_timestamp component has become bulky, i feel it deserver some asbtraction.
WDYT?
There was a problem hiding this comment.
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!
|
@dominiqueclarke I think we should treat this as a completely new review given how much code I moved around and modified since Shahzad's review. |
There was a problem hiding this comment.
Looks great! Works in Chrome and Firefox (Kibana currently has a Safari bug that prevents you from loading Kibana in safari). Thanks for taking the time to break things out and clean up. A few comments. Feel free to let me know if you'd like to defer anything, since I realize this refactor primarily involved a lot of copying.
| export interface StepImageCaptionProps { | ||
| captionContent: string; | ||
| imgSrc?: string; | ||
| maxSteps?: number; |
There was a problem hiding this comment.
Seems like maxSteps should be required to prevent broken arrow buttons, since it controls the disabled state.
| return ( | ||
| <> | ||
| <div className="stepArrowsFullScreen"> | ||
| {imgSrc && ( |
There was a problem hiding this comment.
It doesn't seem like any of this data is reliant on imgSrc and it's odd to me that this is conditionally rendered based on imgSrc. Does imgSrc need to be prop for the caption?
There was a problem hiding this comment.
I was trying to avoid making any direct modifications to rendering logic as this was largely intended as a refactor, outside of the additional change to resolve #80173.
This is related to your above comment about maxSteps as well. All of this weirdness happens because useFetcher can return null | undefined for data. Perhaps we should require all props for these child components, and let a top-level data is truthy assertion dictate whether we even try to render any of them.
maxSteps, for example, appears to have a default value. So unless data itself is not returned, we should have values for these fields.
| import { StepImagePopover, StepImagePopoverProps } from './step_image_popover'; | ||
| import { render } from '../../../../../lib/helper/rtl_helpers'; | ||
|
|
||
| describe('StepImagePopover', () => { |
| expect(getByAltText(`A larger version of the screenshot for this journey step's thumbnail.`)); | ||
| }); | ||
|
|
||
| it('renders caption content', () => { |
There was a problem hiding this comment.
Is this testing the caption content or something else?
There was a problem hiding this comment.
We declare captionContent in the default props; the test is making sure the caption content and img src match in the DOM. I can rename it if you have a better description in mind.
| }; | ||
| }); | ||
|
|
||
| it('opens popover when click on img caption, and hides when popover is closed', async () => { |
There was a problem hiding this comment.
nit: Not sure if this is the popover, but rather the full screen image.
There was a problem hiding this comment.
The result of naming a test before verifying how the implementation should go. Nice catch: 0dfd62d
| isOpen={isImagePopoverOpen} | ||
| > | ||
| <EuiImage | ||
| alt={i18n.translate('xpack.uptime.synthetics.thumbnail.fullSize.alt', { |
There was a problem hiding this comment.
This is really not something to address at the moment, but wondering if we should make constant files for content that we can then import in the test? Plus side is it keeps everything consistent and prevents test changes with content changes. Downside is now the content implementation is separated from the render so you have to work in two different files for full context.
There was a problem hiding this comment.
I think I have actually done this a few times already on my own. Perhaps worth discussing at the next tech sync.
| * you may not use this file except in compliance with the Elastic License. | ||
| */ | ||
|
|
||
| import { i18n } from '@kbn/i18n'; |
There was a problem hiding this comment.
Some translations are here and not others.
There was a problem hiding this comment.
I've moved an inline translation to this file, and created a format function for another. I decided to keep the one in NoImageAvailable there because I feel the usage of FormattedMessage improves readability, vs. importing. ad1ebbd
| @@ -98,7 +98,7 @@ export const StepScreenshotDisplay: FC<StepScreenshotDisplayProps> = ({ | |||
| closePopover={() => setIsImagePopoverOpen(false)} | |||
| isOpen={isImagePopoverOpen} | |||
| > | |||
| <img | |||
| <EuiImage | |||
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / X-Pack Reporting API Integration Tests.x-pack/test/reporting_api_integration/reporting_and_security/csv_job_params·ts.Reporting APIs Generation from Job Params "before all" hook for "Rejects bogus jobParams"Standard OutStack TraceMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
dominiqueclarke
approved these changes
Feb 1, 2021
justinkambic
dismissed
shahzad31’s stale review
He's on leave, he told me he was ok with the changes, and another team member reviewed.
justinkambic
added a commit
to justinkambic/kibana
that referenced
this pull request
Feb 2, 2021
) * Add desired hover functionality and a test. * Switch render from img to EuiImage for step view. * Create new module for ping_timestamp. Extract a function. Add a test. * Extract nav buttons, translations. Add tests. * Fix a typo. * Extract caption to own file. Add tests. * Extract no image display to dedicated file. Add aria label. Add tests. * Make import path more explicit. * Move step image popover to dedicated file. Add tests. * Clean up inline code in timestamp component. * Explicit var names. * Simplicity. * Fix refactoring issues in test files. * Move translations to central file. * Rename test for better accuracy.
gmmorris
added a commit
to lizozom/kibana
that referenced
this pull request
Feb 2, 2021
…om/kibana into pr/89570 * 'sessions/save-all-sessions' of https://github.com/lizozom/kibana: (44 commits) [ML] Functional tests - skip DFA clone tests [Uptime] Fix synthetics detail step count (elastic#89940) Fixes the permissions to require cluster.manage in order to create an index and in order to update an index (elastic#89947) [Security Solution] [Detections] adds log info level for logging in cloud (elastic#89941) [Time to Visualize] Dashboard By Value Testing Lens (elastic#89581) [Uptime] Expand synthetic journey step thumbnail on hover (elastic#89179) TS project refs: Migrates snapshot_restore to a TS Project (elastic#89653) docs: APM 7.11 updates (elastic#89789) move skip to higher level (elastic#86952) Revert "Migrations v2: don't auto-create indices + FTR/esArchiver support (elastic#85778)" Revert "Revert "Enable v2 so migrations, disable in FTR tests (elastic#89297)"" Revert "Enable v2 so migrations, disable in FTR tests (elastic#89297)" [data.search] Allow search response to follow new hits format (elastic#88115) [Maps] Change 'create multi-layer map' title to be use-case focused (elastic#89520) skip flaky suite (elastic#86952) [Security Solution] Remove focustrap (elastic#89905) [Workplace Search] Add remaining i18n support for the Content Sources tree (elastic#89910) [esArchiver] log when migrations complete and we're done loading data (elastic#89938) Add --ssl flag to make resolver generator use ssl with kbn and elasticsearch clients (elastic#89873) TS project refs: Migrates grokdebugger (elastic#89652) ...
justinkambic
added a commit
that referenced
this pull request
Feb 2, 2021
…89970) * Add desired hover functionality and a test. * Switch render from img to EuiImage for step view. * Create new module for ping_timestamp. Extract a function. Add a test. * Extract nav buttons, translations. Add tests. * Fix a typo. * Extract caption to own file. Add tests. * Extract no image display to dedicated file. Add aria label. Add tests. * Make import path more explicit. * Move step image popover to dedicated file. Add tests. * Clean up inline code in timestamp component. * Explicit var names. * Simplicity. * Fix refactoring issues in test files. * Move translations to central file. * Rename test for better accuracy.
|
Backported to: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Resolves #80173.
This PR attempts to address the desired enhancement linked above. We now display a larger version of the thumbnail when the mouse enters the thumbnail or its caption.
Checklist
Delete any items that are not applicable to this PR.
For maintainers