I strongly object to removing the class in src/ui/public/config_template.js and then embedding it into the config directive.

@spalger why?

I misunderstood the way this was working and thought that the config template was being decorated with is() and such. This seems to be defining an API in the config directive, and then trying to export it to the parent scopes so that they can manipulate the current template.

The idea of exporting an API up to your parent directives is strange, but I suppose stranger things have happened. I don't really have strong arguments against it.

I don't like this name thing.

presumably this should be something like

<kbn-top-nav name="dashboard" kbn-top-nav-button="kbnTopNav">

This directive has no tests

It does have tests. Not the most comprehensive, but they exist. I just check for the existence of the functions is, open, close, and toggle. Should I add more?

Because this directive isn't isolate, and adds properties to $scope, is has the non-obvious side effect of causing every view that uses it to end up with a property on $scope called kbnTopNav

However, because that property is not defined until after the directive is compiled, it isn't available in the controller until after the initial setup. That means if you wish to toggle something when your controller starts you'll need to wait until the directive links:

  $timeout(function () {
    if (config.get('timelion:showTutorial', true)) {
      $scope.kbnTopNav.open('docs');
    }
  }, 0);

Not a blocker, but something to watch out for.

Hmm, I would think that because I define kbnTopNav in the controller it
would work. But I can make an issue if it's really a nuisance and
investigate.
On Mar 31, 2016 11:48, "Rashid Khan" notifications@github.com wrote:

In src/ui/public/directives/kbn_top_nav.js
#6657 (comment):

•    }
  

•    const templateToCompile = ctrlObj.templates[ctrlObj.curr] || false;
  

•    $scope.kbnTopNav.currTemplate = templateToCompile ? $compile(templateToCompile)($scope) : false;
  

•  };
  

•  const normalizeOpts = _.partial(optionsNormalizer, (item) => {
  

•    ctrlObj.toggleCurrTemplate(item.key);
  

•  });
  

•  const niceMenuItems = _.compact(($scope[$attrs.config] || []).map(normalizeOpts));
  

•  ctrlObj.templates = _.assign({
  

•    interval: intervalTemplate,
  

•    filter: filterTemplate,
  

•  }, getTemplatesMap(niceMenuItems));
  

•  $scope.kbnTopNav = {
  

Not a blocker, but something to watch out for.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/elastic/kibana/pull/6657/files/8ccbc124822eecd23477ea4aed97c7e3ccaa021e#r58077811