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.

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

[Enterprise Search][bug/tech debt] Fix route navigation #75369

Merged
merged 4 commits into from
Aug 20, 2020

Conversation

cee-chen
Copy link
Member

Summary

Thanks to some help from Josh Dover, Nathan Smith, and Pierre Gayvallet's original PR comment, I realized that our entire app was unmounting and remounting (also causing our Kea/Redux state to unmount & remount) on every in-app navigation when it should not have been.

This was happening because we were not using correctly Kibana's history navigation but instead directly accessing history.push().

This PR fixes all instances of history.push() to navigateToUrl() (and refactors our breadcrumbs helper to use hooks in order to simplify not passing around context). I recommend following along by commit.

QA

  • With enterpriseSearch.host set:
    • Go to App Search and confirm it redirects you to the Engines Overview (URL: /engines)
    • Click on "App Search" in the Kibana breadcrumbs and confirms it redirects you again back to the Engines Overview and does not show a blank page
    • Manually go to http://localhost:5601/app/enterprise_search/app_search/setup_guide, click on App Search in the Kibana breadcrumbs, and confirm it redirects you to Engines Overview and does not break/show a blank page
    • Go to Workplace Search and confirm their breadcrumbs still works as before
  • With enterpriseSearch.host unset/commented out, from Kibana home:
    • Go to App Search and confirm it redirects you to the Setup Guide
    • Click on "App Search" in the Kibana breadcrumbs and confirms it redirects you again back to the Setup Guide and does not show a blank page
    • Go to Workplace Search and confirm it redirects you to the Setup Guide
    • Click on "Workplace Search" in the Kibana breadcrumbs and confirms it redirects you again back to the Setup Guide and does not show a blank page

Checklist

generate_breadcrumbs:
- Change base breadcrumb generator to a custom React useHook instead of passing history/context around
- Change use{Product}Breadcrumbs helpers to mainly involve merging arrays
- Update + simplify tests accordingly (test link behavior in main useBreadcrumb suite, not in subsequent helpers)

set_chrome:
- Update to use new breadcrumb hooks (requires pulling out of useEffect, hooks can't be used inside another hook)
- Clean up/refactor tests
@cee-chen cee-chen added Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Aug 18, 2020
@cee-chen
Copy link
Member Author

To test that unmounting and mounting is not still happening, you can go here:

return () => ReactDOM.unmountComponentAtNode(params.element);

and add a console.log('hello world') and check how many times 'hello world' logs between breadcrumb navigations.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

async chunks size

id value diff baseline
enterpriseSearch 359.3KB +218.0B 359.1KB

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

Copy link
Contributor

@scottybollinger scottybollinger left a comment

Choose a reason for hiding this comment

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

Looks great! Pulled it down locally and it works great. 🎉

@cee-chen cee-chen merged commit 7699f1c into elastic:master Aug 20, 2020
@cee-chen cee-chen deleted the fix-route-navigation branch August 20, 2020 16:44
cee-chen pushed a commit that referenced this pull request Aug 20, 2020
)

* Set up navigateToUrl context

* Update RR link helpers to use navigateToUrl

* Update breadcrumbs to use navigateToUrl + refactor

generate_breadcrumbs:
- Change base breadcrumb generator to a custom React useHook instead of passing history/context around
- Change use{Product}Breadcrumbs helpers to mainly involve merging arrays
- Update + simplify tests accordingly (test link behavior in main useBreadcrumb suite, not in subsequent helpers)

set_chrome:
- Update to use new breadcrumb hooks (requires pulling out of useEffect, hooks can't be used inside another hook)
- Clean up/refactor tests

* Update route redirects now that navigation works correctly
thomasneirynck pushed a commit to thomasneirynck/kibana that referenced this pull request Aug 21, 2020
* Set up navigateToUrl context

* Update RR link helpers to use navigateToUrl

* Update breadcrumbs to use navigateToUrl + refactor

generate_breadcrumbs:
- Change base breadcrumb generator to a custom React useHook instead of passing history/context around
- Change use{Product}Breadcrumbs helpers to mainly involve merging arrays
- Update + simplify tests accordingly (test link behavior in main useBreadcrumb suite, not in subsequent helpers)

set_chrome:
- Update to use new breadcrumb hooks (requires pulling out of useEffect, hooks can't be used inside another hook)
- Clean up/refactor tests

* Update route redirects now that navigation works correctly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants