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

[ENDPOINT] Reintroduced tabs to endpoint management and migrated pages to use common security components #74886

Merged
merged 16 commits into from
Aug 18, 2020
Merged

[ENDPOINT] Reintroduced tabs to endpoint management and migrated pages to use common security components #74886

merged 16 commits into from
Aug 18, 2020

Conversation

efreeti
Copy link
Contributor

@efreeti efreeti commented Aug 12, 2020 •
edited
Loading

Summary

Reintroduced tabs to endpoint management section of security and migrated the views to shared security components.

image

Checklist

@efreeti efreeti requested review from a team as code owners August 12, 2020 17:43
@efreeti efreeti self-assigned this Aug 12, 2020
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@efreeti efreeti added the release_note:skip Skip the PR/issue when compiling release notes label Aug 12, 2020
paul-tavares
paul-tavares reviewed Aug 12, 2020
View reviewed changes
Copy link
Contributor

@paul-tavares paul-tavares left a comment

Choose a reason for hiding this comment

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

Some feedback included. I'm also going to try and check this out today and run it locally - will post back if I notice anything.

Have you had a quick Demo with @bfishel on this? Only because it maybe changing the design from what she had originally envisioned so just want to ensure she's aware.

One last thing: can you include some screen capture (or video/GIF) to the PR description?

x-pack/plugins/security_solution/public/management/components/management_tabs.ts Outdated Show resolved Hide resolved
x-pack/plugins/security_solution/public/management/pages/endpoint_hosts/view/index.tsx Outdated Show resolved Hide resolved
x-pack/plugins/security_solution/public/common/components/wrapper_page/index.tsx Outdated Show resolved Hide resolved
@paul-tavares
Copy link
Contributor

Ran it locally and it looks good. Disregard my comment around tab clicking and full page refresh - it seems to work as expected.

@kevinlog
Copy link
Contributor

kevinlog commented Aug 13, 2020 •
edited
Loading

pulled it down and it looks good to me, just fix the tests and consider @paul-tavares suggestion on wrapping the <WrapperComponent>

EDIT: Also, put a screenshot up!

paul-tavares
paul-tavares approved these changes Aug 17, 2020
View reviewed changes
Copy link
Contributor

@paul-tavares paul-tavares left a comment

Choose a reason for hiding this comment

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

LGTM

Had some question and minor feedback which can be address later

x-pack/plugins/security_solution/public/management/common/routing.ts Outdated Show resolved Hide resolved
x-pack/plugins/security_solution/public/management/components/administration_list_page.tsx Outdated Show resolved Hide resolved
x-pack/plugins/security_solution/public/management/pages/index.test.tsx Show resolved Hide resolved
x-pack/plugins/security_solution/public/management/pages/index.test.tsx Show resolved Hide resolved
x-pack/plugins/security_solution/public/management/pages/index.tsx Show resolved Hide resolved
XavierM
XavierM reviewed Aug 17, 2020
View reviewed changes
Copy link
Contributor

@XavierM XavierM left a comment

Choose a reason for hiding this comment

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

I think we should put back the const and avoid merging the code with function. So we do not start introducing back the function style in the repo

@efreeti
Copy link
Contributor Author

efreeti commented Aug 18, 2020

@XavierM What is the reason to discourage function declarations? I find even readability of the code worse when using const and lambda.

paul-tavares
paul-tavares approved these changes Aug 18, 2020
View reviewed changes
Copy link
Contributor

@paul-tavares paul-tavares left a comment

Choose a reason for hiding this comment

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

Agree with @kevinlog that we should get this in. My comments were minor and optional. I left some more in response to your last set of comments.

👍

x-pack/plugins/security_solution/public/management/common/routing.ts Outdated Show resolved Hide resolved
x-pack/plugins/security_solution/public/management/components/administration_list_page.tsx Outdated Show resolved Hide resolved
kevinlog
kevinlog suggested changes Aug 18, 2020
View reviewed changes
Copy link
Contributor

@kevinlog kevinlog left a comment

Choose a reason for hiding this comment

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

I would ask that we change the function expressions back to lamda's before merging.

@XavierM
Copy link
Contributor

XavierM commented Aug 18, 2020

@XavierM What is the reason to discourage function declarations? I find even readability of the code worse when using const and lambda.

I think because we want to follow that https://github.com/elastic/kibana/blob/master/STYLEGUIDE.md#prefer-modern-javascripttypescript-syntax

@efreeti
Copy link
Contributor Author

efreeti commented Aug 18, 2020

https://github.com/elastic/kibana/blob/master/STYLEGUIDE.md#prefer-modern-javascripttypescript-syntax

@XavierM What is the reason to discourage function declarations? I find even readability of the code worse when using const and lambda.

