[app switcher] nav link enhancements #7601

ycombinator · 2016-07-01T10:47:33Z

This PR adds the following functionality:

Allows retrieval of a nav link from the app switcher nav, given the link's title.
Allows toggling the display of the nav link in the app switcher nav
Allows toggling the enabled/disabled state of the nav link in the app switcher nav
Allows setting (and showing) a tooltip on the nav link in the app switcher nav

For plugins, by default the plugin ID is used as the nav link ID. This can be overridden by explicitly specifying an ID on the nav link (useful for plugins that register multiple nav links).

epixa · 2016-07-01T12:19:58Z

src/ui/public/chrome/api/__tests__/nav.js

@@ -45,6 +45,19 @@ describe('chrome nav apis', function () {
    });
  });

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


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?

epixa · 2016-07-01T12:31:32Z

src/ui/ui_nav_link.js

@@ -3,6 +3,7 @@ import { join } from 'path';

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


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

ycombinator · 2016-07-01T23:16:15Z

@epixa, @cjcenizal This is ready for review again.

The build 💥 look unrelated to tests. Also Jenkins hosts seem to be changing. I'm keeping an eye on things.

cjcenizal · 2016-07-01T23:40:58Z

src/ui/public/chrome/api/__tests__/nav.js

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


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

Ah cool. Will give this a shot. Thanks!

cjcenizal · 2016-07-01T23:44:12Z

👍 Looks great!

ycombinator · 2016-07-02T00:10:57Z

@cjcenizal Made the two changes you suggested. Ready for review again. 😄

cjcenizal · 2016-07-02T00:12:55Z

LGTM!

ycombinator · 2016-07-05T14:47:57Z

@epixa Can you take a look at this one again today, please? It looks like the build off the latest commit (81072ad) succeeded on the old CI environment: http://build-eu-00.elastic.co/job/kibana_core_pr/4454/

epixa · 2016-07-05T20:16:26Z

This needs to be rebased or have master merged.

epixa · 2016-07-05T21:41:05Z

src/ui/public/chrome/api/nav.js

