-
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 #81802
Conversation
@elasticmachine merge upstream |
x-pack/plugins/security_solution/public/timelines/components/timeline/properties/index.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/public/timelines/components/timeline/header/save_timeline.tsx
Outdated
Show resolved
Hide resolved
}) => { | ||
const onDescriptionChanged = useCallback( | ||
(e) => { | ||
updateDescription({ id: timelineId, description: e.target.value, disableAutoSave }); |
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 should add debounce
to don't dispatch updateDescription
on every key press?
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.
It seems that there's already a delay 500ms in x-pack/plugins/security_solution/public/timelines/store/timeline/epic.ts Line 500, so I think it's ok here
export const Name = React.memo<NameProps>( | ||
({ | ||
autoFocus = false, | ||
className = TIMELINE_TITLE_CLASSNAME, |
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.
why do we need this className
?
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.
yeah, I'm using that to auto focus the input field.
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.
@patrykkopycinski you're right! useRef works well for autoFocus, I've removed classname accordingly. Thank you 👍
marginRight, | ||
}) => { | ||
const handleChange = useCallback( | ||
(e) => updateTitle({ id: timelineId, title: e.target.value, disableAutoSave }), |
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 should add debounce to don't dispatch updateTitle
on every key press?
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.
It seems that there's already a delay 500ms in x-pack/plugins/security_solution/public/timelines/store/timeline/epic.ts Line 500, so I think it's ok here
</EuiFlexItem> | ||
<EuiFlexItem grow={false} component="span"> | ||
<EuiButton | ||
isDisabled={title.trim().length === 0 || isSaving} |
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.
Thanks for adding these checks! 😄
Would you be willing to consider adding a condition that disables the button when the modal is launched in "edit" mode, but no changes have been made?
The rational for this change is clicking save when no mutations have been made (in edit mode) kicks off IO that updates the saved object, per the gif below:
Even though nothing really changed, updating the saved object effects, for example, the Recent timelines widget on the Overview:
Recent Timelines before save
Recent Timelines after save
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, no problem. I'll open another PR for this.
...ugins/security_solution/public/timelines/components/timeline/header/save_timeline_button.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/public/timelines/components/timeline/header/translations.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.
Thanks for this enhancement as part of the Timeline Evolution @angorayc! 🙏
Desk tested locally, including the following a11y goals:
- The pencil icon buttons (located next to the timeline name and description labels) shall be accessible via keyboard
- The
Save Timeline / Name Timeline
modal shall focus theName
field when the modal is launched - All inputs and buttons in the modal shall be keyboard focusable
- Changes to the modal contents may be discarded by pressing the Escape key (note: this can't be fully tested until the old inputs are removed, but I confirmed it doesn't make a network call when Escape is pressed)
I left a few minor comments, but it looks great overall!
LGTM 🚀
💚 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: |
* init modal for saving timeline * disable auto save * unit test * fix type error * update translation * add unit tests * rename constant * break components into files * autoFocus and close modal on finish * rename constant * fix description label * update wording * review * fix dependency * remove classname * update wording Co-authored-by: Kibana Machine <[email protected]>
* master: (71 commits) [Chrome] Extension to append an element to the last breadcrumb (elastic#82015) [Monitoring] Thread pool rejections alert (elastic#79433) [Actions] Fix actionType type on registerType function (elastic#82125) [Security Solution] Modal for saving timeline (elastic#81802) add tests for index pattern switching (elastic#81987) TS project references for share plugin (elastic#82051) [Graph] Fix problem with duplicate ids (elastic#82109) skip 'returns a single bucket if array has 1'. related elastic#81460 Add a link to documentation in the alerts and actions management UI (elastic#81909) [Fleet] fix duplicate ingest pipeline refs (elastic#82078) Context menu trigger for URL Drilldown (elastic#81158) SO management: fix legacy import index pattern selection being reset when switching page (elastic#81621) Fixed dead links (elastic#78696) [Search] Add "restore" to session service (elastic#81924) fix Lens heading structure (elastic#81752) [ML] Data Frame Analytics: Fix feature importance cell value and decision path chart (elastic#82011) Remove legacy app arch items from codeowners. (elastic#82084) [TSVB] Renamed 'positive rate' to 'counter rate' (elastic#80939) Expressions/migrations2 (elastic#81281) [Telemetry] [Schema] remove number type and support all es number types (elastic#81774) ...
* init modal for saving timeline * disable auto save * unit test * fix type error * update translation * add unit tests * rename constant * break components into files * autoFocus and close modal on finish * rename constant * fix description label * update wording * review * fix dependency * remove classname * update wording Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
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
showWarning
in x-pack/plugins/security_solution/public/timelines/components/timeline/header/title_and_description.tsx line 66is to reserve for using the modal as a prompt, incase user navigating away before saving the timeline.
Trigger the modal for an unsaved timeline before navigating away is not included in this PR, but the layout and the action is there:
Checklist
Delete any items that are not applicable to this PR.