[SIEM] Fix Inspect query 'request timestamp' value changes when curso… #54223

patrykkopycinski · 2020-01-08T08:09:20Z

…r leaves modal

Summary

Switched from useState based approach to css, also extracted logic to a separate component, so there is no more need to set up state and callbacks in every component
Refactored couple components to a new approach for React.memo to properly propagate displayName
Fixed https://github.com/elastic/kibana/pull/54223/files#diff-203bd20c5fdd5ee6229bc376a8a586b9L23 so now the icon will be hidden by default, ; was missing

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

…r leaves modal

elasticmachine · 2020-01-08T08:09:22Z

Pinging @elastic/siem (Team:SIEM)

…ect-modal-rerender

rylnd

Great stuff! 👍to the CSS approach for showing/hiding, that's much cleaner.

Fixing the "hide until hovered" behavior on those buttons is huge! There's much less visual clutter on our dashboards now (although our panels look a little bare now! 😦).

I think we could move those CSS unit tests into dedicated InspectButtonContainer unit tests, but other than that this is awesome. Thanks for updating those components re: displayName as you touched them, too. I'll try to do the same.

One other thing I noticed in review: the timestamp value still changes when the browser is resized. I'm not sure whether this is expected (do we have some kind of global re-render on window resize?) or whether we want to fix it, but I figured it was worth mentioning.

x-pack/package.json

rylnd · 2020-01-08T21:52:22Z

x-pack/legacy/plugins/siem/public/components/events_viewer/index.test.tsx

