[SECURITY_SOLUTION][ENDPOINT] Grid view for trusted apps. #79485

efreeti · 2020-10-05T15:23:04Z

Summary

Implemented grid view for trusted apps and toggling between views.

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

… would be easy to introduce a new view type parameter.

…bala/grid-view

…bala/grid-view-1

...curity_solution/public/management/pages/trusted_apps/view/components/control_panel/index.tsx

paul-tavares · 2020-10-05T18:44:35Z

...ty_solution/public/management/pages/trusted_apps/view/components/trsuted_apps_grid/index.tsx

+  const isLoading = useTrustedAppsSelector(isListLoading);
+  const error = useTrustedAppsSelector(getListErrorMessage);
+
+  const handleTrustedAppDelete = useTrustedAppsStoreActionCallback((trustedApp) => ({


Why did we need custom hooks for these? why not just use useCallback()?

I resolved all other conversations and let's keep one thread here.

Cause my feeling is that code using pure useCallback to do history navigation and redux action dispatch becomes unnecessarily bulky (prettier is not making it prettier :) )

I would appreciate if we try this out. The whole goal is to write concise code that is not hard to navigate. Currently one single callback that doesn't do much more than navigating url by changing parameters, occupies more than 6 lines.

I also don't like the idea of having to disable the eslint rules, but I've googled for this quite a bit and there are similar stories like this on the git hub page for this rule. In the end we disable eslint rules for even more minor cases which cost almost no effort to do correctly.

If you are worried about memoization of variables from lexical scope then it's true that you can use those in memoization and I could add dependencies list, but I don't think that's the goal of this hook to depend on scope - all that could come from scope is abstracted away (I mean current location) so that you are not supposed to worry about that. It should only get params from event and use those.

In the end this is like ~10 lines of code using this hook? and only in trusted apps (which is pretty small codebase). If it doesn't work out it's an half an hour job (if not less) to replace them.

@efreeti my preference leans towards using useCallback() since it's an established pattern and a built in hook, so there's some familiarity as well. Unless there's a strong reason to do otherwise, I don't see a need to break from the pattern just yet.

Also, since we're right on the line and we'd like to ship this in 7.10, I'd suggest we save the conversation for later. You've built a great view here - let's get it in!

@paul-tavares what are your thoughts?

...ty_solution/public/management/pages/trusted_apps/view/components/trsuted_apps_grid/index.tsx

...ity_solution/public/management/pages/trusted_apps/view/components/view_type_toggle/index.tsx

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

x-pack/plugins/security_solution/public/management/pages/trusted_apps/view/hooks.ts

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

…bala/grid-view-1 � Conflicts: � x-pack/plugins/security_solution/public/management/pages/trusted_apps/view/components/trusted_app_card/index.stories.tsx

elasticmachine · 2020-10-05T21:07:58Z

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

elasticmachine · 2020-10-05T21:08:00Z

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

…bala/grid-view-1 � Conflicts: � x-pack/plugins/security_solution/public/management/pages/trusted_apps/view/trusted_apps_page.test.tsx

paul-tavares · 2020-10-06T13:43:39Z

...y_solution/public/management/pages/trusted_apps/view/components/control_panel/index.test.tsx

+ * you may not use this file except in compliance with the Elastic License.
+ */
+import { render } from '@testing-library/react';
+import { shallow } from 'enzyme';


Maybe consider using only one testing library per test module and not have a mix of the two.

I will see how to resolve this. Enzyme has shallow, render doesn't. But with shallow you can't do the clicking. I will follow up next on this. Will create Debt issue.

kibanamachine · 2020-10-06T15:01:12Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: b113884

Metrics [docs]

@kbn/optimizer bundle module count

id	before	after	diff
`securitySolution`	2000	2003	+3

async chunks size

id	before	after	diff
`securitySolution`	10.4MB	10.4MB	+14.4KB

History

💚 Build #80085 succeeded cb9513d
💔 Build #80077 failed 801aa67
💔 Build #79909 failed dd83c98
💔 Build #79894 failed 965be2e
💔 Build #79679 failed 4796146

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

) * Refactored store code to group properties related to location so that would be easy to introduce a new view type parameter. * Added view type to the location and routing. * Added a simple hook to make navigation easier. * Improved the navigation hook to get params. * Some fix for double notification after creating trusted app. * Added a hook to perform trusted app store actions. * Fixed trusted app card delete callback. * Added grid view. * Fixed the stories structuring. * Shared more logic between grid and list. * Finalized the grid view. * Flattened the props. * Improved memoization. * Moved the flex item elements inside conditions. * Fixed broken stories. * Updated the snapshot. * Updated the snapshot.

…79704) * Refactored store code to group properties related to location so that would be easy to introduce a new view type parameter. * Added view type to the location and routing. * Added a simple hook to make navigation easier. * Improved the navigation hook to get params. * Some fix for double notification after creating trusted app. * Added a hook to perform trusted app store actions. * Fixed trusted app card delete callback. * Added grid view. * Fixed the stories structuring. * Shared more logic between grid and list. * Finalized the grid view. * Flattened the props. * Improved memoization. * Moved the flex item elements inside conditions. * Fixed broken stories. * Updated the snapshot. * Updated the snapshot.

