[Security Solution][Detections] fixes UX issue with discoverability of new features on the Rules page #124343

vitaliidm · 2022-02-02T13:36:02Z

Added tour with 2 steps
- Sorting/filtering for in memory table
- New Bulk Edit actions
Created context with tour management, which wraps rules table
Results of user tour's journey is saved in localStorage
Disabled tour for Cypress tests

Note: Text in second step was changed to "You can now bulk update index patterns and tags for multiple custom rules at once." It's not reflected on screenshots. On screenshots and video present older text: "You can now bulk update index patterns and tags for multiple rules at once.", where word custom is absent

UI

First step

Second step

Eui Tour: https://elastic.github.io/eui/#/display/tour

Screen recording

Screen.Recording.2022-02-03.at.17.00.27.mov

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)

vitaliidm · 2022-02-02T21:28:14Z

@elasticmachine merge upstream

vitaliidm · 2022-02-04T09:07:52Z

@elasticmachine merge upstream

vitaliidm · 2022-02-07T11:58:41Z

The EuiTour doesn't seem to be working well with scroll:

It uses under the hood EuiPopover, which has z-index: 9900 !important;
And has similar behaviour in other components(for example filter tag)

I can try to play with z-index, but it can cause even more issues than current one
For example, header has z-index:1000, but uses popovers as well. If we increase header z-index, Kibana menus got hidden behind header and so on and so on.
Might be viable option to leave as it is, given a fact it's a feature highlight that would get removed in future releases and can/will be dismissed by user at the first interaction. What are you thoughts?

banderror

Found a bug when testing locally. The tour component doesn't work well with the prebuilt rule updates callout:

Video: https://recordit.co/Yn9S9PUKom

...y_solution/public/detections/pages/detection_engine/rules/all/rules_feature_tour_context.tsx

x-pack/plugins/security_solution/cypress/tasks/login.ts

...y_solution/public/detections/pages/detection_engine/rules/all/rules_feature_tour_context.tsx

banderror

Finished going through the changes, and overall have mixed feelings about the Tour's API:

Its hook accepts EuiStatelessTourStep[] which is a mixture of logic (e.g. step number, onFinish) and UI (title, content, children, maxWidth) which forces to move some of the UI props out of the corresponding UI components that implement the steps. I'd expect all these titles and widths to be specified in a component's TSX markup rather than in a react context.
It seems to be an "alpha" implementation, you can't navigate back and forth between the steps out of the box, persistence is not included, adding the "go to next step" button to the step requires some tricks, etc.
When I started reviewing the PR, it looked somewhat surprising to me as someone not familiar with the Tour API that this small feature would require introducing a whole page-level react context needed for this feature specifically.

On the other hand, I guess we have been warned in this comment: https://github.com/elastic/security-team/issues/1972#issuecomment-1027197837 🙂

I don't have any suggestions for how it could be improved immediately in this PR. Hope the Tour's API will be completely reworked as described in #124052

banderror · 2022-02-07T16:56:35Z

...plugins/security_solution/public/detections/pages/detection_engine/rules/all/utility_bar.tsx

+    // use optional rulesFeatureTourContext as AllRulesUtilityBar can be used outside the context
+    const featureTour = useRulesFeatureTourContextOptional();


In what cases it could be outside of the context?

on exceptions page x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/all/exceptions/exceptions_table.tsx

🙀 The bar used in the Exceptions table is very different from the one used in the Rules table:

I think we should split AllRulesUtilityBar into two different components each used for its own table. But that's probably out of the scope of this PR. I'll open a ticket for that.

...y_solution/public/detections/pages/detection_engine/rules/all/rules_feature_tour_context.tsx

xcrzx · 2022-02-08T10:15:05Z

I can try to play with z-index, but it can cause even more issues than current one
For example, header has z-index:1000, but uses popovers as well. If we increase header z-index, Kibana menus got hidden behind header and so on and so on.
Might be viable option to leave as it is, given a fact it's a feature highlight that would get removed in future releases and can/will be dismissed by user at the first interaction. What are you thoughts?

From my perspective, overlapping UI elements don't look great and create an impression of a buggy product. But if you think this issue is too hard to fix, then I'm okay with leaving it as is.

xcrzx

The EuiTour API doesn't seem to be finished as of right now. It would be great to get in touch with the EUI team and share our use case and suggestions to improve the API.

Other than that code changes LGTM

vitaliidm · 2022-02-08T10:34:46Z

I can try to play with z-index, but it can cause even more issues than current one
For example, header has z-index:1000, but uses popovers as well. If we increase header z-index, Kibana menus got hidden behind header and so on and so on.
Might be viable option to leave as it is, given a fact it's a feature highlight that would get removed in future releases and can/will be dismissed by user at the first interaction. What are you thoughts?

From my perspective, overlapping UI elements don't look great and create an impression of a buggy product. But if you think this issue is too hard to fix, then I'm okay with leaving it as is.

There is even issue for that elastic/eui#4756 as well, with no fix resolution because of its complexity

