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][Detections] Adds a back button to the Tour UI on the Rule Management page #128405

Conversation

banderror
Copy link
Contributor

@banderror banderror commented Mar 23, 2022

Ticket: #126084

Summary

This PR adds a back button to each of the two Tour steps. Users will be able to freely navigate between the steps.

Before

After

@banderror banderror added bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Feature:Rule Management Security Solution Detection Rule Management Team:Detection Rule Management Security Detection Rule Management Team v8.1.2 labels Mar 23, 2022
@banderror banderror self-assigned this Mar 23, 2022
@banderror banderror force-pushed the back-button-for-tour-ui-on-rule-management-page branch from 3e8e952 to b00c65e Compare March 23, 2022 17:55
@banderror banderror requested a review from a team March 23, 2022 17:56
@banderror banderror marked this pull request as ready for review March 23, 2022 17:56
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detections-response (Team:Detections and Resp)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@banderror banderror linked an issue Mar 23, 2022 that may be closed by this pull request
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
securitySolution 4.7MB 4.7MB +458.0B

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @banderror

Copy link
Member

@spong spong left a comment

Choose a reason for hiding this comment

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

LGTM!

Two notes:

  1. Seems to be some jitter in the initial position. Not sure if this is new behavior or pre-existing, but this can be seen when clicking Go back or when losing/regaining focus when on the first step.

Go back from step 2:

Focus change:

  1. This is more of component feedback about the EUITour, but it would be nice if there were left/right chevrons next to the tour step indicators that we could use for going forward/backward instead of having to plumb this ourselves. Would also be nice if the step indicator could be clicked too, but minor details. I know they're iterating fast on this component now as it's getting more use, so just some feedback we can provide the EUI folks.

@banderror
Copy link
Contributor Author

banderror commented Mar 24, 2022

@spong thank you, completely agree!

Seems to be some jitter in the initial position. Not sure if this is new behavior or pre-existing, but this can be seen when clicking Go back or when losing/regaining focus when on the first step.

It's a pre-existing behavior. EuiTour implementation has other issues like a step's popover not following the HTML element it points to, if the element changes its position on the page (this is an issue for us because we can show callouts on the page after the initial rendering).

There's an issue elastic/eui#5731 that might be related. The suggestion is elastic/eui#5731 (comment). I think next time we decide to show a tour, we should try to use this new anchor property and see if it helps the step popovers to manage their own positions on the screen properly.

it would be nice if there were left/right chevrons next to the tour step indicators that we could use for going forward/backward instead of having to plumb this ourselves. Would also be nice if the step indicator could be clicked too, but minor details.

++ There are two existing issues in the EUI repo about this, I posted some comments there: elastic/eui#4831 and elastic/eui#5715

I know they're iterating fast on this component now as it's getting more use, so just some feedback we can provide the EUI folks.

It looks like they addressed #124052 in elastic/eui#5696 which would be fantastic!

All in all, I think when we revisit the Tour UI, we should build it differently - using the new anchor property and consolidating all the tour steps and logic in a single component. We shouldn't need to wrap the page with the provider anymore. And there's a chance that this implementation based on query selectors will have fewer UI glitches. And then, if we still see any bugs, we will open tickets for them in the EUI repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting bug Fixes for quality problems that affect the customer experience Feature:Rule Management Security Solution Detection Rule Management release_note:skip Skip the PR/issue when compiling release notes Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.1.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Security Solution] "Tour" under rules cannot go back to Step 1
4 participants