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

[app switcher] nav link enhancements #7601

Merged
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
8df249b
Method to get nav link by its title + control nav link display state
ycombinator Jul 1, 2016
2712203
Add tooltip to nav link
ycombinator Jul 1, 2016
96588bd
Refactoring: extracting functions
ycombinator Jul 1, 2016
b25456a
Giving nav links an ID
ycombinator Jul 1, 2016
0b1acad
Fixing test so it passes
ycombinator Jul 1, 2016
9161c0c
Throw error if nav link with given ID is not found
ycombinator Jul 1, 2016
bb45a39
Adding unit test for toJSON()
ycombinator Jul 1, 2016
921533a
Fixing minor typo
ycombinator Jul 1, 2016
7e931bd
Adding unit test for constructor
ycombinator Jul 1, 2016
a8995e3
Outdenting closing bracket per styleguide
ycombinator Jul 1, 2016
a376c03
Use ES6 find instead of _.find
ycombinator Jul 1, 2016
f4390ba
Using ES6 template literal instead of old school string concatenation
ycombinator Jul 1, 2016
30032c5
Using native Array.prototype.filter() instead of _.filter()
ycombinator Jul 1, 2016
8dac9c3
Using class instead of element selector
ycombinator Jul 1, 2016
2601fcf
Adding unit tests for default paths involving various properties
ycombinator Jul 1, 2016
42cc301
Adding hidden, disabled and tooltip properties to UiNavLink class
ycombinator Jul 1, 2016
de1ea90
Replace 2 assertions with 1
ycombinator Jul 2, 2016
81072ad
Remove unused var
ycombinator Jul 2, 2016
03eebad
Merge branch 'master' into app-switcher/nav-link-enhancements
ycombinator Jul 5, 2016
3a8948f
Merge branch 'master' into app-switcher/nav-link-enhancements
ycombinator Jul 5, 2016
c8b4d2d
Return early from function
ycombinator Jul 5, 2016
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/plugins/kibana/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,27 +40,31 @@ module.exports = function (kibana) {

links: [
{
id: 'kibana:discover',
title: 'Discover',
order: -1003,
url: '/app/kibana#/discover',
description: 'interactively explore your data',
icon: 'plugins/kibana/assets/discover.svg',
},
{
id: 'kibana:visualize',
title: 'Visualize',
order: -1002,
url: '/app/kibana#/visualize',
description: 'design data visualizations',
icon: 'plugins/kibana/assets/visualize.svg',
},
{
id: 'kibana:dashboard',
title: 'Dashboard',
order: -1001,
url: '/app/kibana#/dashboard',
description: 'compose visualizations for much win',
icon: 'plugins/kibana/assets/dashboard.svg',
},
{
id: 'kibana:management',
title: 'Management',
order: 1000,
url: '/app/kibana#/management',
Expand Down
159 changes: 159 additions & 0 deletions src/ui/__tests__/ui_nav_link.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
import expect from 'expect.js';

import UiNavLink from '../ui_nav_link';

describe('UiNavLink', () => {
Copy link
Contributor

@cjcenizal cjcenizal Jul 1, 2016

Choose a reason for hiding this comment

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

This class has a couple other wrinkles to its logic that I think are worth testing / documenting via tests:

  • uiExports.urlBasePath defaults to an empty string if it's falsy, which affects the UiNavLink instance's url prop.
  • order defaults to 0.
  • linkToLastSubUrl will be false only if spec.linkToLastSubUrl is false, but will default to true for any other value (including falsy ones!).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

describe('constructor', () => {
it ('initializes the object properties as expected', () => {
const uiExports = {
urlBasePath: 'http://localhost:5601/rnd'
};
const spec = {
id: 'kibana:discover',
title: 'Discover',
order: -1003,
url: '/app/kibana#/discover',
description: 'interactively explore your data',
icon: 'plugins/kibana/assets/discover.svg',
hidden: true,
disabled: true
};
const link = new UiNavLink(uiExports, spec);

expect(link.id).to.be(spec.id);
expect(link.title).to.be(spec.title);
expect(link.order).to.be(spec.order);
expect(link.url).to.be(`${uiExports.urlBasePath}${spec.url}`);
expect(link.description).to.be(spec.description);
expect(link.icon).to.be(spec.icon);
expect(link.hidden).to.be(spec.hidden);
expect(link.disabled).to.be(spec.disabled);
});

it ('initializes the url property without a base path when one is not specified in the spec', () => {
const uiExports = {};
const spec = {
id: 'kibana:discover',
title: 'Discover',
order: -1003,
url: '/app/kibana#/discover',
description: 'interactively explore your data',
icon: 'plugins/kibana/assets/discover.svg',
};
const link = new UiNavLink(uiExports, spec);

expect(link.url).to.be(spec.url);
});

it ('initializes the order property to 0 when order is not specified in the spec', () => {
const uiExports = {};
const spec = {
id: 'kibana:discover',
title: 'Discover',
url: '/app/kibana#/discover',
description: 'interactively explore your data',
icon: 'plugins/kibana/assets/discover.svg',
};
const link = new UiNavLink(uiExports, spec);

expect(link.order).to.be(0);
});

it ('initializes the linkToLastSubUrl property to false when false is specified in the spec', () => {
const uiExports = {};
const spec = {
id: 'kibana:discover',
title: 'Discover',
order: -1003,
url: '/app/kibana#/discover',
description: 'interactively explore your data',
icon: 'plugins/kibana/assets/discover.svg',
linkToLastSubUrl: false
};
const link = new UiNavLink(uiExports, spec);

expect(link.linkToLastSubUrl).to.be(false);
});

it ('initializes the linkToLastSubUrl property to true by default', () => {
const uiExports = {};
const spec = {
id: 'kibana:discover',
title: 'Discover',
order: -1003,
url: '/app/kibana#/discover',
description: 'interactively explore your data',
icon: 'plugins/kibana/assets/discover.svg',
};
const link = new UiNavLink(uiExports, spec);

expect(link.linkToLastSubUrl).to.be(true);
});

it ('initializes the hidden property to false by default', () => {
const uiExports = {};
const spec = {
id: 'kibana:discover',
title: 'Discover',
order: -1003,
url: '/app/kibana#/discover',
description: 'interactively explore your data',
icon: 'plugins/kibana/assets/discover.svg',
};
const link = new UiNavLink(uiExports, spec);

expect(link.hidden).to.be(false);
});

it ('initializes the disabled property to false by default', () => {
const uiExports = {};
const spec = {
id: 'kibana:discover',
title: 'Discover',
order: -1003,
url: '/app/kibana#/discover',
description: 'interactively explore your data',
icon: 'plugins/kibana/assets/discover.svg',
};
const link = new UiNavLink(uiExports, spec);

expect(link.disabled).to.be(false);
});

it ('initializes the tooltip property to an empty string by default', () => {
const uiExports = {};
const spec = {
id: 'kibana:discover',
title: 'Discover',
order: -1003,
url: '/app/kibana#/discover',
description: 'interactively explore your data',
icon: 'plugins/kibana/assets/discover.svg',
};
const link = new UiNavLink(uiExports, spec);

expect(link.tooltip).to.be('');
});
});

describe('#toJSON', () => {
it ('returns the expected properties', () => {
const uiExports = {
urlBasePath: 'http://localhost:5601/rnd'
};
const spec = {
id: 'kibana:discover',
title: 'Discover',
order: -1003,
url: '/app/kibana#/discover',
description: 'interactively explore your data',
icon: 'plugins/kibana/assets/discover.svg',
};
const link = new UiNavLink(uiExports, spec);
const json = link.toJSON();

['id', 'title', 'url', 'order', 'description', 'icon', 'linkToLastSubUrl', 'hidden', 'disabled', 'tooltip']
.forEach(expectedProperty => expect(json).to.have.property(expectedProperty));
});
});
});
30 changes: 30 additions & 0 deletions src/ui/public/chrome/api/__tests__/nav.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,36 @@ describe('chrome nav apis', function () {
});
});

describe('#getNavLinkById', () => {
it ('retrieves the correct nav link, given its ID', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be a test for the case where the id is not found. If possible, it'd be great if we could throw in that case, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

const appUrlStore = new StubBrowserStorage();
const nav = [
{ id: 'kibana:discover', title: 'Discover' }
];
const { chrome, internals } = init({ appUrlStore, nav });

const navLink = chrome.getNavLinkById('kibana:discover');
expect(navLink).to.not.be(undefined);
expect(navLink.title).to.be('Discover');
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these two expectations can be replaced with:

expect(navLink).to.eql(nav[0]);

This should do a deep comparison: http://chaijs.com/api/bdd/#method_eql

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah cool. Will give this a shot. Thanks!

});

it ('throws an error if the nav link with the given ID is not found', () => {
const appUrlStore = new StubBrowserStorage();
const nav = [
{ id: 'kibana:discover', title: 'Discover' }
];
const { chrome, internals } = init({ appUrlStore, nav });

let errorThrown = false;
try {
const navLink = chrome.getNavLinkById('nonexistent');
} catch (e) {
errorThrown = true;
}
expect(errorThrown).to.be(true);
});
});

describe('internals.trackPossibleSubUrl()', function () {
it('injects the globalState of the current url to all links for the same app', function () {
const appUrlStore = new StubBrowserStorage();
Expand Down
9 changes: 9 additions & 0 deletions src/ui/public/chrome/api/nav.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,15 @@ export default function (chrome, internals) {
return internals.nav;
};

chrome.getNavLinkById = (id) => {
const navLink = internals.nav.find(link => link.id === id);
if (navLink) {
return navLink;
} else {
throw new Error(`Nav link for id = ${id} not found`);
Copy link
Contributor

Choose a reason for hiding this comment

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

In the spirit of Return early from functions, I recommend doing this assertion up front and then returning outside of the conditional:

const navLink = internals.nav.find(link => link.id === id);
if (!navLink) {
  throw new Error(`Nav link for id = ${id} not found`);
}
return navLink;

}
};

chrome.getBasePath = function () {
return internals.basePath || '';
};
Expand Down
13 changes: 10 additions & 3 deletions src/ui/public/chrome/directives/app_switcher/app_switcher.html
Original file line number Diff line number Diff line change
@@ -1,13 +1,20 @@
<div class="app-links">
<div
class="app-link"
ng-repeat="link in switcher.getNavLinks()"
ng-class="{ active: link.active }">
ng-repeat="link in switcher.shownNavLinks"
ng-class="{ active: link.active, 'is-app-switcher-app-link-disabled': !switcher.isNavLinkEnabled(link) }"
tooltip="{{link.tooltip}}"
tooltip-placement="right"
tooltip-popup-delay="400"
tooltip-append-to-body="1"
>

<a
ng-click="switcher.ensureNavigation($event, link)"
ng-href="{{ link.active ? link.url : (link.lastSubUrl || link.url) }}"
data-test-subj="appLink">
data-test-subj="appLink"
ng-class="{ 'app-link__anchor': !switcher.isNavLinkEnabled(link) }"
>

<div ng-if="link.icon" class="app-icon"><img kbn-src="{{'/' + link.icon}}"></div>
<div ng-if="!link.icon" class="app-icon-missing">{{ link.title[0] }}</div>
Expand Down
21 changes: 18 additions & 3 deletions src/ui/public/chrome/directives/app_switcher/app_switcher.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,18 @@
import DomLocationProvider from 'ui/dom_location';
import { parse } from 'url';
import { bindKey } from 'lodash';
import { bindKey, filter } from 'lodash';
Copy link
Contributor

Choose a reason for hiding this comment

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

filter is no longer needed her. We should really update our eslinter to catch unused vars like this (#7609).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh. Good catch. Thanks!

import '../app_switcher/app_switcher.less';
import uiModules from 'ui/modules';
import appSwitcherTemplate from './app_switcher.html';

function isNavLinkEnabled(link) {
return !link.disabled;
}

function isNavLinkShown(link) {
return !link.hidden;
}

uiModules
.get('kibana')
.provider('appSwitcherEnsureNavigation', function () {
Expand All @@ -17,7 +25,11 @@ uiModules
this.$get = ['Private', function (Private) {
const domLocation = Private(DomLocationProvider);

return function (event) {
return function (event, link) {
if (!isNavLinkEnabled(link)) {
event.preventDefault();
}

if (!forceNavigation || event.isDefaultPrevented() || event.altKey || event.metaKey || event.ctrlKey) {
return;
}
Expand Down Expand Up @@ -52,7 +64,10 @@ uiModules
throw new TypeError('appSwitcher directive requires the "chrome" config-object');
}

this.getNavLinks = bindKey($scope.chrome, 'getNavLinks');
this.isNavLinkEnabled = isNavLinkEnabled;

const allNavLinks = $scope.chrome.getNavLinks();
this.shownNavLinks = allNavLinks.filter(isNavLinkShown);

// links don't cause full-navigation events in certain scenarios
// so we force them when needed
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,13 +79,20 @@ body { overflow-x: hidden; }
height: @app-icon-height;
line-height: @app-line-height;


> a {
display: block;
height: 100%;
color: #ebf7fa;
}

&.is-app-switcher-app-link-disabled {
opacity: 0.5;

.app-link__anchor {
cursor: default;
}
}

.app-icon {
float: left;
font-weight: bold;
Expand Down
1 change: 1 addition & 0 deletions src/ui/ui_app.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ class UiApp {
if (!this.hidden) {
// any non-hidden app has a url, so it gets a "navLink"
this.navLink = this.uiExports.navLinks.new({
id: this.id,
title: this.title,
description: this.description,
icon: this.icon,
Expand Down
6 changes: 5 additions & 1 deletion src/ui/ui_nav_link.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,19 @@ import { join } from 'path';

export default class UiNavLink {
constructor(uiExports, spec) {
this.id = spec.id;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since there are other things that rely on this property, can you add a test to verify that the constructor sets it properly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

this.title = spec.title;
this.order = spec.order || 0;
this.url = `${uiExports.urlBasePath || ''}${spec.url}`;
this.description = spec.description;
this.icon = spec.icon;
this.linkToLastSubUrl = spec.linkToLastSubUrl === false ? false : true;
this.hidden = spec.hidden || false;
this.disabled = spec.disabled || false;
this.tooltip = spec.tooltip || '';
}

toJSON() {
return pick(this, ['title', 'url', 'order', 'description', 'icon', 'linkToLastSubUrl']);
return pick(this, ['id', 'title', 'url', 'order', 'description', 'icon', 'linkToLastSubUrl', 'hidden', 'disabled', 'tooltip']);
}
}