vitaliidm · 2022-02-08T10:46:05Z

Found a bug when testing locally. The tour component doesn't work well with the prebuilt rule updates callout:

Video: https://recordit.co/Yn9S9PUKom

Thanks for catching this 👍

So this issue appeared to be related to a fact, that EuiPopover is not reacting to layout changes. I've spoke to Eui team and advise was to delay render of tour. It's indeed, not a perfect solution, but it fixes at least current issue.

Other solution we might do as well: put a time delay for tour render. But it's uncertain, and I don't like it.

Here is a fix commit: ce81e4e

banderror

Thank you @vitaliidm for responding to the comments and fixing the issue with the callout.
LGTM 👍

vitaliidm · 2022-02-08T13:36:32Z

@elasticmachine merge upstream

kibana-ci · 2022-02-08T17:08:23Z

💛 Build succeeded, but was flaky

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	2869	2871	+2

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	+3.2KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`securitySolution`	246.4KB	246.5KB	+99.0B

History

💚 Build #22225 succeeded 568d11b
💔 Build #22153 failed 2a6bd55
💔 Build #22147 failed 76f2316
💚 Build #22087 succeeded 0767c69
💔 Build #22080 failed f95badb
💚 Build #21948 succeeded 4af0a91

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

cc @vitaliidm

