[SECURITY_SOLUTION] Trusted apps list expand/collapse details by efreeti · Pull Request #78601 · elastic/kibana

efreeti · 2020-09-28T14:08:12Z

Summary

Added a component to visualise trusted app as a card (with some generic components extracted out). Integrated card component into the list as expand/collapse functionality.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility

…e is more than one item.

...ck/plugins/security_solution/public/management/pages/trusted_apps/view/trusted_apps_list.tsx

kevinlog · 2020-09-28T20:37:22Z

initial pass through LGTM. I like the storybook and the code is written in a way to make change component data input easy.

elasticmachine · 2020-09-29T12:47:30Z

Pinging @elastic/endpoint-app-team (Feature:Endpoint)

elasticmachine · 2020-09-29T12:47:31Z

Pinging @elastic/endpoint-management (Team:Endpoint Management)

…bala/trusted-app-card

kevinlog · 2020-09-29T19:29:42Z

pulled it down and tried it out, flow feels good and table looks good!

x-pack/plugins/security_solution/public/common/components/item_details_card/index.tsx

kevinlog

LGTM

paul-tavares

Looks good.
Minor questions/comments - none of which should hold this up

paul-tavares · 2020-09-29T20:01:27Z

x-pack/plugins/security_solution/public/common/components/item_details_card/index.tsx

+  PropsForButton,
+} from '@elastic/eui';
+
+const OTHER_NODES = {};


You are only using this object as a key to the Map() right? If so, consider using a Symbol() instead.

Will do that as part of my next story.

Fixed this in a follow up PR.

paul-tavares · 2020-09-29T20:12:16Z

x-pack/plugins/security_solution/public/common/components/item_details_card/index.tsx

+
+ItemDetailsAction.displayName = 'ItemDetailsAction';
+
+export const ItemDetailsCard: FC = memo(({ children }) => {


Its the first time I'm seeing this type of component, so let just see if I got this right:

this Component can take in any type of children allowed by React.Children type, and then it groups those components up by Type (a specific React component type) and uses those groupings to actually build the returned card.

Do I have that right?

One other request. Because the approach taken with this component is so different from existing ones, can you add TS typings to the children prop so that consumers of this can have a bit of help understanding how to use it?

paul-tavares · 2020-09-29T20:19:48Z

...ck/plugins/security_solution/public/management/pages/trusted_apps/view/trusted_apps_list.tsx

+    changedMap[item.id] = (
+      <TrustedAppCard
+        trustedApp={item}
+        onDelete={() => {


is there a way you can memoize this callback?

Will do that as part of my next story, somehow slipped through.

paul-tavares · 2020-09-30T11:53:04Z

x-pack/plugins/security_solution/public/common/components/item_details_card/index.tsx

+export const ItemDetailsCard: FC = memo(({ children }) => {
+  const childElements = useMemo(
+    () => groupChildrenByType(children, [ItemDetailsPropertySummary, ItemDetailsAction]),
+    [children]


Is this really being memoized? I think children will always be new JSX, no?

I'll be honest I have no clue :D

paul-tavares

I posted another two comments, but still maintaint that none are reuqired or should block this PR from being merged. They can be followed up on post-merge.

kevinlog · 2020-09-30T16:37:19Z

merging for @efreeti

…c#78601)

kibanamachine · 2020-09-30T18:09:33Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 125f2ea

Metrics [docs]

@kbn/optimizer bundle module count

id	value	diff	baseline
`securitySolution`	1973	+3	1970

async chunks size

id	value	diff	baseline
`securitySolution`	10.2MB	+30.2KB	10.2MB

History

💛 Build #77891 was flaky 125f2ea
💔 Build #77833 failed 0c3ad5e
💚 Build #77807 succeeded 3ab219a
💔 Build #77427 failed bc1c6ca

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

* master: (97 commits) [Actions] Adds a "Test Connector" button on the Connectors List to make discovery of the Test tab easier (elastic#78746) [Discover] Fix functional time picker test permissions (elastic#78564) [ML] Fixing module datafeed overrides (elastic#78925) Adds some missing licenses to the CSV export (elastic#78719) [dev/cli] ensure plugins/ and all watch source dirs exist (elastic#78973) [Lens] Stop using scripted metric to collect telemetry (elastic#78687) [Lens] fix wrong message in fields accordion (elastic#78924) [Enterprise Search][App Search] Credentials Logic updates (elastic#78644) [Monitoring] Disk usage alerting (elastic#75419) [SECURITY_SOLUTION] Trusted apps list expand/collapse details (elastic#78601) Update content on interstitial page (elastic#78881) chore(NA): include hjson as a prod dependency (elastic#78941) Fix empty meta fields input in Advanced Settings (elastic#78576) [Lens] Maintain order of operations in dimension panel (elastic#78864) Fix plugin doc title (elastic#78880) load apm-rum agent lazily (elastic#78760) [ML] Skip full ML access permission test Optimize charts plugin (elastic#78922) ui_actions service initial docs (elastic#78902) skip failing suite (elastic#78942) ...

#78989) Co-authored-by: Bohdan Tsymbala <bohdan.tsymbala@elastic.co>

efreeti added 5 commits September 28, 2020 15:15

Added generic component for item details card.

4b923dc

Added table component that adds AndOrBadge on the left side when ther…

e322d72

…e is more than one item.

Added trusted app card component.

a81e9f4

Added expanding and collapsing trusted apps list item details.

bc1c6ca

Added tests for item details card.

a3d0018

kevinlog reviewed Sep 28, 2020

View reviewed changes

...ck/plugins/security_solution/public/management/pages/trusted_apps/view/trusted_apps_list.tsx Show resolved Hide resolved

efreeti added 6 commits September 29, 2020 11:39

Added tests for conditions table.

72845d5

Added tests for trusted app card.

5d5859e

Marked trusted apps list item expand button and updated snapshots.

2ce96dd

Added test for row expanding.

4211967

Fixed the warning about not key.

f68b5ff

Separate section for trusted apps component stories.

3ab219a

efreeti self-assigned this Sep 29, 2020

efreeti added Feature:Endpoint Elastic Endpoint feature release_note:skip Skip the PR/issue when compiling release notes Team:Endpoint Management v7.10.0 v8.0.0 labels Sep 29, 2020

efreeti marked this pull request as ready for review September 29, 2020 12:47

efreeti requested review from a team as code owners September 29, 2020 12:47

efreeti added 2 commits September 29, 2020 14:48

Merge branch 'master' of https://github.com/elastic/kibana into btsym…

0c3ad5e

…bala/trusted-app-card

Updated snapshot after pulling in master.

125f2ea

kevinlog reviewed Sep 29, 2020

View reviewed changes

x-pack/plugins/security_solution/public/common/components/item_details_card/index.tsx Show resolved Hide resolved

kevinlog approved these changes Sep 29, 2020

View reviewed changes

paul-tavares approved these changes Sep 29, 2020

View reviewed changes

paul-tavares reviewed Sep 30, 2020

View reviewed changes

paul-tavares approved these changes Sep 30, 2020

View reviewed changes

kevinlog merged commit b6db62e into elastic:master Sep 30, 2020

kevinlog changed the title ~~Trusted apps list expand/collapse details~~ [SECURITY_SOLUTION] Trusted apps list expand/collapse details Sep 30, 2020

kevinlog mentioned this pull request Sep 30, 2020

[7.x] [SECURITY_SOLUTION] Trusted apps list expand/collapse details (#78601) #78989

Merged

kevinlog pushed a commit to kevinlog/kibana that referenced this pull request Sep 30, 2020

[SECURITY_SOLUTION] Trusted apps list expand/collapse details (elasti…

5fc505e

…c#78601)

kevinlog added a commit that referenced this pull request Sep 30, 2020

[SECURITY_SOLUTION] Trusted apps list expand/collapse details (#78601) (

2efee0b

#78989) Co-authored-by: Bohdan Tsymbala <bohdan.tsymbala@elastic.co>


		ItemDetailsAction.displayName = 'ItemDetailsAction';

		export const ItemDetailsCard: FC = memo(({ children }) => {

Conversation

efreeti commented Sep 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

Uh oh!

kevinlog commented Sep 28, 2020

Uh oh!

elasticmachine commented Sep 29, 2020

Uh oh!

elasticmachine commented Sep 29, 2020

Uh oh!

kevinlog commented Sep 29, 2020

Uh oh!

Uh oh!

kevinlog left a comment

Choose a reason for hiding this comment

Uh oh!

paul-tavares left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paul-tavares left a comment

Choose a reason for hiding this comment

Uh oh!

kevinlog commented Sep 30, 2020

Uh oh!

kibanamachine commented Sep 30, 2020

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

async chunks size

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

efreeti commented Sep 28, 2020 •

edited

Loading