+    if (navLink) {
+      return navLink;
+    } else {
+      throw new Error(`Nav link for id = ${id} not found`);


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;

epixa · 2016-07-05T22:22:04Z

LGTM on green

cjcenizal · 2016-07-05T22:39:00Z

LGTM

…k-enhancements [app switcher] nav link enhancements Former-commit-id: 0169492

`v94.1.0-backport.0` ⏩ `v94.2.1-backport.0` _[Questions? Please see our Kibana upgrade FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams)_ --- ## [`v94.2.1-backport.0`](https://github.com/elastic/eui/releases/v94.2.1-backport.0) **This is a backport release only intended for use by Kibana.** - Reverted the `EuiFlexGroup`/`EuiFlexItem` `component` prop feature due to Kibana typing issues ## [`v94.2.1`](https://github.com/elastic/eui/releases/v94.2.1) **Bug fixes** - Fixed an `EuiTabbedContent` edge case bug that occurred when updated with a completely different set of `tabs` ([#7713](elastic/eui#7713)) - Fixed the `@storybook/test` dependency to be listed in `devDependencies` and not `dependencies` ([#7719](elastic/eui#7719)) ## [`v94.2.0`](https://github.com/elastic/eui/releases/v94.2.0) - Updated `getDefaultEuiMarkdownPlugins()` to allow excluding the following plugins in addition to `tooltip`: ([#7676](elastic/eui#7676)) - `checkbox` - `linkValidator` - `lineBreaks` - `emoji` - Updated `EuiSelectable`'s `isPreFiltered` prop to allow passing a configuration object, which allows disabling search highlighting in addition to search filtering ([#7683](elastic/eui#7683)) - Updated `EuiFlexGroup` and `EuiFlexItem` prop types to support passing any valid React component type to the `component` prop and ensure proper type checking of the extra props forwarded to the `component`. ([#7688](elastic/eui#7688)) - Updated `EuiSearchBar` to allow the `@` special character in query string searches ([#7702](elastic/eui#7702)) - Added a new, optional `optionMatcher` prop to `EuiSelectable` and `EuiComboBox` allowing passing a custom option matcher function to these components and controlling option filtering for given search string ([#7709](elastic/eui#7709)) **Bug fixes** - Fixed an `EuiPageTemplate` bug where prop updates would not cascade down to child sections ([#7648](elastic/eui#7648)) - To cascade props down to the sidebar, `EuiPageTemplate` now explicitly requires using the `EuiPageTemplate.Sidebar` rather than `EuiPageSidebar` - Fixed `EuiFieldNumber`'s typing to accept an icon configuration shape ([#7666](elastic/eui#7666)) - Fixed `EuiFieldText` and `EuiFieldNumber` to render the correct paddings for icon shapes set to `side: 'right'` ([#7666](elastic/eui#7666)) - Fixed `EuiFieldText` and `EuiFieldNumber` to fully ignore `icon`/`prepend`/`append` when `controlOnly` is set to true ([#7666](elastic/eui#7666)) - Fixed `EuiColorPicker`'s input not setting the correct right padding for the number of icons displayed ([#7666](elastic/eui#7666)) - Visual fixes for `EuiRange`s with `showInput`: ([#7678](elastic/eui#7678)) - Longer `append`/`prepend` labels no longer cause a background bug - Inputs can no longer overwhelm the actual range in width - Fixed a visual text alignment regression in `EuiTableRowCell`s with the `row` header scope ([#7681](elastic/eui#7681)) - Fixed `toolTipProps` type on `EuiSuperUpdateButton` to use `Partial<EuiToolTipProps>` ([#7692](elastic/eui#7692)) - Fixes missing prop type for `popperProps` on `EuiDatePicker` ([#7694](elastic/eui#7694)) - Fixed a focus bug with `EuiDataGrid`s with `leadingControlColumns` when moving columns to the left/right ([#7701](elastic/eui#7701)) ([#7698](elastic/eui#7698)) - Fixed `EuiSuperDatePicker` to validate date string with respect of locale on `EuiAbsoluteTab`. ([#7705](elastic/eui#7705)) - Fixed a visual bug with `EuiSuperDatePicker`'s absolute tab on small mobile screens ([#7708](elastic/eui#7708)) - Fixed i18n of empty and loading state messages for the `FieldValueSelectionFilter` component ([#7718](elastic/eui#7718)) **Dependency updates** - Updated `@hello-pangea/dnd` to v16.6.0 ([#7599](elastic/eui#7599)) - Updated `remark-rehype` to v8.1.0 ([#7601](elastic/eui#7601)) **Accessibility** - Improved `EuiBasicTable` and `EuiInMemoryTable`'s selection checkboxes to have unique aria-labels per row ([#7672](elastic/eui#7672)) - Added `aria-valuetext` attributes to `EuiRange`s with tick labels for improved screen reader UX ([#7675](elastic/eui#7675)) - Updated `EuiAccordion` to keep focus on accordion trigger instead of moving to content on click/keypress ([#7696](elastic/eui#7696)) - Added `aria-disabled` attribute to `EuiHorizontalSteps` when status is "disabled" ([#7699](elastic/eui#7699)) --------- Co-authored-by: Tomasz Kajtoch <[email protected]>

ycombinator added review v5.0.0 v5.0.0-beta1 labels Jul 1, 2016

ycombinator added 2 commits July 1, 2016 03:48

Method to get nav link by its title + control nav link display state

8df249b

Add tooltip to nav link

2712203

ycombinator force-pushed the app-switcher/nav-link-enhancements branch from 757f500 to 2712203 Compare July 1, 2016 10:48

ycombinator assigned epixa Jul 1, 2016

Refactoring: extracting functions

96588bd

ycombinator changed the title ~~App switcher/nav link enhancements~~ [app switcher] nav link enhancements Jul 1, 2016

epixa assigned ycombinator and unassigned epixa Jul 1, 2016

epixa removed the review label Jul 1, 2016

Giving nav links an ID

b25456a

For plugins, by default the plugin ID is used as the nav link ID. This can be overridden by explicitly specifying an ID on the nav link (useful for plugins that register multiple nav links).

ycombinator added the review label Jul 1, 2016

ycombinator assigned epixa and unassigned ycombinator Jul 1, 2016

epixa reviewed Jul 1, 2016
View reviewed changes

ycombinator removed the review label Jul 1, 2016

ycombinator assigned ycombinator and unassigned epixa Jul 1, 2016

epixa reviewed Jul 1, 2016
View reviewed changes

ycombinator added 5 commits July 1, 2016 05:34

Fixing test so it passes

0b1acad

Throw error if nav link with given ID is not found

9161c0c

Adding unit test for toJSON()

bb45a39

Fixing minor typo

921533a

Adding unit test for constructor

7e931bd

ycombinator assigned epixa and unassigned ycombinator Jul 1, 2016

ycombinator added the review label Jul 1, 2016

ycombinator added 4 commits July 1, 2016 11:35

Using native Array.prototype.filter() instead of _.filter()

30032c5

Using class instead of element selector

8dac9c3

Adding unit tests for default paths involving various properties

2601fcf

Adding hidden, disabled and tooltip properties to UiNavLink class

42cc301

cjcenizal reviewed Jul 1, 2016
View reviewed changes

ycombinator added 2 commits July 1, 2016 17:07

Replace 2 assertions with 1

de1ea90

Remove unused var

81072ad

ycombinator added 2 commits July 5, 2016 13:23

Merge branch 'master' into app-switcher/nav-link-enhancements

03eebad

Merge branch 'master' into app-switcher/nav-link-enhancements

3a8948f

epixa reviewed Jul 5, 2016
View reviewed changes

Return early from function

c8b4d2d

epixa assigned ycombinator and unassigned epixa Jul 5, 2016

ycombinator merged commit 0169492 into elastic:master Jul 5, 2016

ycombinator deleted the app-switcher/nav-link-enhancements branch July 5, 2016 22:39

epixa added v5.0.0-alpha5 and removed v5.0.0-beta1 labels Jul 20, 2016

epixa added v5.0.0 and removed v5.0.0 labels Aug 1, 2016

airow pushed a commit to airow/kibana that referenced this pull request Feb 16, 2017

Merge pull request elastic#7601 from ycombinator/app-switcher/nav-lin…

92253c0

…k-enhancements [app switcher] nav link enhancements Former-commit-id: 0169492

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[app switcher] nav link enhancements #7601

[app switcher] nav link enhancements #7601

ycombinator commented Jul 1, 2016

epixa Jul 1, 2016

ycombinator Jul 1, 2016

epixa Jul 1, 2016

ycombinator Jul 1, 2016

ycombinator commented Jul 1, 2016 •

edited

Loading

cjcenizal Jul 1, 2016

ycombinator Jul 1, 2016

cjcenizal commented Jul 1, 2016

ycombinator commented Jul 2, 2016

cjcenizal commented Jul 2, 2016

ycombinator commented Jul 5, 2016

epixa commented Jul 5, 2016

epixa Jul 5, 2016

epixa commented Jul 5, 2016

cjcenizal commented Jul 5, 2016

[app switcher] nav link enhancements #7601

[app switcher] nav link enhancements #7601

Conversation

ycombinator commented Jul 1, 2016

epixa Jul 1, 2016

Choose a reason for hiding this comment

ycombinator Jul 1, 2016

Choose a reason for hiding this comment

epixa Jul 1, 2016

Choose a reason for hiding this comment

ycombinator Jul 1, 2016

Choose a reason for hiding this comment

ycombinator commented Jul 1, 2016 • edited Loading

cjcenizal Jul 1, 2016

Choose a reason for hiding this comment

ycombinator Jul 1, 2016

Choose a reason for hiding this comment

cjcenizal commented Jul 1, 2016

ycombinator commented Jul 2, 2016

cjcenizal commented Jul 2, 2016

ycombinator commented Jul 5, 2016

epixa commented Jul 5, 2016

epixa Jul 5, 2016

Choose a reason for hiding this comment

epixa commented Jul 5, 2016

cjcenizal commented Jul 5, 2016

ycombinator commented Jul 1, 2016 •

edited

Loading