Skip to content

Commit

Permalink
[Security Solution] Fix - Notes Flyout Product Feedback (elastic#188129)
Browse files Browse the repository at this point in the history 
# Summary

Fixes below bugs based on feedback from @paulewing.


## Event Details Toggle in Notes

@paulewing requested to remove the event toggle 

|Before|After|
|---|---|
|![Bildschirmfoto 2024-07-11 um 17 48
15](https://github.com/elastic/kibana/assets/7485038/2b45d3a9-6f1a-4f05-8824-10e2c6265266)|
![Bildschirmfoto 2024-07-11 um 17 46
01](https://github.com/elastic/kibana/assets/7485038/b02c06ff-f556-4894-a588-a88bcdd8bc8c)|


## Notes Flyout remains open when switching tabs
|Before|After|
|---|---|
|<video
src="https://github.com/elastic/kibana/assets/7485038/4228d2d6-c2ad-40dc-9e6c-ec049f834e8f"
/>|<video
src="https://github.com/elastic/kibana/assets/7485038/0e010c22-4539-4428-9b1b-3b323a9f491c"
/>|


## Notes Flyout should be resizable

As shown in above video, notes flyout is now resizable.
  • Loading branch information
@logeekal
logeekal authored Jul 12, 2024
1 parent 9440ea5 commit 309b907
Show file tree
Hide file tree
Showing 10 changed files with 244 additions and 59 deletions.
8 changes: 7 additions & 1 deletion x-pack/plugins/security_solution/public/timelines/components/notes/note_cards/index.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ export interface NoteCardsProps {
eventId?: string;
timelineId: string;
onCancel?: () => void;
showToggleEventDetailsAction?: boolean;
}

/** A view for entering and reviewing notes */
Expand All @@ -65,6 +66,7 @@ export const NoteCards = React.memo<NoteCardsProps>(
eventId,
timelineId,
onCancel,
showToggleEventDetailsAction = true,
}) => {
const [newNote, setNewNote] = useState('');

Expand Down Expand Up @@ -109,7 +111,11 @@ export const NoteCards = React.memo<NoteCardsProps>(
<EuiScreenReaderOnly data-test-subj="screenReaderOnly">
<p>{i18n.YOU_ARE_VIEWING_NOTES(ariaRowindex)}</p>
</EuiScreenReaderOnly>
<NotePreviews timelineId={timelineId} notes={notes} />
<NotePreviews
timelineId={timelineId}
notes={notes}
showToggleEventDetailsAction={showToggleEventDetailsAction}
/>
</NotesContainer>
</NotePreviewsContainer>
) : null}
Expand Down
57 changes: 57 additions & 0 deletions .../security_solution/public/timelines/components/open_timeline/note_previews/index.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,63 @@ describe('NotePreviews', () => {
expect(wrapper.find('[data-test-subj="delete-note"] button').prop('disabled')).toBeFalsy();
});

test('should render toggle event details action by default', () => {
const timeline = mockTimelineResults[0];
(useDeepEqualSelector as jest.Mock).mockReturnValue(timeline);

const wrapper = mountWithI18nProvider(
<TestProviders>
<NotePreviews
notes={[
{
noteId: 'noteId1',
note: 'enabled delete',
savedObjectId: 'test-id',
updated: note2updated,
updatedBy: 'alice',
},
]}
showTimelineDescription
timelineId="test-timeline-id"
/>
</TestProviders>,
{
wrappingComponent: createReactQueryWrapper(),
}
);

expect(wrapper.find('[data-test-subj="notes-toggle-event-details"]').exists()).toBeTruthy();
});

test('should not render toggle event details action when showToggleEventDetailsAction is false ', () => {
const timeline = mockTimelineResults[0];
(useDeepEqualSelector as jest.Mock).mockReturnValue(timeline);

const wrapper = mountWithI18nProvider(
<TestProviders>
<NotePreviews
notes={[
{
noteId: 'noteId1',
note: 'enabled delete',
savedObjectId: 'test-id',
updated: note2updated,
updatedBy: 'alice',
},
]}
showTimelineDescription
timelineId="test-timeline-id"
showToggleEventDetailsAction={false}
/>
</TestProviders>,
{
wrappingComponent: createReactQueryWrapper(),
}
);

expect(wrapper.find('[data-test-subj="notes-toggle-event-details"]').exists()).toBeFalsy();
});

describe('Delete Notes', () => {
it('should delete note correctly', async () => {
const timeline = {
Expand Down
58 changes: 40 additions & 18 deletions ...ugins/security_solution/public/timelines/components/open_timeline/note_previews/index.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ const ToggleEventDetailsButtonComponent: React.FC<ToggleEventDetailsButtonProps>

return (
<EuiButtonIcon
data-test-subj="notes-toggle-event-details"
title={i18n.TOGGLE_EXPAND_EVENT_DETAILS}
aria-label={i18n.TOGGLE_EXPAND_EVENT_DETAILS}
color="text"
Expand Down Expand Up @@ -177,10 +178,32 @@ const NoteActions = React.memo<{
savedObjectId?: string | null;
confirmingNoteId?: string | null;
eventIdToNoteIds?: Record<string, string[]>;
}>(({ eventId, timelineId, noteId, confirmingNoteId, eventIdToNoteIds, savedObjectId }) => {
return eventId && timelineId ? (
<>
<ToggleEventDetailsButton eventId={eventId} timelineId={timelineId} />
showToggleEventDetailsAction?: boolean;
}>(
({
eventId,
timelineId,
noteId,
confirmingNoteId,
eventIdToNoteIds,
savedObjectId,
showToggleEventDetailsAction = true,
}) => {
return eventId && timelineId ? (
<>
{showToggleEventDetailsAction ? (
<ToggleEventDetailsButton eventId={eventId} timelineId={timelineId} />
) : null}
<DeleteNoteButton
noteId={noteId}
eventId={eventId}
confirmingNoteId={confirmingNoteId}
savedObjectId={savedObjectId}
timelineId={timelineId}
eventIdToNoteIds={eventIdToNoteIds}
/>
</>
) : (
<DeleteNoteButton
noteId={noteId}
eventId={eventId}
Expand All @@ -189,18 +212,9 @@ const NoteActions = React.memo<{
timelineId={timelineId}
eventIdToNoteIds={eventIdToNoteIds}
/>
</>
) : (
<DeleteNoteButton
noteId={noteId}
eventId={eventId}
confirmingNoteId={confirmingNoteId}
savedObjectId={savedObjectId}
timelineId={timelineId}
eventIdToNoteIds={eventIdToNoteIds}
/>
);
});
);
}
);

NoteActions.displayName = 'NoteActions';
/**
Expand All @@ -211,10 +225,11 @@ interface NotePreviewsProps {
notes?: TimelineResultNote[] | null;
timelineId?: string;
showTimelineDescription?: boolean;
showToggleEventDetailsAction?: boolean;
}

export const NotePreviews = React.memo<NotePreviewsProps>(
({ notes, timelineId, showTimelineDescription }) => {
({ notes, timelineId, showTimelineDescription, showToggleEventDetailsAction = true }) => {
const getTimeline = useMemo(() => timelineSelectors.getTimelineByIdSelector(), []);
const getTimelineNotes = useMemo(() => getTimelineNoteSelector(), []);
const timeline = useDeepEqualSelector((state) =>
Expand Down Expand Up @@ -288,6 +303,7 @@ export const NotePreviews = React.memo<NotePreviewsProps>(
savedObjectId={note.savedObjectId}
confirmingNoteId={timeline?.confirmingNoteId}
eventIdToNoteIds={eventIdToNoteIds}
showToggleEventDetailsAction={showToggleEventDetailsAction}
/>
),
timelineAvatar: (
Expand All @@ -299,7 +315,13 @@ export const NotePreviews = React.memo<NotePreviewsProps>(
),
};
}),
[eventIdToNoteIds, notes, timelineId, timeline?.confirmingNoteId]
[
eventIdToNoteIds,
notes,
timelineId,
timeline?.confirmingNoteId,
showToggleEventDetailsAction,
]
);

const commentList = useMemo(
Expand Down
65 changes: 35 additions & 30 deletions ...lugins/security_solution/public/timelines/components/timeline/properties/notes_flyout.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,10 @@

import React from 'react';
import {
EuiFlyout,
EuiFlyoutBody,
EuiFlyoutHeader,
EuiFlyoutResizable,
EuiOutsideClickDetector,
EuiTitle,
useGeneratedHtmlId,
} from '@elastic/eui';
Expand All @@ -32,7 +33,7 @@ export type NotesFlyoutProps = {
* z-index override is needed because otherwise NotesFlyout appears below
* Timeline Modal as they both have same z-index of 1000
*/
const NotesFlyoutContainer = styled(EuiFlyout)`
const NotesFlyoutContainer = styled(EuiFlyoutResizable)`
/*
* We want the width of flyout to be less than 50% of screen because
* otherwise it interferes with the delete notes modal
Expand All @@ -55,33 +56,37 @@ export const NotesFlyout = React.memo(function NotesFlyout(props: NotesFlyoutPro
}

return (
<NotesFlyoutContainer
ownFocus={false}
className="timeline-notes-flyout"
data-test-subj="timeline-notes-flyout"
onClose={onClose}
aria-labelledby={notesFlyoutTitleId}
maxWidth={750}
>
<EuiFlyoutHeader hasBorder>
<EuiTitle size="m">
<h2>{i18n.NOTES}</h2>
</EuiTitle>
</EuiFlyoutHeader>
<EuiFlyoutBody>
<NoteCards
ariaRowindex={0}
associateNote={associateNote}
className="notes-in-flyout"
data-test-subj="note-cards"
notes={notes}
showAddNote={true}
toggleShowAddNote={toggleShowAddNote}
eventId={eventId}
timelineId={timelineId}
onCancel={onCancel}
/>
</EuiFlyoutBody>
</NotesFlyoutContainer>
<EuiOutsideClickDetector onOutsideClick={onClose}>
<NotesFlyoutContainer
ownFocus={false}
className="timeline-notes-flyout"
data-test-subj="timeline-notes-flyout"
onClose={onClose}
aria-labelledby={notesFlyoutTitleId}
minWidth={500}
maxWidth={1400}
>
<EuiFlyoutHeader hasBorder>
<EuiTitle size="m">
<h2>{i18n.NOTES}</h2>
</EuiTitle>
</EuiFlyoutHeader>
<EuiFlyoutBody>
<NoteCards
ariaRowindex={0}
associateNote={associateNote}
className="notes-in-flyout"
data-test-subj="note-cards"
notes={notes}
showAddNote={true}
toggleShowAddNote={toggleShowAddNote}
eventId={eventId}
timelineId={timelineId}
onCancel={onCancel}
showToggleEventDetailsAction={false}
/>
</EuiFlyoutBody>
</NotesFlyoutContainer>
</EuiOutsideClickDetector>
);
});
36 changes: 34 additions & 2 deletions ...ity_solution/public/timelines/components/timeline/properties/use_notes_in_flyout.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,10 @@
*/

import React from 'react';
import { TimelineId } from '../../../../../common/types';
import { TimelineId, TimelineTabs } from '../../../../../common/types';
import { renderHook, act } from '@testing-library/react-hooks/dom';
import { createMockStore, mockGlobalState, TestProviders } from '../../../../common/mock';
import type { UseNotesInFlyoutArgs } from './use_notes_in_flyout';
import { useNotesInFlyout } from './use_notes_in_flyout';
import { waitFor } from '@testing-library/react';
import { useDispatch } from 'react-redux';
Expand Down Expand Up @@ -85,11 +86,13 @@ const refetchMock = jest.fn();

const renderTestHook = () => {
return renderHook(
() =>
(props?: Partial<UseNotesInFlyoutArgs>) =>
useNotesInFlyout({
eventIdToNoteIds: mockEventIdToNoteIds,
timelineId: TimelineId.test,
refetch: refetchMock,
activeTab: TimelineTabs.query,
...props,
}),
{
wrapper: ({ children }) => (
Expand Down Expand Up @@ -198,4 +201,33 @@ describe('useNotesInFlyout', () => {

expect(result.current.isNotesFlyoutVisible).toBe(false);
});

it('should close the flyout when activeTab is changed', () => {
const { result, rerender, waitForNextUpdate } = renderTestHook();

act(() => {
result.current.setNotesEventId('event-1');
});

act(() => {
result.current.showNotesFlyout();
});

expect(result.current.isNotesFlyoutVisible).toBe(true);

act(() => {
// no change in active Tab
rerender({ activeTab: TimelineTabs.query });
});

expect(result.current.isNotesFlyoutVisible).toBe(true);

act(() => {
rerender({ activeTab: TimelineTabs.eql });
});

waitForNextUpdate();

expect(result.current.isNotesFlyoutVisible).toBe(false);
});
});
15 changes: 12 additions & 3 deletions .../security_solution/public/timelines/components/timeline/properties/use_notes_in_flyout.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,18 @@
* 2.0.
*/

import { useCallback, useMemo, useState } from 'react';
import { useCallback, useEffect, useMemo, useState } from 'react';
import { useDispatch } from 'react-redux';
import type { TimelineTabs } from '../../../../../common/types';
import { useDeepEqualSelector } from '../../../../common/hooks/use_selector';
import { appSelectors } from '../../../../common/store';
import { timelineActions } from '../../../store';

interface UseNotesInFlyoutArgs {
export interface UseNotesInFlyoutArgs {
eventIdToNoteIds: Record<string, string[]>;
refetch?: () => void;
timelineId: string;
activeTab: TimelineTabs;
}

const EMPTY_STRING_ARRAY: string[] = [];
Expand All @@ -36,12 +38,19 @@ export const useNotesInFlyout = (args: UseNotesInFlyoutArgs) => {
setIsNotesFlyoutVisible(true);
}, []);

const { eventIdToNoteIds, refetch, timelineId } = args;
const { eventIdToNoteIds, refetch, timelineId, activeTab } = args;

const getNotesByIds = useMemo(() => appSelectors.notesByIdsSelector(), []);

const notesById = useDeepEqualSelector(getNotesByIds);

useEffect(() => {
if (activeTab) {
// if activeTab changes, close the notes flyout
closeNotesFlyout();
}
}, [activeTab, closeNotesFlyout]);

const dispatch = useDispatch();

const noteIds: string[] = useMemo(
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/security_solution/public/timelines/components/timeline/tabs/eql/index.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ export const EqlTabContentComponent: React.FC<Props> = ({
eventIdToNoteIds,
refetch,
timelineId,
activeTab: TimelineTabs.eql,
});

const onToggleShowNotes = useCallback(
Expand Down
Loading

0 comments on commit 309b907

Please sign in to comment.