Skip to content
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] 7.11 Timeline EVOLUTION #83378

Merged

Conversation

patrykkopycinski
Copy link
Contributor

@patrykkopycinski patrykkopycinski commented Nov 13, 2020

Summary

  • Timeline is always a full screen modal
  • When Timeline is open, the details of an event shall be displayed in the Event Details section of the split pane
  • Timeline's Events Table (grid view) was updated to support row-selection, to indicate which event in the table is selected
  • Added Persistent Timeline Taskbar (single document interface)
  • Header Updates / Call to Action
  • The Attach timeline to new | existing case actions have been migrated to a new always-visible "Call to Action" button, to encourage these workflows
  • The "lock" icon is replaced by a Sync time with current page toggle switch
  • The "star" icon is replaced by a Add to Favorites toggle button
  • Convert the Timeline body to a fixed-width split pane (w/header)

This PR is focused on layout changes, accessibility improvements will be addressed in the followup PR

Checklist

patrykkopycinski and others added 24 commits October 27, 2020 17:30
…-modal-full-width

# Conflicts:
#	x-pack/plugins/security_solution/public/timelines/components/timeline/body/events/index.tsx
#	x-pack/plugins/security_solution/public/timelines/components/timeline/body/events/stateful_event.tsx
@tylersmalley
Copy link
Contributor

@elasticmachine merge upstream

@patrykkopycinski patrykkopycinski changed the title Feat/timeline modal layout [Security Solution] 7.11 Timeline EVOLUTION Dec 3, 2020
({ browserFields, columnHeaders, data, eventId, onUpdateColumns, timelineId, toggleColumn }) => {
({ browserFields, data, eventId, timelineId }) => {
const dispatch = useDispatch();
const getTimeline = useMemo(() => timelineSelectors.getTimelineByIdSelector(), []);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading more and more your PR, I agree with you we should get more generic selector, the same way that we did/fix it for sourcerer.

export const JsonView = React.memo<Props>(({ data }) => {
const value = useMemo(
() =>
JSON.stringify(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit -> we should have a try catch just in case that the stringify blow out


const toggleColumn = useCallback(
(column: ColumnHeaderOptions) => {
const exists = columnHeaders.findIndex((c) => c.id === column.id) !== -1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit ->

if (columnHeaders.some((c) => c.id === column.id)) {
          dispatch(
            timelineActions.removeColumn({
              columnId: column.id,
              id: timelineId,
            })
          );
        } else {
          dispatch(
            timelineActions.upsertColumn({
              column,
              id: timelineId,
              index: 1,
            })
          );
        }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by the way we had the same code in solution/public/common/components/event_details/event_fields_browser.tsx

@XavierM
Copy link
Contributor

XavierM commented Dec 5, 2020

with the timeline in footer, I think we should disable the fixed header when the size of the screen is too small.

image

@XavierM
Copy link
Contributor

XavierM commented Dec 5, 2020

I have this user feeling when I open a timeline, it is slower than before. Do you have the same feeling? if you do we should keep this in mind and fix it in another PR.

Copy link
Contributor

@XavierM XavierM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed all the lines of this PR and I played/tested the timeline and other features from the app. Everything seems to work as expected. I think we should merge it and see if other people are discovering issue that I missed.

There is a lot improvement in this PR thanks to you @patrykkopycinski and you are also giving a revolution to timeline. We/I appreciate all this work.

Would you mind creating a ticket about improving our timeline selector so we do not have to get the full timeline object and memoized specific attributes. Like you say, it will improve timeline.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 2104 2105 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 8.0MB 8.0MB +14.0KB

Distributable file count

id before after diff
default 47650 47659 +9

Page load bundle

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

id before after diff
securitySolution 210.3KB 210.2KB -130.0B
Unknown metric groups

async chunk count

id before after diff
securitySolution 19 22 +3

History

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

@patrykkopycinski patrykkopycinski merged commit 2f386e8 into elastic:master Dec 5, 2020
@patrykkopycinski patrykkopycinski deleted the feat/timeline-modal-layout branch December 5, 2020 15:04
patrykkopycinski added a commit to patrykkopycinski/kibana that referenced this pull request Dec 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants