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.

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

[Security Solution][DQD] Add historical results tour guide #196127

Conversation

kapral18
Copy link
Contributor

@kapral18 kapral18 commented Oct 14, 2024

addresses #195971

This PR adds missing new historical results feature tour guide.

Tour guide features:

  • ability to maintain visual presence while collapsing accordions in list-view
  • move from list-view to flyout view and back
  • seamlessly integrates with existing opening flyout and history tab functionality

PR decisions with explanation:

  • data-tour-element has been introduced on select elements (like first actions of each first row) to avoid polluting every single element with data-test-subj. This way it's imho specific and semantically more clear what the elements are for.
  • early on I tried to control the anchoring with refs but some eui elements don't allow passing refs like EuiTab, so instead a more simpler and straightforward approach with dom selectors has been chosen
  • localStorage key name has been picked in accordance with other instances of usage securitySolution.dataQualityDashboard.historicalResultsTour.v8.16.isActive the name includes the full domain + the version when it's introduced. And since this tour step is a single step there is no need to stringify an object with isTourActive in and it's much simpler to just bake the activity state into the name and make the value just a boolean.

UI Demo

Anchor reposition demo (listview + flyout)

anchor_reposition.mov

List view tour guide try it + reload demo

list_view_try_it_reload.mov

FlyOut Try It + reload demo

flyout_try_it_reload.mov

Manual history tab selection path + reload demo

select_history_tab_reload.mov

Manual open history view path + reload demo

open_history_view_reload.mov

Dismiss list view tour guide + reload demo

dismiss_list_view_reload.mov

Dismiss FlyOut tour guide + reload demo

dismiss_from_flyout_reload.mov

Serverless empty pattern handling + reposition demo

Screen.Recording.2024-10-15.at.15.00.31.mov

@kapral18 kapral18 self-assigned this Oct 14, 2024
@kapral18 kapral18 requested a review from a team as a code owner October 14, 2024 12:33
@kapral18 kapral18 added v9.0.0 Team:Threat Hunting Security Solution Threat Hunting Team Team:Threat Hunting:Explore backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) v8.16.0 labels Oct 14, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-threat-hunting-explore (Team:Threat Hunting:Explore)

@kapral18 kapral18 added release_note:skip Skip the PR/issue when compiling release notes release_note:feature Makes this part of the condensed release notes and removed release_note:skip Skip the PR/issue when compiling release notes labels Oct 14, 2024
Copy link
Contributor

@angorayc angorayc left a comment

Choose a reason for hiding this comment

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

Hey @kapral18 , thanks for adding the tour guide!

Found some cases that we might want to temporarily hide the tour guide:

Screen.Recording.2024-10-14.at.17.02.03.mov
Screenshot 2024-10-14 at 17 02 48

@kapral18
Copy link
Contributor Author

kapral18 commented Oct 14, 2024

Hey @kapral18 , thanks for adding the tour guide!

Found some cases that we might want to temporarily hide the tour guide:

Screen.Recording.2024-10-14.at.17.02.03.mov
Screenshot 2024-10-14 at 17 02 48

Thanks for checking.

Also turns out this bug is happening for others as well (ex. attack discovery). And rightfully so because turns out that both left navigation and our timeline have incorrect z-indices according to https://eui.elastic.co/#/theming/more-tokens%23levels and there is no relationship between parent and child z-indices. Eui is fixing the former elastic/eui#8075. We should fix the latter, even tho it's not trivial I understand because other teams should allow ability to control z-index from their interface.

For now I will try to add to injury with my own z-index hack that is below timeline, but we need to be aware that this is not a how it's supposed to be. cc: @semd

@kapral18 kapral18 added backport:skip This commit does not require backporting backport:version Backport to applied version labels and removed backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) backport:skip This commit does not require backporting labels Oct 14, 2024
@kapral18 kapral18 force-pushed the feat/DQD/185882-add-historical-results-tour-guide branch from f74da26 to e92a06f Compare October 14, 2024 23:38
@kapral18 kapral18 force-pushed the feat/DQD/185882-add-historical-results-tour-guide branch 2 times, most recently from faf0d7d to 78906bd Compare October 15, 2024 13:41
@kapral18
Copy link
Contributor Author

@angorayc @semd I fixed the z-index with a temp solution. Please sees screenshots from both serverless and ess.

SCR-20241015-nwjc
SCR-20241015-nwkf
SCR-20241015-nvam
SCR-20241015-nvbv

@kapral18 kapral18 requested review from angorayc and semd October 15, 2024 13:43
@angorayc
Copy link
Contributor

@kapral18 ,
I found the tour is cropped when opening the flyout. Could you please try if it is reproducible?

Screen.Recording.2024-10-15.at.15.21.41.mov

Copy link
Contributor

@semd semd left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for taking the time to address the comments.

@kapral18 kapral18 force-pushed the feat/DQD/185882-add-historical-results-tour-guide branch from 78906bd to 7744ee9 Compare October 15, 2024 15:31
@kapral18
Copy link
Contributor Author

@kapral18 , I found the tour is cropped when opening the flyout. Could you please try if it is reproducible?

Screen.Recording.2024-10-15.at.15.21.41.mov

thanks for checking but cannot reproduce this

Screen.Recording.2024-10-15.at.17.22.18.mov

@kapral18 kapral18 added backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) and removed v9.0.0 v8.16.0 backport:version Backport to applied version labels v8.17.0 labels Oct 15, 2024
@kapral18 kapral18 force-pushed the feat/DQD/185882-add-historical-results-tour-guide branch 2 times, most recently from ab2dede to 06c7bbb Compare October 15, 2024 19:18
- fix tour guide's visibility of empty patterns (happens mostly in
  serverless)
- fix eui bug preventing from proper recalculation of tour guide popover
  position on surrounding accordions' toggle
- reduce potential flakiness of some tests in CI with increased timeouts
- Renamed storage key for historical results tour from
`HISTORICAL_RESULTS_TOUR_IS_ACTIVE_STORAGE_KEY` to
`HISTORICAL_RESULTS_TOUR_IS_DISMISSED_STORAGE_KEY`.
- Added a new custom hook `useIsHistoricalResultsTourActive` to manage
the state of the historical results tour.
- Updated `IndicesDetails` component to use the new hook for determining
if the tour is active.
- Modified tests to reflect changes in storage key and tour dismissal
logic.
- Adjusted `HistoricalResultsTour` component to accept an optional
`zIndex` prop for better positioning.
- removed unnecessary useMemo hooks
@kapral18 kapral18 force-pushed the feat/DQD/185882-add-historical-results-tour-guide branch from 06c7bbb to 0d85881 Compare October 15, 2024 21:25
@elasticmachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #42 / Actions and Triggers app Maintenance Windows Maintenance window update form should update a maintenance window

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 6016 6020 +4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 20.6MB 20.6MB +3.0KB
Unknown metric groups

ESLint disabled line counts

id before after diff
@kbn/ecs-data-quality-dashboard 7 8 +1

Total ESLint disabled count

id before after diff
@kbn/ecs-data-quality-dashboard 7 8 +1

History

  • 💔 Build #242820 failed 06c7bbb4a0ee325e129dcb5eafb1f9c53f984bc0
  • 💔 Build #242797 failed ab2dede3d96cf5a4ad449660f3e42746dd1f98ff
  • 💔 Build #242714 failed 7744ee914da33d56295fd4dfd0681e38cdcc6e9c
  • 💔 Build #242635 failed 78906bdb7bf709c0b02ec7142178faa8fcef8d20
  • 💚 Build #242385 succeeded e92a06fcc874390befea393416da7f9865c8b1df

cc @kapral18

@kapral18 kapral18 merged commit c448593 into elastic:main Oct 15, 2024
41 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11355862524

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 15, 2024
…96127)

addresses elastic#195971

This PR adds missing new historical results feature tour guide.

## Tour guide features:
- ability to maintain visual presence while collapsing accordions in
list-view
- move from list-view to flyout view and back
- seamlessly integrates with existing opening flyout and history tab
functionality

