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

[chrome/nav] implement forcedNavigation logic in new nav #29770

Merged
merged 8 commits into from
Feb 2, 2019

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Jan 31, 2019

Summary

When the Kibana server is red we render the status page in place of the current page, kind of like an error page. This prevents us from having to worry about redirects, but also means that links which normally trigger navigation with hash changes are broken because the status page app is rendered instead of the current app. To fix this we enable "forced navigation" mode on the status page so that any nav bar link that is clicked, which resolves to the same protocol+host+path will trigger a page refresh.

This used to be implemented as a click handler on each nav item, but EUI doesn't support click handlers on links in the Nav, so this uses a single event listener at the container around the nav links which handles the click events which bubble up. This means the feature now also works for the recent links and the home link.

To test this you should start Kibana in oss mode as security prevents the display of the status page, since license info can't be retrieved.

  1. node scripts/kibana --dev --oss without elasticsearch
  2. open http://localhost:5601
  3. on the status page, try clicking the app links, the page should refresh and attempt to load the clicked url
  4. start es yarn es snapshot --license oss
  5. wait for Kibana to go green
  6. click an app link and the app should load correctly

Original implementation: https://github.com/spalger/kibana/blob/597062fcbd346c2e11f1266f54c041d4469f8dee/src/ui/public/chrome/directives/global_nav/app_switcher/app_switcher.js#L35-L61

@spalger spalger added review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.0.0 labels Jan 31, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform

@elasticmachine

This comment has been minimized.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@joshdover
Copy link
Contributor

Not sure if it's important to this PR, but the steps above do not quite work for me. I get the message Kibana server is not ready yet if ES is not started. I can get to the status page if I start ES, let Kibana connect, and then shut down ES.

@joshdover
Copy link
Contributor

Is this change necessary? Before your PR if you click on a NavLink that would trigger a hash change in Angular router, it appears to just stay on the status page. With this change, it refreshes the status page when you click on a navlink. Do we consider this current behavior broken? Is that refreshing worth the complexity of this change?

@spalger
Copy link
Contributor Author

spalger commented Feb 1, 2019

Is this change necessary? ... Do we consider this current behavior broken?

It's necessary in that it has been a "feature" since 4.2, and I think having app links that are clickable but don't work without refreshing the page considers them broken, personally.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

LGTM, just want a some comments to help me remember what in the world this is later 😄

src/ui/public/chrome/api/nav.js Show resolved Hide resolved
@elasticmachine

This comment has been minimized.

@spalger spalger merged commit b44d1ad into elastic:master Feb 2, 2019
@spalger spalger deleted the fix/forced-navigation branch February 2, 2019 01:59
@elasticmachine
Copy link
Contributor

💔 Build Failed

@spalger spalger added the non-issue Indicates to automation that a pull request should not appear in the release notes label Mar 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
non-issue Indicates to automation that a pull request should not appear in the release notes review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants