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

[SIEM] Fix Timeline footer styling #59587

Merged

Conversation

patrykkopycinski
Copy link
Contributor

Summary

Use EuiFlyoutFooter component in place of custom javascript useResizeObserver logic

Checklist

Delete any items that are not applicable to this PR.

For maintainers

…line-footer-styles

# Conflicts:
#	x-pack/legacy/plugins/siem/public/components/flyout/pane/index.tsx
@patrykkopycinski patrykkopycinski added Team:SIEM v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 labels Mar 6, 2020
@patrykkopycinski patrykkopycinski self-assigned this Mar 6, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/siem (Team:SIEM)

<TimelineKqlFetch id={id} indexPattern={indexPattern} inputId="timeline" />
{combinedQueries != null ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm seeing a change in behavior while desk testing (not necessarily related to this location) where the Timeline is populated by default, even on a fresh reload of the SIEM app.

As a result, Create new timeline appears to not work, because the timeline is populated with data even though there are no filters or KQL entered.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seeing the following in the JS console while desk testing (not necessarily related to this location) when clicking the Load more button:

react_devtools_backend.js:6 Warning: Encountered two children with the same key, `j2xwvXABOOUskGlPa61E`. Keys should be unique so that components maintain their identity across updates. Non-unique keys may cause children to be duplicated and/or omitted — the behavior is unsupported and could change in a future version.
    in div (created by EventsTbody)
    in EventsTbody (created by EventsComponent)
    in EventsComponent
    in div (created by EventsTable)
    in EventsTable
    in div (created by TimelineBody)
    in TimelineBody
    in Unknown
    in Unknown
    in Unknown (created by ConnectFunction)
    in ConnectFunction (created by Query)
    in div (created by EuiFlyoutBody)
    in div (created by EuiFlyoutBody)
    in div (created by EuiFlyoutBody)
    in EuiFlyoutBody (created by StyledEuiFlyoutBody)
    in StyledEuiFlyoutBody (created by Query)
    in ManageTimelineContextComponent (created by Query)
    in Query (created by TimelineQueryComponent)
    in TimelineQueryComponent (created by EnhancedType)
    in EnhancedType (created by ConnectFunction)
    in ConnectFunction (created by TimelineComponent)
    in div (created by TimelineContainer)
    in TimelineContainer (created by TimelineComponent)
    in TimelineComponent (created by Query)
    in Query
    in Unknown
    in Unknown
    in Unknown (created by ConnectFunction)
    in ConnectFunction
    in div (created by Resizable)
    in Resizable (created by StyledResizable)
    in StyledResizable (created by FlyoutPaneComponent)
    in div (created by EuiFlyout)
    in div (created by FocusLock)
    in FocusLock (created by EuiFocusTrap)
    in div (created by OutsideEventDetector)
    in OutsideEventDetector (created by EuiFocusTrap)
    in EuiOutsideClickDetector (created by EuiFocusTrap)
    in EuiFocusTrap (created by EuiFlyout)
    in EuiFlyout (created by FlyoutPaneComponent)
    in div (created by EuiFlyoutContainer)
    in EuiFlyoutContainer (created by FlyoutPaneComponent)
    in FlyoutPaneComponent
    in div (created by Visible)
    in Visible
    in Unknown (created by ConnectFunction)
    in ConnectFunction (created by Query)
    in Provider (created by App)
    in App (created by ErrorBoundary)
    in ErrorBoundary (created by DragDropContext)
    in DragDropContext
    in Unknown
    in Unknown (created by ConnectFunction)
    in ConnectFunction (created by Query)
    in Query
    in Unknown (created by HomePage)
    in main (created by Main)
    in Main (created by HomePage)
    in div (created by WrappedByAutoSizer)
    in WrappedByAutoSizer (created by HomePage)
    in HomePage (created by PageRouterComponent)
    in Route (created by PageRouterComponent)
    in Switch (created by PageRouterComponent)
    in Router (created by PageRouterComponent)
    in ManageRoutesSpyComponent (created by PageRouterComponent)
    in PageRouterComponent (created by AppPluginRootComponent)
    in Unknown (created by AppPluginRootComponent)
    in ThemeProvider (created by AppPluginRootComponent)
    in ApolloProvider (created by AppPluginRootComponent)
    in Provider (created by AppPluginRootComponent)
    in ManageGlobalToaster (created by AppPluginRootComponent)
    in AppPluginRootComponent (created by StartAppComponent)
    in EuiContext (created by I18nContext)
    in PseudoLocaleWrapper (created by I18nProvider)
    in IntlProvider (created by I18nProvider)
    in I18nProvider (created by I18nContext)
    in I18nContext (created by StartAppComponent)
    in EuiErrorBoundary (created by StartAppComponent)
    in StartAppComponent (created by SiemAppComponent)
    in Provider (created by SiemAppComponent)
    in SiemAppComponent

Copy link
Contributor

Choose a reason for hiding this comment

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

Seeing an issue (not necessarily related to this location) where items dragged from the timeline "fall behind" the header, per the following screenshot:

falls-behind-while-dragging

Copy link
Contributor

Choose a reason for hiding this comment

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

Fields in expanded events also fall behind, per the following gif:
expanded-fields-fall-behind

Copy link
Contributor

Choose a reason for hiding this comment

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

Seeing an issue where the columns in the Events table cannot be re-ordered, per the following gif:

cannot-re-order-events-columns

Copy link
Contributor

Choose a reason for hiding this comment

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

Seeing an issue in the Events table where a column cannot be added via D&D from an expanded event, per the following gif:

events-cannot-drag-column-from-expanded-event

.find('[data-test-subj="timeline-title"]')
.first()
.props().placeholder
).toContain('Untitled Timeline');
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an equivalent version of this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