I think because we want to follow that https://github.com/elastic/kibana/blob/master/STYLEGUIDE.md#prefer-modern-javascripttypescript-syntax

As mentioned it says Prefer arrow function over function expressions not over function declarations. But sure I'll revert it back.

@efreeti
Copy link
Contributor Author

efreeti commented Aug 18, 2020

fixed function declarations, and added memo.

@kevinlog
Copy link
Contributor

Thanks for the changes!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
securitySolution 1916 +2 1914

async chunks size

id value diff baseline
securitySolution 7.2MB -3.5KB 7.2MB

page load bundle size

id value diff baseline
securitySolution 806.8KB +763.0B 806.1KB

History

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

@efreeti efreeti merged commit 4a274e0 into elastic:master Aug 18, 2020
@efreeti efreeti deleted the btsymbala/trusted-apps-page branch August 18, 2020 17:42
@efreeti efreeti mentioned this pull request Aug 18, 2020
Merged
efreeti added a commit to efreeti/kibana that referenced this pull request Aug 18, 2020
@efreeti
[ENDPOINT] Reintroduced tabs to endpoint management and migrated page…
dac2ae0 
…s to use common security components (elastic#74886)

* Reintroduced tabs to endpoint management and migrated pages to use common security components.

* Empty trusted apps tab.

* Changed casing in the translations.

* Switched to using route path generation functions.

* Added propagation of data-test-subj attribute to Wrapper component.

* Fixed CommonProps import.

* Moved out shared component for administration list page.

* Removed unused file.

* Removed unused translation keys.

* Removed redundant snapshot.

* Added some minimal tests.

* Attempt to fix functional tests.

* Attempt to fix functional tests again.

* Reverted function declarations back to const.

* Wrapped component in memo.
efreeti added a commit that referenced this pull request Aug 18, 2020
@efreeti
[ENDPOINT] Reintroduced tabs to endpoint management and migrated page…
c1b50e6 
…s to use common security components (#74886) (#75352)

* Reintroduced tabs to endpoint management and migrated pages to use common security components.

* Empty trusted apps tab.

* Changed casing in the translations.

* Switched to using route path generation functions.

* Added propagation of data-test-subj attribute to Wrapper component.

* Fixed CommonProps import.

* Moved out shared component for administration list page.

* Removed unused file.

* Removed unused translation keys.

* Removed redundant snapshot.

* Added some minimal tests.

* Attempt to fix functional tests.

* Attempt to fix functional tests again.

* Reverted function declarations back to const.

* Wrapped component in memo.
gmmorris added a commit to gmmorris/kibana that referenced this pull request Aug 19, 2020
@gmmorris
Merge branch 'master' into task-manager/poller-mon
b0f7c0a 
* master: (112 commits)
  [Ingest Manager] Fix agent config rollout rate limit to use constants (elastic#75364)
  Update Node.js to version 10.22.0 (elastic#75254)
  [ML] Anomaly Explorer / Single Metric Viewer: Fix error reporting for annotations. (elastic#74953)
  [Discover] Fix histogram cloud tests (elastic#75268)
  Uiactions to navigate to visualize or maps (elastic#74121)
  Use prefix search invis editor field/agg combo box (elastic#75290)
  Fix docs in trigger alerting UI (elastic#75363)
  [SIEM] Fixes search bar Cypress test (elastic#74833)
  Add libnss3.so to Dockerfile template (reporting) (elastic#75370)
  [Discover] Create field_button and add popovers to sidebar (elastic#73226)
  [Reporting] Network Policy: Do not throw from the intercept handler (elastic#75105)
  [Reporting] Increase capture.timeouts.openUrl to 1 minute (elastic#75207)
  Allow routes to specify the idle socket timeout in addition to the payload timeout (elastic#73730)
  [src/dev/build] remove node-version from snapshots (elastic#75303)
  [ENDPOINT] Reintroduced tabs to endpoint management and migrated pages to use common security components (elastic#74886)
  [Canvas] Remove dependency on legacy expressions APIs (elastic#74885)
  Skip failing test in CI (elastic#75266)
  [Task Manager] time out work when it overruns in poller (elastic#74980)
  [Drilldowns] misc improvements & fixes (elastic#75276)
  Small README note on bumping memory for builds (elastic#75247)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pzl pzl pzl left review comments

@XavierM XavierM XavierM left review comments

@paul-tavares paul-tavares paul-tavares approved these changes

@kevinlog kevinlog kevinlog approved these changes

Assignees

@efreeti efreeti

Labels
Feature:Endpoint Elastic Endpoint feature release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@efreeti @elasticmachine @paul-tavares @kevinlog @XavierM @kibanamachine @pzl