## PR decisions with explanation:
- data-tour-element has been introduced on select elements (like first
actions of each first row) to avoid polluting every single element with
data-test-subj. This way it's imho specific and semantically more clear
what the elements are for.
- early on I tried to control the anchoring with refs but some eui
elements don't allow passing refs like EuiTab, so instead a more simpler
and straightforward approach with dom selectors has been chosen
- localStorage key name has been picked in accordance with other
instances of usage
`securitySolution.dataQualityDashboard.historicalResultsTour.v8.16.isActive`
the name includes the full domain + the version when it's introduced.
And since this tour step is a single step there is no need to stringify
an object with `isTourActive` in and it's much simpler to just bake the
activity state into the name and make the value just a boolean.

## UI Demo

### Anchor reposition demo (listview + flyout)

https://github.com/user-attachments/assets/0f961c51-0e36-48ca-aab4-bef3b0d1269e

### List view tour guide try it + reload demo

https://github.com/user-attachments/assets/ca1f5fda-ee02-4a48-827c-91df757a8ddf

### FlyOut Try It + reload demo

https://github.com/user-attachments/assets/d0801ac3-1ed1-4e64-9d6b-3140b8402bdf

### Manual history tab selection path + reload demo

https://github.com/user-attachments/assets/34dbb447-2fd6-4dc0-a4f5-682c9c65cc8b

### Manual open history view path + reload demo

https://github.com/user-attachments/assets/945dd042-fc12-476e-8d23-f48c9ded9f65

### Dismiss list view tour guide + reload demo

https://github.com/user-attachments/assets/d20d1416-827f-46f2-9161-a3c0a8cbd932

### Dismiss FlyOut tour guide + reload demo

https://github.com/user-attachments/assets/8f085f59-20a9-49f0-b5b3-959c4719f5cb

### Serverless empty pattern handling + reposition demo