…f new features on the Rules page (elastic#124343) - Added tour with 2 steps - Sorting/filtering for in memory table - New Bulk Edit actions - Created context with tour management, which wraps rules table - Results of user tour's journey is saved in localStorage - Disabled tour for Cypress tests **Note:** Text in second step was changed to "You can now bulk update index patterns and tags for multiple custom rules at once." It's not reflected on screenshots. On screenshots and video present older text: "You can now bulk update index patterns and tags for multiple rules at once.", where word `custom` is absent ## UI ### First step <img width="1287" alt="Screenshot 2022-02-03 at 16 54 20" src="https://user-images.githubusercontent.com/92328789/152391079-57a12b97-a273-4852-95e6-222680d09464.png"> ### Second step <img width="1281" alt="Screenshot 2022-02-03 at 16 54 29" src="https://user-images.githubusercontent.com/92328789/152391090-87cd436a-eb74-4358-b699-53c3e79fdb48.png"> Eui Tour: https://elastic.github.io/eui/#/display/tour ## Screen recording https://user-images.githubusercontent.com/92328789/152391296-0b03b299-270a-4d96-9adf-d98bc6405b4f.mov ### Checklist Delete any items that are not applicable to this PR. - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [x] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [x] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) (cherry picked from commit 6f71ffc)

kibanamachine · 2022-02-08T17:11:18Z

💚 All backports created successfully

Status	Branch	Result
✅	8.1

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

Questions ?

Please refer to the Backport tool documentation

…f new features on the Rules page (#124343) (#124992) - Added tour with 2 steps - Sorting/filtering for in memory table - New Bulk Edit actions - Created context with tour management, which wraps rules table - Results of user tour's journey is saved in localStorage - Disabled tour for Cypress tests **Note:** Text in second step was changed to "You can now bulk update index patterns and tags for multiple custom rules at once." It's not reflected on screenshots. On screenshots and video present older text: "You can now bulk update index patterns and tags for multiple rules at once.", where word `custom` is absent ## UI ### First step <img width="1287" alt="Screenshot 2022-02-03 at 16 54 20" src="https://user-images.githubusercontent.com/92328789/152391079-57a12b97-a273-4852-95e6-222680d09464.png"> ### Second step <img width="1281" alt="Screenshot 2022-02-03 at 16 54 29" src="https://user-images.githubusercontent.com/92328789/152391090-87cd436a-eb74-4358-b699-53c3e79fdb48.png"> Eui Tour: https://elastic.github.io/eui/#/display/tour ## Screen recording https://user-images.githubusercontent.com/92328789/152391296-0b03b299-270a-4d96-9adf-d98bc6405b4f.mov ### Checklist Delete any items that are not applicable to this PR. - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [x] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [x] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) (cherry picked from commit 6f71ffc) Co-authored-by: Vitalii Dmyterko <92328789+vitaliidm@users.noreply.github.com>

… on the Rule Management page introduced in 8.1 (#128398) **Ticket:** #125504 ## Summary This PR removes the Tour UI from components of the Rule Management page. We don't need to show `8.1` features in the `8.2.0` version. The Tour was previously introduced in #124343. ## Details - The tour steps are removed from the components (`<EuiTourStep>`). - The tour provider is removed from the page. It has been changed a little bit. - I thought it could be useful to leave the implementation for now, in case we want to show new tours in the next versions. If we don't use it during the next few dev cycles given we will be shipping new features on the Rule Management page, we will need to completely remove it from the codebase. - A short README is added with some notes on the Tour UI.

add feature tour

5051cf1

vitaliidm self-assigned this Feb 2, 2022

vitaliidm added 2 commits February 2, 2022 17:23

small refactoring

adff78f

mock useFeatureTour in tests

6551657

vitaliidm added 2 commits February 2, 2022 18:33

refactoring

f03fd39

use useMemo

5211418

Merge branch 'main' into security-solution/feature-tour

ad5cab6

elastic deleted a comment from kibana-ci Feb 2, 2022

use context

3540024

elastic deleted a comment from kibana-ci Feb 3, 2022

vitaliidm added 2 commits February 3, 2022 16:57

UI & refactoring

783d6b7

adds code comments

1de545c

kibanamachine and others added 4 commits February 4, 2022 04:07

Merge branch 'main' into security-solution/feature-tour

73ea893

fixes

65b96c9

fix

9fce967

fix

6e9fcb3

vitaliidm added auto-backport Deprecated - use backport:version if exact versions are needed and removed backport:skip This commit does not require backporting labels Feb 4, 2022

vitaliidm added 2 commits February 4, 2022 13:40

remove duplicated copyright

f66ac82

remove unnecessary onBeforeLoad

794b2fa

fix eslint

0767c69

vitaliidm changed the title ~~[Security Solution][Detections] adds new features tour for rules page~~ [Security Solution][Detections] fixes UX issue with discoverability of new features on the Rules page Feb 7, 2022

banderror reviewed Feb 7, 2022

View reviewed changes

vitaliidm and others added 2 commits February 7, 2022 15:13

Merge branch 'main' into security-solution/feature-tour

76f2316

fix conflict issue

2a6bd55

banderror reviewed Feb 7, 2022

View reviewed changes

vitaliidm added 2 commits February 7, 2022 18:19

CR fixes

568d11b

CR imporvements, fixes

1435286

xcrzx approved these changes Feb 8, 2022

View reviewed changes

CR: fix issue with prebuilt rules callout

ce81e4e

add comment

5d69303

banderror approved these changes Feb 8, 2022

View reviewed changes

Merge branch 'main' into security-solution/feature-tour

752913f

vitaliidm enabled auto-merge (squash) February 8, 2022 13:37

vitaliidm merged commit 6f71ffc into elastic:main Feb 8, 2022

vitaliidm deleted the security-solution/feature-tour branch February 8, 2022 17:09

kibanamachine mentioned this pull request Feb 8, 2022

[8.1] [Security Solution][Detections] fixes UX issue with discoverability of new features on the Rules page (#124343) #124992

Merged

banderror mentioned this pull request Feb 14, 2022

[Security Solution][Detections] Remove the Tour for the new 8.1 features #125504

Closed

banderror mentioned this pull request Mar 23, 2022

[Security Solution][Detections] Removes the Tour for the new features on the Rule Management page introduced in 8.1 #128398

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Detections] fixes UX issue with discoverability of new features on the Rules page #124343

[Security Solution][Detections] fixes UX issue with discoverability of new features on the Rules page #124343

vitaliidm commented Feb 2, 2022 •

edited

Loading

vitaliidm commented Feb 2, 2022

vitaliidm commented Feb 4, 2022

vitaliidm commented Feb 7, 2022

banderror left a comment

banderror left a comment

banderror Feb 7, 2022

vitaliidm Feb 7, 2022

banderror Feb 8, 2022

xcrzx commented Feb 8, 2022

xcrzx left a comment

vitaliidm commented Feb 8, 2022

vitaliidm commented Feb 8, 2022

banderror left a comment

vitaliidm commented Feb 8, 2022

kibana-ci commented Feb 8, 2022

kibanamachine commented Feb 8, 2022

		// use optional rulesFeatureTourContext as AllRulesUtilityBar can be used outside the context
		const featureTour = useRulesFeatureTourContextOptional();

[Security Solution][Detections] fixes UX issue with discoverability of new features on the Rules page #124343

[Security Solution][Detections] fixes UX issue with discoverability of new features on the Rules page #124343

Conversation

vitaliidm commented Feb 2, 2022 • edited Loading

UI

First step

Second step

Screen recording

Checklist

vitaliidm commented Feb 2, 2022

vitaliidm commented Feb 4, 2022

vitaliidm commented Feb 7, 2022

banderror left a comment

Choose a reason for hiding this comment

banderror left a comment

Choose a reason for hiding this comment

banderror Feb 7, 2022

Choose a reason for hiding this comment

vitaliidm Feb 7, 2022

Choose a reason for hiding this comment

banderror Feb 8, 2022

Choose a reason for hiding this comment

xcrzx commented Feb 8, 2022

xcrzx left a comment

Choose a reason for hiding this comment

vitaliidm commented Feb 8, 2022

vitaliidm commented Feb 8, 2022

banderror left a comment

Choose a reason for hiding this comment

vitaliidm commented Feb 8, 2022

kibana-ci commented Feb 8, 2022

💛 Build succeeded, but was flaky

Metrics [docs]

Module Count

Async chunks

Page load bundle

History

kibanamachine commented Feb 8, 2022

💚 All backports created successfully

Questions ?

vitaliidm commented Feb 2, 2022 •

edited

Loading