Integrate UI Framework #8679

cjcenizal · 2016-10-13T23:36:47Z

Changes

Add Kibana UI Framework as NPM dependency, v0.0.5.
Integrate localNav component.
Update Dashboard, Discover, Management, Visualize, Dev Tools, Timelion to use kbnTopNav directive and display dropdowns consistently.
Addresses 'Open' or 'Load' a Saved Visualization #7902

When you review, don't forget to run npm i.

UI review

Please review the top navs for the affected apps. The screenshots below are for reference; please actually test out the top navs thoroughly on a locally running instance of the branch.

Please claim an app below to review.

Dashboard

Before

After

Discover

Before

After

Management

Before

After

Visualize

Before

After

Dev Tools

Before

After

Timelion

Before

After

spalger · 2016-10-17T21:03:11Z

I plan to start by reviewing visualize, but also plan to review all apps

cjcenizal · 2016-10-18T02:08:10Z

test/support/page_objects/common.js

@@ -304,10 +304,10 @@ export default class Common {
  }

  findTestSubject(selector) {
-    this.debug('in findTestSubject: ' + testSubjSelector(selector));
+    this.debug(`in findTestSubject: ${selector}`);


@spalger I'm thinking we can remove the testSubjSelector dependency. I think we should avoid nested test selectors in favor of very explicit, specific test selectors. This will make them less brittle and easier to grep. Once we do that, then it seems like we won't need the testSubjSelector dependency. Thoughts?

Changing the way this works is out of the scope of this pull request. Please put it back the way it was. I'm fine chatting about it but changing it as a part of this pr isn't the way to do it.

OK I'll change it back.

thomasneirynck · 2016-10-18T05:31:37Z

@cjcenizal I'm afraid I will need some hand-holding on this one. This is a large change, and I am not 100% sure about all the ins& outs.

A few initial comments:

There is a transfer of styling code of the localNav out of Kibana to an external package, the Kibana UI Framework. We will transfer out quite a bit more UI-boilerplate as time progresses. While the KUIF will stabilize UI-component development, by extracting it, it will be more cumbersome to quickly iterate with minor adjustments in context of entire Kibana-app.
The KUIF is its own project (in github, published to npm), and all the misery - euhr, delight ;) - that will entail. Issue tracking, PRs, version management, .... The extra indirections are minor things, but they compound.
is the UI-F is tightly integrated with Kibana. Will any of this ever be used outside Kibana?

In short, I need some education as to what the added value is of having this be a separate project. I also come in late to the discussion, so many of these things may already have been clarified.