const handleResizeStop: ResizeCallback = (e, direction, ref, delta) => {
onColumnResized({ columnId: header.id, delta: delta.width });
};
const resizableEnable = useMemo(() => ({ right: true }), []);
Copy link
Contributor

Choose a reason for hiding this comment

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

This pure function doesn't appear to capture any variables via closure, (as opposed to for example, resizableSize below it).

Consider declaring resizableEnable outside the scope of this component, because it doesn't appear that every instance of the component needs its own memoized version of resizableEnable.

@andrew-goldstein
Copy link
Contributor

Seeing the following error when clicking the Load more button:

Warning: Encountered two children with the same key, `884d7db499eb0ab0b7f4680279edecd3e9bbf8a04a0ff4e9b42d6dedd8d04a45`. Keys should be unique so that components maintain their identity across updates. Non-unique keys may cause children to be duplicated and/or omitted — the behavior is unsupported and could change in a future version.
    in div (created by EventsTbody)
    in EventsTbody (created by EventsComponent)
    in EventsComponent
    in div (created by EventsTable)
    in EventsTable
    in div (created by TimelineBody)
    in TimelineBody
    in Unknown
    in Unknown
    in Unknown (created by ConnectFunction)
    in ConnectFunction (created by Query)
    in div (created by EuiFlyoutBody)
    in div (created by EuiFlyoutBody)
    in div (created by EuiFlyoutBody)
    in EuiFlyoutBody (created by StyledEuiFlyoutBody)
    in StyledEuiFlyoutBody (created by Query)
    in ManageTimelineContextComponent (created by Query)
    in Query (created by TimelineQueryComponent)
    in TimelineQueryComponent (created by EnhancedType)
    in EnhancedType (created by ConnectFunction)
    in ConnectFunction (created by TimelineComponent)
    in div (created by TimelineContainer)
    in TimelineContainer (created by TimelineComponent)
    in TimelineComponent (created by Query)
    in Query
    in Unknown
    in Unknown
    in Unknown (created by ConnectFunction)
    in ConnectFunction
    in div (created by Resizable)
    in Resizable (created by StyledResizable)
    in StyledResizable (created by FlyoutPaneComponent)
    in div (created by EuiFlyout)
    in div (created by FocusLock)
    in FocusLock (created by EuiFocusTrap)
    in div (created by OutsideEventDetector)
    in OutsideEventDetector (created by EuiFocusTrap)
    in EuiOutsideClickDetector (created by EuiFocusTrap)
    in EuiFocusTrap (created by EuiFlyout)
    in EuiFlyout (created by FlyoutPaneComponent)
    in div (created by EuiFlyoutContainer)
    in EuiFlyoutContainer (created by FlyoutPaneComponent)
    in FlyoutPaneComponent
    in div (created by Visible)
    in Visible
    in Unknown (created by ConnectFunction)
    in ConnectFunction (created by Query)
    in Provider (created by App)
    in App (created by ErrorBoundary)
    in ErrorBoundary (created by DragDropContext)
    in DragDropContext
    in Unknown
    in Unknown (created by ConnectFunction)
    in ConnectFunction (created by Query)
    in Query
    in Unknown (created by HomePage)
    in main (created by Main)
    in Main (created by HomePage)
    in div (created by WrappedByAutoSizer)
    in WrappedByAutoSizer (created by HomePage)
    in HomePage (created by PageRouterComponent)
    in Route (created by PageRouterComponent)
    in Switch (created by PageRouterComponent)
    in Router (created by PageRouterComponent)
    in ManageRoutesSpyComponent (created by PageRouterComponent)
    in PageRouterComponent (created by AppPluginRootComponent)
    in Unknown (created by AppPluginRootComponent)
    in ThemeProvider (created by AppPluginRootComponent)
    in ApolloProvider (created by AppPluginRootComponent)
    in Provider (created by AppPluginRootComponent)
    in ManageGlobalToaster (created by AppPluginRootComponent)
    in AppPluginRootComponent (created by StartAppComponent)
    in EuiContext (created by I18nContext)
    in PseudoLocaleWrapper (created by I18nProvider)
    in IntlProvider (created by I18nProvider)
    in I18nProvider (created by I18nContext)
    in I18nContext (created by StartAppComponent)
    in EuiErrorBoundary (created by StartAppComponent)
    in StartAppComponent (created by SiemAppComponent)
    in Provider (created by SiemAppComponent)
    in SiemAppComponent

