-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] Guided onboarding, alerts & cases design updates #144249
[Security solution] Guided onboarding, alerts & cases design updates #144249
Conversation
x-pack/plugins/security_solution/public/common/components/guided_onboarding_tour/tour_config.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/public/common/components/guided_onboarding_tour/tour_config.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One change and some questions.
x-pack/plugins/security_solution/public/common/components/guided_onboarding_tour/tour_config.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/public/common/components/guided_onboarding_tour/tour_config.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, better suggestions this time.
x-pack/plugins/security_solution/public/common/components/guided_onboarding_tour/tour_config.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/public/common/components/guided_onboarding_tour/tour_config.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/public/detections/components/take_action_dropdown/index.tsx
Outdated
Show resolved
Hide resolved
|
||
describe('Related Cases', () => { | ||
beforeEach(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests needed improvement. Also, the async call in related_cases
had side effects that the tests were yelling about, so I fixed the side effects. The last 2 tests in this file are what you should pay attention to
} | ||
setRelatedCases(relatedCaseList); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The unit tests were yelling about this being called when the component was unmounted. Therefore, I refactored the async call to avoid this side effect
document.querySelector(`[data-test-subj="create-case-submit"]`); | ||
|
||
export const CasesTourSteps = () => { | ||
const [activeStep, setActiveStep] = useState(AlertsCasesTourSteps.createCase); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this component is passed to Cases to render, it needs to manage its own activeStep
state as the activeStep
managed by security solution useTourContext
is out of context
@@ -81,6 +82,7 @@ export const CreateCaseFlyout = React.memo<CreateCaseFlyoutProps>( | |||
<GlobalStyle /> | |||
<StyledFlyout | |||
onClose={onClose} | |||
tour-step="create-case-flyout" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are labels for my tour step anchor. Previously I was using data-test-subj="create-case-flyout"
but it makes me nervous to call on a test ref in production.
@@ -45,6 +45,7 @@ export const useResponderActionItem = ( | |||
if (isResponseActionsConsoleEnabled && !isAuthzLoading && canAccessResponseConsole && isAlert) { | |||
actions.push( | |||
<ResponderContextMenuItem | |||
key="endpointResponseActions-action-item" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error in tests pointed out that all members of the context menu did not have keys
@@ -123,6 +131,7 @@ export const useAddToCaseActions = ({ | |||
<EuiContextMenuItem | |||
aria-label={ariaLabel} | |||
data-test-subj="add-to-existing-case-action" | |||
key="add-to-existing-case-action" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error in tests pointed out that all members of the context menu did not have keys
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cases changes look good 👍
x-pack/plugins/security_solution/public/common/components/guided_onboarding_tour/README.md
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally and all the functionality work as expected.
Suggesting to improve the naming for the properties for GuidedOnboardingTourStep
, incrementStep
and remove "Allow me to explain some weirdness"
|
||
const afterCaseCreated = useCallback(async () => { | ||
if (isTourShown(SecurityStepId.alertsCases)) { | ||
endTourStep(SecurityStepId.alertsCases); | ||
// user could be on step 5 or 6 at this point, increment to 7 no matter what | ||
incrementStep(SecurityStepId.alertsCases, AlertsCasesTourSteps.viewCase); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could create a new setStep
for this one, using incrementStep
may seem we are incrementing by 7, but we are not incrementing, we are setting a specific step value. If we change the function name we won't need the comment explaining what it does.
...ins/security_solution/public/common/components/event_details/insights/related_cases.test.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thanks Steph this is awesome 🚀
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsasync chunk count
ESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Summary
Makes updates to the guided onboarding design for the alerts and cases tour as requested here.
onClick
property toSecurityTourStep
to accomplish this. When next is click, eitherincrementStep
if noonClick
provided or call theonClick
. Therefore, any providedonClick
should handle incrementing the tour stephideNextButton
)number
was still being used instead ofAlertsCasesTourSteps
Here is what all of that looks like:
flow-wednesday2.mov
To test
xpack.securitySolution.enableExperimental: ['guidedOnboarding']
xpack.cloud.id: 'test'
yarn start --run-examples
/app/guidedOnboardingExample
security
and Step ID toalertsCases
Changes to the
cases
appFor the cases tour step, our designer requested that when the create case flyout opens, the form is pre-populated.
CreateCaseFlyout
ofinitialValue: Partial<Case>
. This value is piped to the form context and spread with in the definition of the create form:defaultValue: { ...initialCaseValue, ...initialValue }
tour-step
properties to Eui elements to use as references for anchors andfocus()
instead ofdata-test-subj