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] Hook to handle events needing navigation via Router #63863

Merged
merged 12 commits into from
Apr 21, 2020
Merged

[Endpoint] Hook to handle events needing navigation via Router #63863

merged 12 commits into from
Apr 21, 2020

Conversation

paul-tavares
Copy link
Contributor

@paul-tavares paul-tavares commented Apr 17, 2020 •
edited
Loading

Summary

  • Added a new react hook (useNavigateByRouterEventHandler()) that provides a common callback for handing click events that are meant to navigate to a page via the router's history.push
  • Refactored several components to use new hook

Checklist

@paul-tavares paul-tavares added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:Endpoint Management Feature:Endpoint Elastic Endpoint feature v7.8.0 labels Apr 17, 2020
@paul-tavares paul-tavares self-assigned this Apr 17, 2020
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@paul-tavares paul-tavares requested review from oatkiller, kevinlog and parkiino and removed request for oatkiller and kevinlog April 20, 2020 14:51
@paul-tavares paul-tavares marked this pull request as ready for review April 20, 2020 14:52
@paul-tavares paul-tavares requested a review from a team as a code owner April 20, 2020 14:52
oatkiller
oatkiller reviewed Apr 20, 2020
View reviewed changes
...ins/endpoint/public/applications/endpoint/view/hooks/use_navigate_by_router_event_handler.ts
routeTo: string | [string, unknown] | LocationDescriptorObject<unknown>, // Cover the calling signature of `history.push()`

/** Additional onClick callback */
onClick?: EventHandlerCallback
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you could add a type parameter to this hook like: TargetElement extends HTMLButtonElement | HTMLAnchorElement
and then define onClick like: onClick?: MouseEventHandler<TargetElement> and then the return type of the hook could be MouseEventHandler<TargetElement>

I think this would allow onClick to have button or link specific logic (if needed)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 isn't that the same that I already have defined above in the EventHandlerCallback type?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the difference here with what @paul-tavares did vs @oatkiller suggestion. Maybe we could reuse TargetElement if we defined it outside of the the generic types for MouseEventHandler. But we could do that later if needed