efreeti added 14 commits October 1, 2020 11:30

Refactored store code to group properties related to location so that…

ad1bce8

… would be easy to introduce a new view type parameter.

Pulled in master.

13bd9da

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

8b8fd1a

…bala/grid-view

Added view type to the location and routing.

3fcc3c6

Added a simple hook to make navigation easier.

2ae610d

Improved the navigation hook to get params.

49098be

Some fix for double notification after creating trusted app.

1c09a9d

Added a hook to perform trusted app store actions.

88d200c

Fixed trusted app card delete callback.

403a841

Added grid view.

b5d8235

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

3abe17a

…bala/grid-view-1

Fixed the stories structuring.

4796146

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

3b91233

…bala/grid-view-1

Shared more logic between grid and list.

d44b394

paul-tavares reviewed Oct 5, 2020

View reviewed changes

efreeti added 5 commits October 5, 2020 22:22

Finalized the grid view.

a503cdd

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

4757d70

…bala/grid-view-1 � Conflicts: � x-pack/plugins/security_solution/public/management/pages/trusted_apps/view/components/trusted_app_card/index.stories.tsx

Flattened the props.

6ca1729

Improved memoization.

8132c54

Moved the flex item elements inside conditions.

965be2e

efreeti self-assigned this Oct 5, 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 Oct 5, 2020

efreeti marked this pull request as ready for review October 5, 2020 21:07

efreeti requested review from a team as code owners October 5, 2020 21:07

Fixed broken stories.

dd83c98

efreeti changed the title ~~Btsymbala/grid view~~ [ENDPOINT] Grid view for trusted apps. Oct 5, 2020

efreeti changed the title ~~[ENDPOINT] Grid view for trusted apps.~~ [SECURITY_SOLUTION][ENDPOINT] Grid view for trusted apps. Oct 5, 2020

efreeti added 3 commits October 6, 2020 14:19

Updated the snapshot.

801aa67

Updated the snapshot.

cb9513d

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

b113884

…bala/grid-view-1 � Conflicts: � x-pack/plugins/security_solution/public/management/pages/trusted_apps/view/trusted_apps_page.test.tsx

paul-tavares approved these changes Oct 6, 2020

View reviewed changes

kevinlog approved these changes Oct 6, 2020

View reviewed changes

efreeti merged commit f340d71 into elastic:master Oct 6, 2020

efreeti deleted the btsymbala/grid-view branch October 6, 2020 15:31

efreeti mentioned this pull request Oct 6, 2020

[7.x] [SECURITY_SOLUTION][ENDPOINT] Grid view for trusted apps. (#79485) #79704

Merged

efreeti mentioned this pull request Oct 12, 2020

[ENDPOINT][TECH DEBT] Simplify the code to do history navigation in trusted apps (and other sections) #80143

Open

[SECURITY_SOLUTION][ENDPOINT] Grid view for trusted apps. #79485

[SECURITY_SOLUTION][ENDPOINT] Grid view for trusted apps. #79485

Uh oh!

Conversation

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

Uh oh!

Summary

Checklist

Uh oh!

Uh oh!

Uh oh!

paul-tavares Oct 5, 2020

Choose a reason for hiding this comment

Uh oh!

efreeti Oct 5, 2020

Choose a reason for hiding this comment

Uh oh!

kevinlog Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elasticmachine commented Oct 5, 2020

Uh oh!

elasticmachine commented Oct 5, 2020

Uh oh!

paul-tavares Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

efreeti Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Oct 6, 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 Oct 5, 2020 •

edited

Loading