Move Nav APIs to new platform #34490

joshdover · 2019-04-03T21:15:18Z

Summary

This moves the core Nav APIs from ui/chrome into the ChromeService in the new platform.

ChromeStart now exposes a sub-service for reading and making limited updates to navlinks. These are powered by apps registered with the ApplicationService and filtered by UI Capabilities before being exposed by the ChromeService.
The header-global-nav directive now consumes navlinks from the new platform.
The lastSubUrl feature utilized by legacy apps has been refactored and will remain in ui/chrome. This feature utilizes the limited fields that ChromeService exposes to updates by outside code.

This change is the main blocker to moving the Chrome UI to the new platform. This will be necessary to enable the new platform to control top-level routing.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

elasticmachine · 2019-04-16T19:19:15Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

joshdover · 2019-05-08T15:59:25Z

At some point in the future we might want to look at what should be available to plugins when adding icons to the nav bar.
It appears the ability to disable an icon is broken or not available any more. This has been the case from before this PR. Is it a feature that is needed at all?
Looking at the license checking in most plugins, they are all quite similar, probably due to people copying code. When we first came to add ML to kibana we copied the way an existing plugin was doing it.
Because of this I believe there is some redundant code which people have added to their plugins because they assumed they needed to.
For example, most plugins add an enableLinks flag to the features passed to the client side.
It looks like logstash and reporting are making use of this flag, but others aren't.

@jgowdyelastic

I'm going to dig into why this is broken exactly, it may be that the Nav UI code is not reading the disabled property correctly.

My idea for this going forward is to allow applications to update their status via the ApplicationService in the new platform, rather than interfacing with Chrome APIs to update links. This way, the ApplicationService can manage displaying a license page of information or redirecting to somewhere else in Kibana.

eliperelman

Looks great!

elasticmachine · 2019-05-08T17:04:18Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-05-08T20:22:14Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

spalger

Things are looking really good, but I'm pretty sure that subUrl handling is applying outside of Kibana and I'm pretty sure that wasn't the case before recently (maybe before this change)

src/legacy/ui/public/chrome/api/sub_url_hooks.js

spalger · 2019-05-08T21:27:57Z

src/legacy/ui/public/chrome/directives/header_global_nav/components/header.tsx

-  const href = relativeToAbsolute(chrome.addBasePath(recentlyAccessed.link));
-  const navLink = navLinks.find(nl => href.startsWith(nl.subUrlBase));
+  const href = chrome.addBasePath(recentlyAccessed.link);
+  const navLink = navLinks.find(nl => href.startsWith(nl.appUrl));


This no longer finds the navLink for recently accessed items when basePath is in use, because nl.appUrl doesn't have the basePath, but href does.

src/legacy/ui/public/chrome/directives/header_global_nav/components/header.tsx

joshdover · 2019-05-09T19:27:03Z

@spalger I've updated this to fix the bug when there are multiple Kibana instances running under different base paths on the same domain. This requires using mostly absolute URLs as the internal representation (as it was before this change). I've also renamed appUrl (which wasn't actually a URL) to baseUrl (which now is an absolute URL).

spalger · 2019-05-09T19:49:19Z

x-pack/plugins/ml/public/hacks/toggle_app_link_in_nav.js

-    navLink.disabled = !xpackInfo.get('features.ml.isAvailable', false);
-  }
+
+  const navLinkUpdates = {


👍 want to apply the same treatment to x-pack/plugins/graph/public/hacks/toggle_app_link_in_nav.js?

elasticmachine · 2019-05-09T19:54:03Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-05-09T21:36:11Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

Bring subUrlBase into new platform ☹️

elasticmachine · 2019-05-09T23:17:14Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

spalger

LGTM!

This moves the core Nav APIs from `ui/chrome` into the `ChromeService` in the new platform. - `ChromeStart` now exposes a sub-service for reading and making limited updates to navlinks. These are powered by apps registered with the `ApplicationService` and filtered by UI Capabilities before being exposed by the `ChromeService`. - The `header-global-nav` directive now consumes navlinks from the new platform. - The `lastSubUrl` feature utilized by legacy apps has been refactored and will remain in `ui/chrome`. This feature utilizes the limited fields that `ChromeService` exposes to updates by outside code. This change is the main blocker to moving the Chrome UI to the new platform. This will be necessary to enable the new platform to control top-level routing.