oatkiller
oatkiller reviewed Apr 20, 2020
View reviewed changes
x-pack/plugins/endpoint/public/applications/endpoint/view/policy/policy_details.test.tsx Outdated
@@ -101,7 +101,8 @@ describe('Policy Details', () => {
'EuiPageHeaderSection[data-test-subj="pageViewHeaderLeft"] EuiButtonEmpty'
);
expect(history.location.pathname).toEqual('/policy/1');
backToListButton.simulate('click');
backToListButton.simulate('click', { button: 0 });
await sleep();
Copy link
Contributor

Choose a reason for hiding this comment

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

is this sleep necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like a mistake/typo on my part. Will remove it. Thanks

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@paul-tavares paul-tavares merged commit ed91275 into elastic:master Apr 21, 2020
@paul-tavares paul-tavares deleted the task/EMT-230-react-router-click-handler-hook branch April 21, 2020 20:30
@paul-tavares paul-tavares mentioned this pull request Apr 21, 2020
Merged
paul-tavares added a commit to paul-tavares/kibana that referenced this pull request Apr 21, 2020
@paul-tavares
[Endpoint] Hook to handle events needing navigation via Router (elast…
5b39987 
…ic#63863)

* new hook providing generic event handler for use with react router
* Refactor of Header Naviagtion to use useNavigateByRouterEventHandler
* Policy list refactor to use useNavigateByRouterEventHandler hook
* Policy list Policy name link to use useNavigateByRouterEventHandler hook
* Host list use of useNavigateByRouteEventHandler
paul-tavares added a commit that referenced this pull request Apr 22, 2020
@paul-tavares
[Endpoint] Hook to handle events needing navigation via Router (#63863)…
8c99b0a 
… (#64113)

* new hook providing generic event handler for use with react router
* Refactor of Header Naviagtion to use useNavigateByRouterEventHandler
* Policy list refactor to use useNavigateByRouterEventHandler hook
* Policy list Policy name link to use useNavigateByRouterEventHandler hook
* Host list use of useNavigateByRouteEventHandler
gmmorris added a commit to gmmorris/kibana that referenced this pull request Apr 22, 2020
@gmmorris
Merge branch 'master' into alerting/no-explicit-any
02077cc 
* master: (29 commits)
  [Dashboard] Deangularize navbar, attempt nr. 2 (elastic#61611)
  refactor action filter creation utils (elastic#62969)
  Refresh index pattern list before redirecting (elastic#63329)
  [APM]fixing custom link unit tests (elastic#64045)
  [Ingest] EPM & Fleet are enabled when Ingest is enabled (elastic#64103)
  [Alerting] Fixed bug with no possibility to edit the index name after adding (elastic#64033)
  [Maps] Map settings: min and max zoom (elastic#63714)
  [kbn-storybook] Use raw loader for text files (elastic#64108)
  [EPM] /packages/{package} endpoint to support upgrades (elastic#63629)
  [SIEM] New Platform Saved Objects Registration (elastic#64029)
  [Endpoint] Hook to handle events needing navigation via Router (elastic#63863)
  Fixed small issue in clone functionality (elastic#64085)
  [Endpoint]EMT-146: use ingest agent for status info (elastic#63921)
  [SIEM] Server NP Followup (elastic#64010)
  Register uiSettings on New Platform (elastic#64015)
  [Reporting] Integration polling config with client code (elastic#63754)
  [Docs]7.7 SIEM doc updates (elastic#63951)
  [SIEM] [Cases] Tags suggestions (elastic#63878)
  Include datasource UUID in agent config yaml, adjust overflow height of yaml view (elastic#64027)
  [DOCS] Add file size setting for Data Visualizer (elastic#64006)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Apr 22, 2020
@gmmorris
Merge branch 'task-manager/cancel-logging' of github.com:gmmorris/kib…
c3b9f64 
…ana into task-manager/cancel-logging

* 'task-manager/cancel-logging' of github.com:gmmorris/kibana: (28 commits)
  [Dashboard] Deangularize navbar, attempt nr. 2 (elastic#61611)
  refactor action filter creation utils (elastic#62969)
  Refresh index pattern list before redirecting (elastic#63329)
  [APM]fixing custom link unit tests (elastic#64045)
  [Ingest] EPM & Fleet are enabled when Ingest is enabled (elastic#64103)
  [Alerting] Fixed bug with no possibility to edit the index name after adding (elastic#64033)
  [Maps] Map settings: min and max zoom (elastic#63714)
  [kbn-storybook] Use raw loader for text files (elastic#64108)
  [EPM] /packages/{package} endpoint to support upgrades (elastic#63629)
  [SIEM] New Platform Saved Objects Registration (elastic#64029)
  [Endpoint] Hook to handle events needing navigation via Router (elastic#63863)
  Fixed small issue in clone functionality (elastic#64085)
  [Endpoint]EMT-146: use ingest agent for status info (elastic#63921)
  [SIEM] Server NP Followup (elastic#64010)
  Register uiSettings on New Platform (elastic#64015)
  [Reporting] Integration polling config with client code (elastic#63754)
  [Docs]7.7 SIEM doc updates (elastic#63951)
  [SIEM] [Cases] Tags suggestions (elastic#63878)
  Include datasource UUID in agent config yaml, adjust overflow height of yaml view (elastic#64027)
  [DOCS] Add file size setting for Data Visualizer (elastic#64006)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevinlog kevinlog kevinlog approved these changes

@parkiino parkiino Awaiting requested review from parkiino

@oatkiller oatkiller Awaiting requested review from oatkiller

Assignees

@paul-tavares paul-tavares

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

Successfully merging this pull request may close these issues.
5 participants
@paul-tavares @elasticmachine @kibanamachine @oatkiller @kevinlog