-        .first()
-        .exists()
-    ).toBe(true);
+    expect(wrapper.find(`InspectButtonContainer`)).toHaveStyleRule('opacity', '0', {


It feels like these tests should live in inspect.index.test.tsx, no? If we truly want a test for the integration here, I would just keep the exists() check.

Then we won't need to pass around BUTTON_CLASS, either.

I was thinking about that as well, please let me know if it was done how you thought so :)

Looks great! I'd love to get rid of the BUTTON_CLASS constant entirely but I couldn't immediately think of a way to get that to work. I was hoping we could use the css helper described here, but no such luck.

…ect-modal-rerender # Conflicts: # x-pack/legacy/plugins/siem/public/components/inspect/index.tsx # x-pack/legacy/plugins/siem/public/components/stat_items/__snapshots__/index.test.tsx.snap

rylnd

@patrykkopycinski things look good, thanks for making these changes. Just a summary of what's left:

yarn.lock changes
The issue I mentioned earlier about timestamp changing on resize. Is that expected? Do we care about that?

patrykkopycinski · 2020-01-13T18:24:04Z

@rylnd Fixed yarn.lock issue. In terms of resizing there is a PR open for refactoring AutoSizer to hooks, which should fix the issue with unnecessary re-renders #47120

rylnd

Sounds good RE the Autosizer stuff. The rerender only happens on some pages, and even then it's not something we'd expect to happen all the time. 👍

patrykkopycinski · 2020-01-13T21:05:55Z

@elasticmachine merge upstream

kibanamachine · 2020-01-13T22:21:17Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 2ddb60d

History

💚 Build #19855 succeeded 3402b7c
💔 Build #19762 failed 31a69a9
💚 Build #18815 succeeded 1acf6bb
💔 Build #18800 failed 82c7d8f
💔 Build #18789 failed cb3b850

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

#54223) (#54685)

* upstream/master: (26 commits) Take page offset into account too (elastic#54567) [APM] Support error.{log,exception}.stacktrace.classname (elastic#54577) Np migration tsvb route validation (elastic#51850) [ML] MML calculator enhancements for multi-metric job wizard (elastic#54573) [SIEM] Fix Inspect query 'request timestamp' value changes when curso… (elastic#54223) Fix chromeless NP apps not using full page width (elastic#54550) Remove extraneous public import to prevent failing Kibana startup (elastic#54676) [Uptime] Temporarily skip flakey tests (elastic#54675) Skip failing uptime tests Create UI for alerting and actions plugin (elastic#48959) [dev/build/sass] build stylesheets for disabled plugins too (elastic#54654) [SIEM] Use bulk actions API when updating or deleting rules (elastic#54521) Support "Deprecated" label in advanced settings (elastic#54539) [Maps] add text halo color and width style properties (elastic#53827) Service Map Data API at Runtime (elastic#54027) [SIEM] Detection Engine Create Rule Design Review #1 (elastic#54442) Skip flaky test [Canvas] Enable Embeddable maps (elastic#53971) [SIEM][Detection Engine] Increases the number or rules you can view on a single page (elastic#54628) uiSettings - use validation field for image field maxSize (elastic#54522) ...

elastic#54223)

[SIEM] Fix Inspect query 'request timestamp' value changes when curso…

2c1262d

…r leaves modal

patrykkopycinski added Team:SIEM v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.6.0 labels Jan 8, 2020

patrykkopycinski requested a review from XavierM January 8, 2020 08:09

patrykkopycinski self-assigned this Jan 8, 2020

patrykkopycinski added 4 commits January 8, 2020 12:03

Merge branch 'master' of github.com:elastic/kibana into fix/siem-insp…

350b0ea

…ect-modal-rerender

Fix import

dcb700b

[SIEM] Use css approach

2564efe

fix styling

ae450d3

patrykkopycinski requested a review from spong January 8, 2020 14:04

patrykkopycinski added 5 commits January 8, 2020 16:18

Merge branch 'master' of github.com:elastic/kibana into fix/siem-insp…

fec5f3e

…ect-modal-rerender

revert snapshots

7634187

fix unit tests

cb3b850

update snapshots

82c7d8f

update styled-components

1acf6bb

patrykkopycinski requested review from rylnd, stephmilovic, FrankHassanabad and andrew-goldstein January 8, 2020 20:56

patrykkopycinski assigned angorayc Jan 8, 2020

patrykkopycinski requested a review from angorayc January 8, 2020 21:00

rylnd requested changes Jan 8, 2020

View reviewed changes

patrykkopycinski added 2 commits January 13, 2020 15:14

Merge branch 'master' of github.com:elastic/kibana into fix/siem-insp…

8c2cb5c

…ect-modal-rerender # Conflicts: # x-pack/legacy/plugins/siem/public/components/inspect/index.tsx # x-pack/legacy/plugins/siem/public/components/stat_items/__snapshots__/index.test.tsx.snap

PR comments

31a69a9

rylnd reviewed Jan 13, 2020

View reviewed changes

yarn.lock

3402b7c

rylnd approved these changes Jan 13, 2020

View reviewed changes

Merge branch 'master' into fix/siem-inspect-modal-rerender

2ddb60d

patrykkopycinski merged commit 7c4a531 into elastic:master Jan 14, 2020

patrykkopycinski deleted the fix/siem-inspect-modal-rerender branch January 14, 2020 07:50

patrykkopycinski mentioned this pull request Jan 14, 2020

[7.x] [SIEM] Fix Inspect query 'request timestamp' value changes when curso… (#54223) #54685

Merged

patrykkopycinski added a commit that referenced this pull request Jan 14, 2020

[SIEM] Fix Inspect query 'request timestamp' value changes when curso… (

27d6925

#54223) (#54685)

jkelastic pushed a commit to jkelastic/kibana that referenced this pull request Jan 17, 2020

[SIEM] Fix Inspect query 'request timestamp' value changes when curso… (

86f1fd4

elastic#54223)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SIEM] Fix Inspect query 'request timestamp' value changes when curso… #54223

[SIEM] Fix Inspect query 'request timestamp' value changes when curso… #54223

patrykkopycinski commented Jan 8, 2020 •

edited

Loading

elasticmachine commented Jan 8, 2020

rylnd left a comment

rylnd Jan 8, 2020

rylnd Jan 8, 2020

patrykkopycinski Jan 13, 2020

rylnd Jan 13, 2020

rylnd left a comment

patrykkopycinski commented Jan 13, 2020

rylnd left a comment

patrykkopycinski commented Jan 13, 2020

kibanamachine commented Jan 13, 2020

[SIEM] Fix Inspect query 'request timestamp' value changes when curso… #54223

[SIEM] Fix Inspect query 'request timestamp' value changes when curso… #54223

Conversation

patrykkopycinski commented Jan 8, 2020 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Jan 8, 2020

rylnd left a comment

Choose a reason for hiding this comment

rylnd Jan 8, 2020

Choose a reason for hiding this comment

rylnd Jan 8, 2020

Choose a reason for hiding this comment

patrykkopycinski Jan 13, 2020

Choose a reason for hiding this comment

rylnd Jan 13, 2020

Choose a reason for hiding this comment

rylnd left a comment

Choose a reason for hiding this comment

patrykkopycinski commented Jan 13, 2020

rylnd left a comment

Choose a reason for hiding this comment

patrykkopycinski commented Jan 13, 2020

kibanamachine commented Jan 13, 2020

💚 Build Succeeded

History

patrykkopycinski commented Jan 8, 2020 •

edited

Loading