https://github.com/user-attachments/assets/4af5939e-663c-4439-a3fc-deff2d4de7e4
(cherry picked from commit c448593)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Oct 16, 2024
…6127) (#196456)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Security Solution][DQD] Add historical results tour guide
(#196127)](#196127)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Karen
Grigoryan","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-15T23:18:50Z","message":"[Security
Solution][DQD] Add historical results tour guide (#196127)\n\naddresses
#195971\r\n\r\nThis PR adds missing new historical results feature tour
guide.\r\n\r\n## Tour guide features:\r\n- ability to maintain visual
presence while collapsing accordions in\r\nlist-view\r\n- move from
list-view to flyout view and back\r\n- seamlessly integrates with
existing opening flyout and history tab\r\nfunctionality\r\n\r\n## PR
decisions with explanation:\r\n- data-tour-element has been introduced
on select elements (like first\r\nactions of each first row) to avoid
polluting every single element with\r\ndata-test-subj. This way it's
imho specific and semantically more clear\r\nwhat the elements are
for.\r\n- early on I tried to control the anchoring with refs but some
eui\r\nelements don't allow passing refs like EuiTab, so instead a more
simpler\r\nand straightforward approach with dom selectors has been
chosen\r\n- localStorage key name has been picked in accordance with
other\r\ninstances of
usage\r\n`securitySolution.dataQualityDashboard.historicalResultsTour.v8.16.isActive`\r\nthe
name includes the full domain + the version when it's introduced.\r\nAnd
since this tour step is a single step there is no need to
stringify\r\nan object with `isTourActive` in and it's much simpler to
just bake the\r\nactivity state into the name and make the value just a
boolean.\r\n\r\n## UI Demo\r\n\r\n### Anchor reposition demo (listview +
flyout)\r\n\r\nhttps://github.com/user-attachments/assets/0f961c51-0e36-48ca-aab4-bef3b0d1269e\r\n\r\n###
List view tour guide try it + reload
demo\r\n\r\nhttps://github.com/user-attachments/assets/ca1f5fda-ee02-4a48-827c-91df757a8ddf\r\n\r\n###
FlyOut Try It + reload
demo\r\n\r\nhttps://github.com/user-attachments/assets/d0801ac3-1ed1-4e64-9d6b-3140b8402bdf\r\n\r\n###
Manual history tab selection path + reload
demo\r\n\r\nhttps://github.com/user-attachments/assets/34dbb447-2fd6-4dc0-a4f5-682c9c65cc8b\r\n\r\n###
Manual open history view path + reload
demo\r\n\r\nhttps://github.com/user-attachments/assets/945dd042-fc12-476e-8d23-f48c9ded9f65\r\n\r\n###
Dismiss list view tour guide + reload
demo\r\n\r\nhttps://github.com/user-attachments/assets/d20d1416-827f-46f2-9161-a3c0a8cbd932\r\n\r\n###
Dismiss FlyOut tour guide + reload
demo\r\n\r\nhttps://github.com/user-attachments/assets/8f085f59-20a9-49f0-b5b3-959c4719f5cb\r\n\r\n###
Serverless empty pattern handling + reposition
demo\r\n\r\nhttps://github.com/user-attachments/assets/4af5939e-663c-4439-a3fc-deff2d4de7e4","sha":"c448593d546f6200b0d2d35bce043bef521f41a6","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["v9.0.0","Team:Threat
Hunting","release_note:feature","Team:Threat
Hunting:Explore","backport:prev-minor"],"title":"[Security
Solution][DQD] Add historical results tour
guide","number":196127,"url":"https://github.com/elastic/kibana/pull/196127","mergeCommit":{"message":"[Security
Solution][DQD] Add historical results tour guide (#196127)\n\naddresses
#195971\r\n\r\nThis PR adds missing new historical results feature tour
guide.\r\n\r\n## Tour guide features:\r\n- ability to maintain visual
presence while collapsing accordions in\r\nlist-view\r\n- move from
list-view to flyout view and back\r\n- seamlessly integrates with
existing opening flyout and history tab\r\nfunctionality\r\n\r\n## PR
decisions with explanation:\r\n- data-tour-element has been introduced
on select elements (like first\r\nactions of each first row) to avoid
polluting every single element with\r\ndata-test-subj. This way it's
imho specific and semantically more clear\r\nwhat the elements are
for.\r\n- early on I tried to control the anchoring with refs but some
eui\r\nelements don't allow passing refs like EuiTab, so instead a more
simpler\r\nand straightforward approach with dom selectors has been
chosen\r\n- localStorage key name has been picked in accordance with
other\r\ninstances of
usage\r\n`securitySolution.dataQualityDashboard.historicalResultsTour.v8.16.isActive`\r\nthe
name includes the full domain + the version when it's introduced.\r\nAnd
since this tour step is a single step there is no need to
stringify\r\nan object with `isTourActive` in and it's much simpler to
just bake the\r\nactivity state into the name and make the value just a
boolean.\r\n\r\n## UI Demo\r\n\r\n### Anchor reposition demo (listview +
flyout)\r\n\r\nhttps://github.com/user-attachments/assets/0f961c51-0e36-48ca-aab4-bef3b0d1269e\r\n\r\n###
List view tour guide try it + reload
demo\r\n\r\nhttps://github.com/user-attachments/assets/ca1f5fda-ee02-4a48-827c-91df757a8ddf\r\n\r\n###
FlyOut Try It + reload
demo\r\n\r\nhttps://github.com/user-attachments/assets/d0801ac3-1ed1-4e64-9d6b-3140b8402bdf\r\n\r\n###
Manual history tab selection path + reload
demo\r\n\r\nhttps://github.com/user-attachments/assets/34dbb447-2fd6-4dc0-a4f5-682c9c65cc8b\r\n\r\n###
Manual open history view path + reload
demo\r\n\r\nhttps://github.com/user-attachments/assets/945dd042-fc12-476e-8d23-f48c9ded9f65\r\n\r\n###
Dismiss list view tour guide + reload
demo\r\n\r\nhttps://github.com/user-attachments/assets/d20d1416-827f-46f2-9161-a3c0a8cbd932\r\n\r\n###
Dismiss FlyOut tour guide + reload
demo\r\n\r\nhttps://github.com/user-attachments/assets/8f085f59-20a9-49f0-b5b3-959c4719f5cb\r\n\r\n###
Serverless empty pattern handling + reposition
demo\r\n\r\nhttps://github.com/user-attachments/assets/4af5939e-663c-4439-a3fc-deff2d4de7e4","sha":"c448593d546f6200b0d2d35bce043bef521f41a6"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/196127","number":196127,"mergeCommit":{"message":"[Security
Solution][DQD] Add historical results tour guide (#196127)\n\naddresses
#195971\r\n\r\nThis PR adds missing new historical results feature tour
guide.\r\n\r\n## Tour guide features:\r\n- ability to maintain visual
presence while collapsing accordions in\r\nlist-view\r\n- move from
list-view to flyout view and back\r\n- seamlessly integrates with
existing opening flyout and history tab\r\nfunctionality\r\n\r\n## PR
decisions with explanation:\r\n- data-tour-element has been introduced
on select elements (like first\r\nactions of each first row) to avoid
polluting every single element with\r\ndata-test-subj. This way it's
imho specific and semantically more clear\r\nwhat the elements are
for.\r\n- early on I tried to control the anchoring with refs but some
eui\r\nelements don't allow passing refs like EuiTab, so instead a more
simpler\r\nand straightforward approach with dom selectors has been
chosen\r\n- localStorage key name has been picked in accordance with
other\r\ninstances of
usage\r\n`securitySolution.dataQualityDashboard.historicalResultsTour.v8.16.isActive`\r\nthe
name includes the full domain + the version when it's introduced.\r\nAnd
since this tour step is a single step there is no need to
stringify\r\nan object with `isTourActive` in and it's much simpler to
just bake the\r\nactivity state into the name and make the value just a
boolean.\r\n\r\n## UI Demo\r\n\r\n### Anchor reposition demo (listview +
flyout)\r\n\r\nhttps://github.com/user-attachments/assets/0f961c51-0e36-48ca-aab4-bef3b0d1269e\r\n\r\n###
List view tour guide try it + reload
demo\r\n\r\nhttps://github.com/user-attachments/assets/ca1f5fda-ee02-4a48-827c-91df757a8ddf\r\n\r\n###
FlyOut Try It + reload
demo\r\n\r\nhttps://github.com/user-attachments/assets/d0801ac3-1ed1-4e64-9d6b-3140b8402bdf\r\n\r\n###
Manual history tab selection path + reload
demo\r\n\r\nhttps://github.com/user-attachments/assets/34dbb447-2fd6-4dc0-a4f5-682c9c65cc8b\r\n\r\n###
Manual open history view path + reload
demo\r\n\r\nhttps://github.com/user-attachments/assets/945dd042-fc12-476e-8d23-f48c9ded9f65\r\n\r\n###
Dismiss list view tour guide + reload
demo\r\n\r\nhttps://github.com/user-attachments/assets/d20d1416-827f-46f2-9161-a3c0a8cbd932\r\n\r\n###
Dismiss FlyOut tour guide + reload
demo\r\n\r\nhttps://github.com/user-attachments/assets/8f085f59-20a9-49f0-b5b3-959c4719f5cb\r\n\r\n###
Serverless empty pattern handling + reposition
demo\r\n\r\nhttps://github.com/user-attachments/assets/4af5939e-663c-4439-a3fc-deff2d4de7e4","sha":"c448593d546f6200b0d2d35bce043bef521f41a6"}}]}]
BACKPORT-->

Co-authored-by: Karen Grigoryan <[email protected]>
@kapral18 kapral18 deleted the feat/DQD/185882-add-historical-results-tour-guide branch October 16, 2024 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) release_note:feature Makes this part of the condensed release notes Team:Threat Hunting:Explore Team:Threat Hunting Security Solution Threat Hunting Team v8.16.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants