-
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] Modal for saving timeline #81205
Conversation
@@ -179,3 +179,5 @@ export const showAllOthersBucket: string[] = [ | |||
'destination.ip', | |||
'user.name', | |||
]; | |||
|
|||
export const enableNewTimeline = false; |
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.
can use uppercase for the variable name?
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.
yup, renamed it to ENABLE_NEW_TIMELINE
|
||
export const TimelineTitleAndDescription = React.memo<TimelineTitleAndDescriptionProps>( | ||
({ timelineId, toggleSaveTimeline, onSaveTimeline, updateTitle, updateDescription }) => { | ||
const timeline = useShallowEqualSelector((state) => |
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.
const { description, isSaving, savedObjectId, title, timelineType } = ...
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.
I send the entire timeline
to onSaveTimeline
below, so I'm keeping this atm.
x-pack/plugins/security_solution/public/timelines/components/timeline/header/save_timeline.tsx
Outdated
Show resolved
Hide resolved
@@ -39,6 +40,7 @@ const StatefulFlyoutHeader = React.memo<Props>( | |||
noteIds, | |||
notesById, | |||
status, | |||
saveTimeline, |
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.
can we move saveTimeline
action binding to the destination component to avoid unnecessary props drilling?
x-pack/plugins/security_solution/public/timelines/components/timeline/header/save_timeline.tsx
Outdated
Show resolved
Hide resolved
<EuiFlexItem grow={false}> | ||
<EuiFlexGroup justifyContent="flexEnd"> | ||
<EuiFlexItem grow={false} component="span"> | ||
{savedObjectId == null ? ( |
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.
I'm wondering if the behavior shown in the gif below, where the Discard Timeline
button is displayed for a new, non-mutated timeline, is a product of Timeline being in the current "in between" state, where we're just starting to implement the new design, or potentially a bug:
I think the intent of the Discard Timeline
button shown in the gif above is to address the case when a user has performed an action that will result in unsaved changes being discarded, because the timeline was never given a name. I'm not sure if the Discard Timeline
button should appear in the scenario shown above, because we created a new timeline, with no mutations.
@angorayc would you be willing to confirm the expected behavior with @marrasherrier?
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.
@angorayc @andrew-goldstein I think it's best to keep the 'discard' option for consistency.
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.
@monina-n looping you in as well for transparency!
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.
From what we've talked about before, this is the same modal that will also appear when you click 'X' to close an unsaved timeline. I agree with @andrew-goldstein that having "Discard timeline" would be an unexpected option when you click the pencil icon to add a name/description? If I click the pencil icon, I would expect the options to be "Save" and "Cancel" with cancel closing the modal instead of deleting the entire timeline.
I think we can potentially go with @andrew-goldstein's suggestion of not having the "discard button" on the pencil and only when we're closing the timeline. Another possible solution is having two modals: (1) clicking the pencil icon and having the options be "Save Timeline" and "Cancel" and (2) clicking the 'X' to close a timeline and having the options be "Save Timeline" and "Discard Timeline" with the "discard" button being in red and maybe having a note that if you don't save the timeline, it will delete it.
@marrasherrier @andrew-goldstein @angorayc what do you guys think?
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.
@monina-n thanks for clarifying, that makes sense to me. @angorayc here is the separate interaction more clearly mocked out: https://www.figma.com/file/QVxq6M4TFDJlcgFHGe9wYT/Timeline-Evolution?node-id=1034%3A27036
x-pack/plugins/security_solution/public/timelines/components/timeline/header/save_timeline.tsx
Outdated
Show resolved
Hide resolved
Pinging @elastic/ingest-management (Team:Ingest Management) |
Pinging @elastic/apm-ui (Team:apm) |
Pinging @elastic/uptime (Team:uptime) |
9919a3c
to
899336b
Compare
@angorayc In past cases where multiple teams have been accidentally pinged on a PR, I've seen authors close the PR and reopen a new one in order to avoid all of the pinged team-members having to manually unsubscribe from the PR. Otherwise those team-members will just keep getting pinged due to comments, reviews, stuff like that. Would you mind doing that in this case, too please? |
Follow up in #81802 |
💚 Build SucceededMetrics [docs]@kbn/optimizer bundle module count
async chunks size
page load bundle size
History
To update your PR or re-run it, just comment with: |
Summary
This is a button and modal to edit / update timeline's title / description.
This shouldn't change the existing behaviour of title and description field.
Please go to x-pack/plugins/security_solution/common/constants.ts Line 183 and change
enableNewTimeline
to true.There will be a pencil icon appears next to description field. A modal appears after clicking on the button.
https://github.com/elastic/security-team/issues/426
Checklist
Delete any items that are not applicable to this PR.