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.

Sign up for GitHub

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

Jump to bottom

[Security Solution] flyout UI adjustment #108192

Merged
merged 13 commits into from
Aug 16, 2021
Merged

[Security Solution] flyout UI adjustment #108192

merged 13 commits into from
Aug 16, 2021

Conversation

angorayc
Copy link
Contributor

@angorayc angorayc commented Aug 11, 2021 •
edited
Loading

@angorayc angorayc added Team:Threat Hunting Security Solution Threat Hunting Team release_note:skip Skip the PR/issue when compiling release notes v7.15.0 v8.0.0 labels Aug 11, 2021
This was referenced Aug 11, 2021
Merged
Closed
@angorayc angorayc marked this pull request as ready for review August 13, 2021 10:24
@angorayc angorayc requested review from a team as code owners August 13, 2021 10:24
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

@angorayc
Copy link
Contributor Author

@elasticmachine merge upstream

michaelolo24
michaelolo24 reviewed Aug 16, 2021
View reviewed changes
.../plugins/security_solution/public/common/components/hover_actions/use_hover_action_items.tsx
const { browserFields } = useSourcererScope(activeScope);

const showFilters =
values != null && (enableOverflowButton || (!showTopN && !enableOverflowButton));
Copy link
Contributor

@michaelolo24 michaelolo24 Aug 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Can you put a comment here explaining the relationship between showTopN and enableOverflowButton? Just finding it hard to follow exactly why topN is chosen over the other actions. It would be helpful too for the overflowBtn component.. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Here is the comment I put, please let me know if there's better way to put it:

  • In the case of DisableOverflowButton, we show filters only when topN is NOT opened. As after topN button is clicked, the chart panel replace current hover actions in the hover actions' popover, so we have to hide all the actions.
  • in the case of EnableOverflowButton, we only need to hide all the items in the overflow popover as the chart's panel opens in the overflow popover, so non-overflowed actions are not affected.

Copy link
Contributor

@michaelolo24 michaelolo24 Aug 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Awesome, thank you! The wording is great 😄

michaelolo24
michaelolo24 reviewed Aug 16, 2021
View reviewed changes
x-pack/plugins/security_solution/public/timelines/components/side_panel/event_details/index.tsx Outdated
@@ -154,7 +154,7 @@ const EventDetailsPanelComponent: React.FC<EventDetailsPanelProps> = ({

return isFlyoutView ? (
<>
<EuiFlyoutHeader hasBorder>
<EuiFlyoutHeader hasBorder={isHostIsolationPanelOpen ? true : false}>
Copy link
Contributor

Choose a reason for hiding this comment

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

you can just pass hasBorder={isHostIsolationPanelOpen} assuming isHostIsolationPanelOpen is a boolean? Otherwise you can !!isHostIsolationPanelOpen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I can just put isHostIsolationPanelOpen 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Sweet, thanks!

@angorayc angorayc added the auto-backport Deprecated - use backport:version if exact versions are needed label Aug 16, 2021
@machadoum
Copy link
Member

LGTM 👏

@angorayc angorayc enabled auto-merge (squash) August 16, 2021 11:10
michaelolo24
michaelolo24 approved these changes Aug 16, 2021
View reviewed changes
Copy link
Contributor

@michaelolo24 michaelolo24 left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 2364 2365 +1
timelines 312 313 +1
total +2

Async chunks

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

id before after diff
securitySolution 6.5MB 6.5MB +3.7KB
timelines 391.8KB 398.2KB +6.5KB
total +10.2KB

Page load bundle

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

id before after diff
timelines 310.3KB 310.8KB +496.0B
Unknown metric groups

async chunk count

id before after diff
timelines 15 16 +1

History

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

@angorayc angorayc merged commit fe0322a into elastic:master Aug 16, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Aug 16, 2021
@angorayc @kibanamachine
[Security Solution] flyout UI adjustment (elastic#108192)
2963583 
* styling

* fix hover actions

* init overflow button for flyout

* init overflow button

* topN btn

* remove popover from topN

* fix tests

* fix unit test

* add use hover action items hook

* fix for code review

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine kibanamachine mentioned this pull request Aug 16, 2021
Merged
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
✅ 7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Aug 16, 2021
@kibanamachine @angorayc
[Security Solution] flyout UI adjustment (#108192) (#108677)
5e0a92f 
* styling

* fix hover actions

* init overflow button for flyout

* init overflow button

* topN btn

* remove popover from topN

* fix tests

* fix unit test

* add use hover action items hook

* fix for code review

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Angela Chuang <6295984+angorayc@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelolo24 michaelolo24 michaelolo24 approved these changes

Assignees
No one assigned
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting Security Solution Threat Hunting Team v7.15.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@angorayc @elasticmachine @machadoum @kibanamachine @michaelolo24