@andrew-goldstein
Copy link
Contributor

When resizing a timeline with many filters like the screenshot below, performance in Firefox feels like it's dipped from "sluggish" in master to "nearly unresponsive":

many-filters

@andrew-goldstein
Copy link
Contributor

When adding notes, the note area is taking up the entire width instead of just the visible area. As a result, the Add note and Cancel actions are cut off, per the screenshot below:

add-note-cancel-cut-off

@patrykkopycinski
Copy link
Contributor Author

@elasticmachine merge upstream


export const useThrottledResizeObserver = (wait = 100) => {
const [size, setSize] = useState<{ width: number; height: number }>({ width: 0, height: 0 });
const onResize = useMemo(() => throttle(wait, setSize), [wait]);
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible to use useCallback here?

Copy link
Contributor

@andrew-goldstein andrew-goldstein left a comment

Choose a reason for hiding this comment

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

Thanks @patrykkopycinski for all the hard work that went into the fixes and refactoring in this PR! 🙏
Cross-browser desk testing in Chrome 80.0.3987.132, Firefox 74.0, and Safari 13.0.5 is looking solid 👍👍👍
LGTM 🚀

@patrykkopycinski
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@patrykkopycinski patrykkopycinski merged commit 55003b6 into elastic:master Mar 17, 2020
@patrykkopycinski patrykkopycinski deleted the fix/siem-timeline-footer-styles branch March 17, 2020 10:50
patrykkopycinski added a commit to patrykkopycinski/kibana that referenced this pull request Mar 17, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 17, 2020
* master: (30 commits)
  [TSVB] fix text color when using custom background color (elastic#60261)
  Fix import to timefilter from in TSVB (elastic#60296)
  [NP] Get rid of usage redirectWhenMissing service (elastic#59777)
  [SIEM] Fix Timeline footer styling (elastic#59587)
  [ML] Fixes to error handling for analytics jobs and file data viz (elastic#60249)
  Give better stack traces for Unhandled Promise Rejection warnings (elastic#60235)
  resolves elastic#58905 (elastic#60120)
  Added variables button for text fields in Pagerduty component. (elastic#60189)
  adds test that action vars are rendered for alert action parms (elastic#60310)
  Closes 59786 by removing the update toast (elastic#60172)
  [EPM] Packages list tabs (elastic#60167)
  Added message variables button for Webhook body form field (elastic#60174)
  Revert "adds new test (elastic#60064)"
  [Maps] move MapSavedObject type out of telemetry (elastic#60127)
  [Reporting] Fix error handling for job handler in route (elastic#60161)
  [Endpoint] TEST: verify alerts page header says 'Alerts' (elastic#60206)
  EMT-248: implement ack resource to accept event payload to acknowledge agent actions (elastic#60218)
  Migrate dual validated range (elastic#59689)
  Embeddable triggers (elastic#58440)
  [Endpoint] Sample data generator CLI script (elastic#59952)
  ...
cnasikas pushed a commit to cnasikas/kibana that referenced this pull request Mar 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:SIEM v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants