-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[app switcher] nav link enhancements #7601
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
Changes from all commits
8df249b
2712203
96588bd
b25456a
0b1acad
9161c0c
bb45a39
921533a
7e931bd
a8995e3
a376c03
f4390ba
30032c5
8dac9c3
2601fcf
42cc301
de1ea90
81072ad
03eebad
3a8948f
c8b4d2d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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', () => { | ||
| 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)); | ||
| }); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,6 +45,35 @@ describe('chrome nav apis', function () { | |
| }); | ||
| }); | ||
|
|
||
| describe('#getNavLinkById', () => { | ||
| it ('retrieves the correct nav link, given its ID', () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.eql(nav[0]); | ||
| }); | ||
|
|
||
| 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(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,14 @@ export default function (chrome, internals) { | |
| return internals.nav; | ||
| }; | ||
|
|
||
| chrome.getNavLinkById = (id) => { | ||
| const navLink = internals.nav.find(link => link.id === id); | ||
| if (!navLink) { | ||
| throw new Error(`Nav link for id = ${id} not found`); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; |
||
| } | ||
| return navLink; | ||
| }; | ||
|
|
||
| chrome.getBasePath = function () { | ||
| return internals.basePath || ''; | ||
| }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,15 +3,19 @@ import { join } from 'path'; | |
|
|
||
| export default class UiNavLink { | ||
| constructor(uiExports, spec) { | ||
| this.id = spec.id; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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']); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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.urlBasePathdefaults to an empty string if it's falsy, which affects the UiNavLink instance'surlprop.orderdefaults to 0.linkToLastSubUrlwill be false only ifspec.linkToLastSubUrlis false, but will default to true for any other value (including falsy ones!).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.