The beautification of the HTML has also made it difficult to actually spot the actual changes, so some pointers there will be helpful.
Is there anyway we can leverage CSS or Less functionality so we can more easily identify when we pull in styling from the KUIF and when from plain Kibana?
- consider prefixing class-names: eg. 'kuifFoobar'
- 'widgetify' by using attribute selectors in the KUIF-selectors: e.g. like div[data-uif='foobar'']
- .... (?) ....

None of this had anything to do with the purpose of this change, which I think is great!

cjcenizal · 2016-10-18T15:42:27Z

@thomasneirynck Thanks for the well-grounded questions/criticisms!

Why an external framework

I do think this UI framework can be used outside of Kibana, e.g. Logstash and Cloud. It can also be useful if we or anyone else wants to prototype something, e.g. a plugin, using Kibana's UI.

I think it's also useful to be able to version the UI independently of Kibana. The UI will go through many iterations, and we'll need to document breaking changes (e.g. a change in a class name, altered markup structure) and migration strategies to help the Kibana codebase keep pace.

Also, the process of building the UI framework involves adding and publishing new documentation. This is probably a minor point and maybe one we should disregard, but it feels a bit more ergonomic to me to have these tasks and dependencies inside their own repo, instead of alongside those of Kibana.

Making UI development easier vs better

You're totally right that making UI adjustments can become an arduous process. As a developer, you'll need to identify the component you want to change, define the use case for it, and review existing use cases for the component to make sure your change isn't already covered. Then you'll need to figure out the right strategy for implementing your change (e.g. make a new component, add a variation to an existing component, or add a subcomponent to an existing component). Then you'll need to create and publish an example, create a new release, document whether it's breaking or non-breaking, upgrade Kibana, and change Kibana's markup to make use of the new component.

On the other hand, this process will force us to take UI changes seriously, and make them in a consistent and scalable manner. I also fully expect to be the one who is doing the majority of this kind of work, at least at first. 😄

Identifying Framework CSS vs Kibana CSS

Great point! We need a way to be able to tell the two apart. How about a kui prefix on all styles coming from the framework?

Changes walk-through

I will go through the code and add some comments explaining the general classes of changes.

cjcenizal · 2016-10-18T15:44:16Z

src/core_plugins/console/public/src/controllers/sense_top_nav_controller.js

@@ -8,22 +8,26 @@ export function SenseTopNavController(Private) {
    {
      key: 'welcome',
      hideButton: true,
-      template: `<sense-welcome></sense-welcome>`
+      template: `<sense-welcome></sense-welcome>`,
+      testId: 'consoleWelcomeButton',


Originally, data-test-subj attributes were added to the Menu buttons implicitly, by using the key value. I added this property so we can be explicit about this value.

cjcenizal · 2016-10-18T15:50:49Z

src/core_plugins/console/public/src/directives/help.html

@@ -1,36 +1,42 @@
-<h3>Help</h3>
+<div class="localDropdownContent localDropdownSection">


Each app has a bunch of partials for the dropdowns that show up when you click on a menu button. I've made the same kinds of changes to each one of these:

Wrapped it in an element with the localDropdownContent class and sometimes localDropdownSection. localDropdownContent adds the light gray styling and some padding around the element. localDropdownSection is typically used as a child of this element but can sometimes be applied to it directly; it just adds a margin at the bottom.

Add a title element if it's missing, using the localDropdownTitle class.

If there are tabs, use the list-group-item list-group-item--noBorder classes. The first class is from Bootstrap, and the second one is a modifier I made to tweak it slightly. I made this change just to make our tabs look consistent, for now. Later, we'll have our own tabs components to replace this.

localDropdownSection is typically used as a child of this element but can sometimes be applied to it directly

Is there guidance regarding when this is necessary? I played around with the example in the UI docs and it seems like everything works just fine with or without it. Could this mean that bottom padding in the localDropdownContent would be sufficient?

I think you're right. We should definitely figure out a way to make these two classes more obvious to use. Let me think about this and try to come up with a solution that doesn't even require guidance. Thanks man.

cjcenizal · 2016-10-18T15:53:33Z

src/core_plugins/kibana/public/dashboard/index.html

-
-        <kbn-typeahead-items></kbn-typeahead-items>
+    <!-- Transcluded elements. -->
+    <div data-transclude-slots>


I've made a significant change to the way kbnTopNav works. It can now transclude content into its top left corner, and into its bottom row, using this idea of "slots", which are identified by the data-transclude-slots and data-transclude-slot attributes. Refer to the localNav examples to see how these areas of the nav are used.

cjcenizal · 2016-10-18T15:54:36Z

src/core_plugins/kibana/public/dashboard/index.html

-    <div class="button-group"></div>
-  </navbar>
+      <!-- Search. -->
+      <form


Most apps have a search bar in the top nav. I typically had to adjust it to use the new localSearch styles, which generally involved removing some markup too.

cjcenizal · 2016-10-18T15:55:05Z

src/core_plugins/kibana/public/dashboard/index.js

@@ -119,7 +119,7 @@ app.directive('dashboardApp', function (Notifier, courier, AppState, timefilter,
        template: require('plugins/kibana/dashboard/partials/save_dashboard.html')
      }, {
        key: 'open',
-        description: 'Load Saved Dashboard',
+        description: 'Open Saved Dashboard',


I made our language more consistent, using the term "Open" instead of "Load".

cjcenizal · 2016-10-18T16:02:20Z

src/core_plugins/kibana/public/dev_tools/partials/dev_tools_app.html

+        <a
+          ng-repeat="item in kbnDevToolsApp.devTools"
+          class="localTab"
+          ng-class="{'localTab-isSelected': kbnDevToolsApp.currentPath === item.url}"


I need to fix the UI Framework to use this localTab-isSelected format. Currently, it's localTab-is-selected. This change will need to apply to localSearch and localMenuItem states as well.

cjcenizal · 2016-10-18T16:04:58Z

src/core_plugins/kibana/public/management/sections/settings/advanced_row.html

@@ -69,12 +69,12 @@
        ng-model="conf.unsavedValue"
        ng-options="option as option for option in conf.options"
        class="form-control"
-        data-test-subj="selectInput">
+        data-test-subj="advancedSetting {{conf.name}} selectInput">


I had to make this kind of change in this file to get the functional tests to pass.

Changing the selectors used in the functional tests is an option too

cjcenizal · 2016-10-18T16:06:59Z

src/core_plugins/kibana/public/visualize/editor/editor.html

-    </div>
-  </kbn-top-nav>
-  <navbar ng-if="chrome.getVisible()" name="visualize-search">
-    <div class="fill bitty-modal-container">


This file required some extra refactoring. Originally, clicking the "unlink" button would show a "bitty-modal" notification. I changed this to use our Notifier service, for consistency.

cjcenizal · 2016-10-18T16:10:43Z

src/core_plugins/timelion/public/app.less

@@ -88,68 +84,41 @@
  }
 }

-
-navbar.timelion-navbar {


Timelion originally had a sticky header. Unfortunately, this wasn't compatible with the new styles. We do want to implement a sticky top nav throughout Kibana, so I think it's best we take it out for now and then re-implement it once we have a general solution in the UI Framework.

cjcenizal · 2016-10-18T16:11:56Z

src/core_plugins/timelion/public/app.less

  }
 }

-timelion-interval {
-  display: flex;
+.timelion-interval--select {


Timelion also has a new component in the top nav which isn't used in other apps: a select menu inline with the search bar. I had to tweak some app-specific styles to make this work.

cjcenizal · 2016-10-18T16:13:30Z

src/ui/public/directives/bread_crumbs.js

@@ -12,10 +12,11 @@ module.directive('breadCrumbs', function () {
    },
    template: breadCrumbsTemplate,
    controller: function ($scope) {
-      $scope.crumbs = chrome.getBreadcrumbs();
+      // Capitalize the first letter of each bread crumb.
+      $scope.breadcrumbs = chrome.getBreadcrumbs().map(breadcrumb => _.startCase(breadcrumb));


This was originally done via CSS, but I think it's a little more discoverable if we do it in JS.

cjcenizal · 2016-10-18T16:15:51Z

src/ui/public/timepicker/kbn_global_timepicker.js

@@ -18,6 +18,7 @@ UiModules

  return {
    template: toggleHtml,
+    replace: true,


This removes an unwanted node from the DOM, letting our CSS work as intended.

We've shied away from using replace: true because it has been deprecated for so long, but it seems that it won't ever be removed in angular 1.x so hooray!

cjcenizal · 2016-10-18T16:16:28Z

tasks/config/licenses.js

        '[email protected]': ['MIT'],
        '[email protected]': ['MIT'],
        '[email protected]': ['MIT'],
        '[email protected]': ['MIT'],
+        '[email protected]': ['Public domain'],
+        '[email protected]': ['MIT'],


@spalger Is this the right way to fix failing license checks?

Yes, except that [email protected] should be Unlicense

cjcenizal · 2016-10-18T16:17:13Z

test/support/page_objects/dashboard_page.js

@@ -14,13 +14,13 @@ export default class DashboardPage {

  clickNewDashboard() {
    return this.findTimeout
-    .findByCssSelector('button.ng-scope[aria-label="New Dashboard"]')
+    .findByCssSelector('[aria-label="New Dashboard"]')


Making our test selectors too specific results in tests that are brittle and tightly-coupled to the DOM.

stacey-gammon · 2016-10-18T19:24:43Z

The 'Next' button in timelion welcome tutorial is missing.

cjcenizal · 2016-10-21T17:46:29Z

@stacey-gammon Thanks! Fixed.

thomasneirynck · 2016-10-24T05:11:07Z

thanks for the extra info, it really helped spot some changes.

+1 for the kui-prefix.

I also learned something new with angular-transclusion, cool!

spalger · 2016-10-24T20:03:29Z

@cjcenizal because of the size of this pull request would you mind not rebasing so that we can stay up to date with changes as they are made? Before merging I wouldn't be opposed to a cleanup-rebase or squash, but if the entire commit list changes on every change I find it a lot harder to follow along.

spalger · 2016-10-24T21:00:03Z

The breadcrumbs in the visualize "wizzard" are a little cramped now:

spalger · 2016-10-24T21:15:09Z

Looks like the timelion docs panel needs its top-margin replaced

spalger

Everything seems functional, and the changes seem good overall, but there are a couple things (and a couple nitpicks) that need to be resolved.

spalger · 2016-10-24T21:36:08Z

src/core_plugins/kibana/public/discover/index.html

+        <div class="localBreadcrumb">
+          <span ng-show="opts.savedSearch.id" class="localBreadcrumb__emphasis">
+            <span data-test-subj="discoverCurrentQuery" ng-bind="::opts.savedSearch.title"></span>
+            <i aria-label="Reload Saved Search" tooltip="Reload Saved Search" ng-click="resetQuery();" class="fa fa-undo small"></i>&nbsp;


This feels like a misuse of  

I think it's OK for now for a couple reasons:

To remove this space we'd need to either create a custom style for this icon that adds margin-left, or we would need to create a general component that does the same. We're in the midst of significantly redesigning this entire "localNav" component, so there doesn't seem to be much benefit to either of these options since we'd probably end up having to delete them.

Icons in this context can generally be treated as typographic elements (subject to the same line-height and font-size rules), so it's OK in my book to use the same tools to apply spacing as you would with text.

spalger · 2016-10-24T21:37:48Z

src/core_plugins/kibana/public/discover/index.html

+      <!-- Search. -->
+      <form
+        data-transclude-slot="bottomRow"
+        style="width: 100%"


Why style this inline?

spalger · 2016-10-24T22:18:36Z

src/ui/public/styles/base.less

@@ -3,6 +3,9 @@
 @import (reference) "./variables";
 @import (reference) "~ui/styles/bootstrap/bootstrap";

+// Kibana UI Framework
+@import (less) "../../../../node_modules/@elastic/kibana-ui-framework/dist/framework.css";


the ~ is what points the require statements to use webpack resolution, so this should be:

@import (less) "~@elastic/kibana-ui-framework/dist/framework.css";

spalger · 2016-10-24T22:26:26Z

test/support/page_objects/discover_page.js

@@ -24,7 +24,7 @@ export default class DiscoverPage {

  getTimespanText() {
    return this.findTimeout
-    .findByCssSelector('.kibana-nav-options .navbar-timepicker-time-desc pretty-duration')
+    .findByCssSelector('[data-test-subj="globalTimepickerRange"]')


This, and the rest of the [data-test-subj="xyz"] selectors should be using PageObjects.common.findTestSubject('xyz')

cjcenizal · 2016-10-25T06:20:46Z

@spalger Addressed your feedback and ready for another review.

spalger · 2016-10-25T08:08:23Z

Looks like this might have gotten hidden a bit in the noise

cjcenizal · 2016-10-25T16:18:17Z

@spalger I reverted the change you requested.

thomasneirynck · 2016-10-26T22:47:09Z

LGTM

- Migrate to new localNav component and subcomponent styles. - Add support for multi-slot transclusion to kbnTopNav. - Update Dashboard, Discover, Management, Visualize, Dev Tools, Timelion to use kbnTopNav consistently. - Update these apps to also display localNavDropdowns consistently, including Timepicker (this mostly meant adding titles). - Add checkLicenses npm script. - Add .fullWidth utility class and use it to make search forms 100% width.

Backports PR #8679 **Commit 1:** Integrate Kibana UI Framework CSS. - Migrate to new localNav component and subcomponent styles. - Add support for multi-slot transclusion to kbnTopNav. - Update Dashboard, Discover, Management, Visualize, Dev Tools, Timelion to use kbnTopNav consistently. - Update these apps to also display localNavDropdowns consistently, including Timepicker (this mostly meant adding titles). - Add checkLicenses npm script. - Add .fullWidth utility class and use it to make search forms 100% width. * Original sha: b5444c4 * Authored by CJ Cenizal <[email protected]> on 2016-10-14T22:30:48Z

Backports PR elastic#8679 **Commit 1:** Integrate Kibana UI Framework CSS. - Migrate to new localNav component and subcomponent styles. - Add support for multi-slot transclusion to kbnTopNav. - Update Dashboard, Discover, Management, Visualize, Dev Tools, Timelion to use kbnTopNav consistently. - Update these apps to also display localNavDropdowns consistently, including Timepicker (this mostly meant adding titles). - Add checkLicenses npm script. - Add .fullWidth utility class and use it to make search forms 100% width. * Original sha: 90bbe5fdc890d7722f84129385c6223dee5c0bfd [formerly b5444c4] * Authored by CJ Cenizal <[email protected]> on 2016-10-14T22:30:48Z Former-commit-id: 2508a42

cjcenizal force-pushed the experiment/ui-framework-css branch 3 times, most recently from ac2c0e6 to 99199da Compare October 17, 2016 20:12

cjcenizal changed the title ~~[WIP] Integrate UI Framework~~ Integrate UI Framework Oct 17, 2016

cjcenizal force-pushed the experiment/ui-framework-css branch from 99199da to e6f6dd4 Compare October 17, 2016 20:31

cjcenizal added review v5.1.0 labels Oct 17, 2016

spalger self-assigned this Oct 17, 2016

thomasneirynck self-assigned this Oct 17, 2016

cjcenizal commented Oct 18, 2016

View reviewed changes

cjcenizal force-pushed the experiment/ui-framework-css branch 2 times, most recently from 1017793 to c20d9fe Compare October 21, 2016 17:42

spalger suggested changes Oct 24, 2016

View reviewed changes

cjcenizal force-pushed the experiment/ui-framework-css branch from 10526dc to 3af669e Compare October 25, 2016 06:18

cjcenizal mentioned this pull request Oct 25, 2016

Add kui prefix. elastic/kibana-ui-framework#25

Closed

spalger approved these changes Oct 26, 2016

View reviewed changes

cjcenizal force-pushed the experiment/ui-framework-css branch from 567c938 to 2cc3aab Compare October 27, 2016 04:39

cjcenizal force-pushed the experiment/ui-framework-css branch from 2cc3aab to b5444c4 Compare October 27, 2016 04:39

cjcenizal merged commit 2e89ced into elastic:master Oct 27, 2016

cjcenizal deleted the experiment/ui-framework-css branch October 27, 2016 04:40

cjcenizal mentioned this pull request Oct 27, 2016

'Open' or 'Load' a Saved Visualization #7902

Closed

cjcenizal added the v6.0.0 label Nov 3, 2016

elastic-jasper mentioned this pull request Nov 3, 2016

[5.x] Integrate UI Framework #8956

Merged

cjcenizal mentioned this pull request Nov 12, 2016

Rebuild top-nav as LocalNav. #8235

Closed

ppisljar added the backported label Dec 2, 2016

epixa added v5.1.1 and removed v5.1.0 labels Dec 8, 2016

rhoboat removed the review label May 31, 2017

		@@ -1,36 +1,42 @@
		<h3>Help</h3>
		<div class="localDropdownContent localDropdownSection">

Integrate UI Framework #8679

Integrate UI Framework #8679

Conversation

cjcenizal commented Oct 13, 2016 • edited Loading

Changes

UI review

Dashboard

Before

After

Discover

Before

After

Management

Before

After

Visualize

Before

After

Dev Tools

Before

After

Timelion

Before

After

spalger commented Oct 17, 2016

cjcenizal Oct 18, 2016 • edited Loading

Choose a reason for hiding this comment

spalger Oct 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasneirynck commented Oct 18, 2016 • edited Loading

cjcenizal commented Oct 18, 2016 • edited by spalger Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal Oct 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal Oct 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stacey-gammon commented Oct 18, 2016

cjcenizal commented Oct 21, 2016

thomasneirynck commented Oct 24, 2016 • edited Loading

spalger commented Oct 24, 2016

spalger commented Oct 24, 2016

spalger commented Oct 24, 2016

spalger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal commented Oct 25, 2016

spalger commented Oct 25, 2016

cjcenizal commented Oct 25, 2016

thomasneirynck commented Oct 26, 2016

cjcenizal commented Oct 13, 2016 •

edited

Loading

cjcenizal Oct 18, 2016 •

edited

Loading

spalger Oct 24, 2016 •

edited

Loading

thomasneirynck commented Oct 18, 2016 •

edited

Loading

cjcenizal commented Oct 18, 2016 •

edited by spalger

Loading

cjcenizal Oct 18, 2016 •

edited

Loading

cjcenizal Oct 18, 2016 •

edited

Loading

thomasneirynck commented Oct 24, 2016 •

edited

Loading