[Security solution] Attack discovery tour #182605
Conversation
stephmilovic
added
release_note:skip
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
| <h4> | ||
| <EuiIcon type="cheer" color="success" /> {i18n.ATTACK_DISCOVERY_TOUR_VIDEO_STEP_TITLE} | ||
| </h4> |
There was a problem hiding this comment.
nit: No margin above title text to provide space between video element:
x-pack/plugins/security_solution/public/attack_discovery/tour/video_toast.tsx
Outdated
Show resolved
Hide resolved
| onFocus={handleOnMouseOver} | ||
| onBlur={handleOnMouseOut} | ||
| > | ||
| <iframe |
There was a problem hiding this comment.
Would be nice if we could set the background of the embedded page for dark mode. For the video itself it's transparent (and seems that way for the body/page), but it's coming through as white, so on dark mode we get these top and bottom white borders. Maybe we can change on the video site, but then would need one link for each theme (light/dark)....
Maybe we can use css/react to dig into the iframe and set the appropriate euiThemeColor?
CleanShot.2024-05-07.at.13.48.47.mp4
There was a problem hiding this comment.
going to hold on this til i hear from design if we can use a gif here instead
| referrerPolicy="no-referrer" | ||
| sandbox="allow-scripts allow-same-origin" | ||
| scrolling="no" | ||
| // since we cannot go fullscreen, we are autoplaying and removing the controls |
There was a problem hiding this comment.
minor nit: since we can't go fullscreen, can we disable the fullscreen button? It just toggles and doesn't do anything now once the other tab has been opened, so not a big deal.
There was a problem hiding this comment.
going to hold on this til i hear from design if we can use a gif here instead
| const VIDEO_CONTENT_HEIGHT = 160; | ||
| const VIDEO_CONTENT_WIDTH = 250; |
There was a problem hiding this comment.
Since we can't go fullscreen, and you can't really tell what is going on with the video at this size (though you can hear James' beautiful voice), I wonder if we should just show an image capture of the video instead of actually embedding the video and avoid all the useEffect logic.
Alternatively, the whole page should be blank for the user, so could either make the portal bigger or even just use the empty state of the page? That's more discussion with design and there's not much time left so I don't know.
As a user though, I don't find any utility in watching the video in this teeny tiny portal, so would probably just watch it in a new tab.
There was a problem hiding this comment.
I asked design and they don't want to make the toast bigger. it would certainly be easier to do the onClick on a gif vs the hacked iframe solution I have. let me see if that is cool with design
@bojanasan @codearos what do you think about using a gif here instead of the autoplay video?
There was a problem hiding this comment.
gif or image... either way, would design be willing to provide a file?
There was a problem hiding this comment.
Checked out, tested locally, and code changes LGTM!
Made some design nits, and noted a bug where un-muting triggers the 'open in a new tab' behavior, but overall if we're keen on keeping Step 2 of the tour the size it is, I think we should just show a screenshot of the video to avoid all these issues since we can't fullscreen at the moment.
Code looks good though, and I appreciate the tests + follow-up issue to disable this in 8.16, so I think we just need to check with design and see what they think. Thanks @stephmilovic!
|
@elasticmachine merge upstream |
|
Hi @stephmilovic, the implementation looks good. However, this approach won't work with the new side nav (here's our PR adapting to it #179971), it is still in "internal preview" stage so I guess it is not strictly necessary. But this won't work in the serverless nav either (it uses the same shared-ux component), actually, I tested that in serverless and I don't even see the 
|
|
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
miscellaneous assets size
History
To update your PR or re-run it, just comment with: |
(cherry picked from commit ef0bd2e)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
# Backport This will backport the following commits from `main` to `8.14`: - [[Security solution] Attack discovery tour (#182605)](#182605) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Steph Milovic","email":"stephanie.milovic@elastic.co"},"sourceCommit":{"committedDate":"2024-05-08T20:16:54Z","message":"[Security solution] Attack discovery tour (#182605)","sha":"ef0bd2e23146a3a8242be451540a7cad34c9b605","branchLabelMapping":{"^v8.15.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team: SecuritySolution","ci:cloud-deploy","Team:Security Generative AI","v8.14.0","v8.15.0","Feature:Attack Discovery"],"title":"[Security solution] Attack discovery tour","number":182605,"url":"#182605 solution] Attack discovery tour (#182605)","sha":"ef0bd2e23146a3a8242be451540a7cad34c9b605"}},"sourceBranch":"main","suggestedTargetBranches":["8.14"],"targetPullRequestStates":[{"branch":"8.14","label":"v8.14.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.15.0","branchLabelMappingKey":"^v8.15.0$","isSourceBranch":true,"state":"MERGED","url":"#182605 solution] Attack discovery tour (#182605)","sha":"ef0bd2e23146a3a8242be451540a7cad34c9b605"}}]}] BACKPORT--> Co-authored-by: Steph Milovic <stephanie.milovic@elastic.co>
Summary
ad.mov
The Attack discovery tour is a 2 step tour.
EuiTourStepanchored to the nav. The tour component needs an anchor to mount to. Therefore, the nav needs to mount before the tour step. I added anonMountmethod tosolution_side_nav.tsxand I wait for the nav to mount before mounting theAttackDiscoveryTourcomponent. When the user clicks "Try it", the page redirects to Attack discovery and step 2 is activated. When the user clicks "Close" step 2 is activated but the page does not redirect. This way, when the user navigates on their own to Attack discovery, the video toast is up and ready.EuiToastcontaining an overview video. Currently, we cannot make video full screen due to this core CSP issue. Therefore, we decided to open the video in a new tab to play. The video autoplays on mute. I added some fun event listeners to enable anonClickevent on the iframe. When the iframe is clicked, the video opens in a new tab. When the "Watch overview video" button is clicked, the video opens in a new tab. When the user closes the toast, the tour ends.Things to note
main. Here is the issue to remove the tour for 8.16 [Task] Remove Attack discovery tour in 8.16 #182855Testing
securitySolution.attackDiscovery.newFeaturesTour.v8.14Explore reviewer
Because the
EuiTourStepneeds the anchor to mount first, I added anonMountmethod tosolution_side_nav.tsx. I wait to mount the tour until the side nav has mounted. I also added anidto the nav item as this is required to use as an anchor withEuiTourStep