Kibana Home page - phase one #14673

nreese · 2017-10-30T19:42:50Z

Adds phase one of Kibana home page - kibana feature directory

formgeist · 2017-10-31T10:20:50Z

@nreese Hope this format is alright. I made a quick visual design feedback screenshot with notes (hover on the dots) – https://redpen.io/vt41e4ce3e14ef316a

I know that a lot of it might be custom styling certain fundamental KUI elements, but hoping we can make it work. Let me know if you have any comments or questions.

cc @snide @alexfrancoeur

snide · 2017-10-31T15:48:39Z

I can do a cleanup whenever this is close.

snide · 2017-10-31T16:57:37Z

Did a quick pair session with @nreese this morning for some cleanup.

stacey-gammon · 2017-10-31T18:58:25Z

package.json

    "react-router": "2.0.0",
    "react-router-redux": "4.0.4",
+    "react-router-dom": "^4.2.2",


I think we want to leave out the ^ part to match the other packages. I think the @elastic/kibana-operations team has a flow for updating these packages.

formgeist · 2017-10-31T19:13:11Z

@nreese @snide The Home page looks great! Had a glance at the Directory pages, and my only suggestion would be to remove the kuiViewContent--constrainedWidth class on the content here too to make it consistent with the Home page.

formgeist

All my visual design feedback and comments have been implemented now, so LGTM 👍

stacey-gammon

nice job!! Code and UI look great, just some minor style comments.

stacey-gammon · 2017-10-31T19:02:58Z

src/core_plugins/kibana/public/assets/app_timelion.svg

@@ -0,0 +1,6 @@
+<svg xmlns="http://www.w3.org/2000/svg" width="32" height="32" viewBox="0 0 32 32">


file isn't used, should we add a timelion box to the directory?

Do a fresh pull. It should be used. Its the icon for timelion. https://github.com/nreese/kibana/blob/home_phase_one/src/core_plugins/timelion/public/register_feature.js#L8

stacey-gammon · 2017-10-31T19:16:36Z

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

+    title: 'Dashboards',
+    description: 'Create visual landing pages made up of content from other apps',
+    icon: '/plugins/kibana/assets/app_dashboard.svg',
+    path: '/app/kibana#/dashboard',


What about creating this url in dashboard_constants? we already have a createDashboardEditUrl and then you can take advantage of the constant LANDING_PAGE_PATH which I think is what we want here instead of the singular dashboard, the plural. If you use the singular it may sometimes direct to the last opened dashboard. That's not terrible, but it's inconsistent with visualize because visualize specifically links to the landing page path.

The reason this is so confusing, the singular vs the plural, is due to BWC issues. heres a comment left on the kibana index registering dashboard as an app:

}, { id: 'kibana:dashboard', title: 'Dashboard', order: -1001, url: `${kbnBaseUrl}#/dashboards`, // The subUrlBase is the common substring of all urls for this app. If not given, it defaults to the url // above. This app has to use a different subUrlBase, in addition to the url above, because "#/dashboard" // routes to a page that creates a new dashboard. When we introduced a landing page, we needed to change // the url above in order to preserve the original url for BWC. The subUrlBase helps the Chrome api nav // to determine what url to use for the app link. subUrlBase: `${kbnBaseUrl}#/dashboard`, description: 'compose visualizations for much win', icon: 'plugins/kibana/assets/dashboard.svg', }

So what we are using for the other links is the url, which is probably what we should be using here too.

stacey-gammon · 2017-10-31T19:17:28Z

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

+    title: 'Visualize',
+    description: 'Build a variety of graphs for your data.',
+    icon: '/plugins/kibana/assets/app_visualize.svg',
+    path: '/app/kibana#/visualize',


we have a LANDING_PAGE_PATH constant we should probably use to create this url.

stacey-gammon · 2017-10-31T19:28:24Z

src/core_plugins/kibana/public/home/components/feature_directory.js

+
+  renderDirectories = () => {
+    return this.props.directories.inTitleOrder
+    .filter((directory) => {


nit, think the .filter and .map functions should be indented by two because it's a line continuation.

stacey-gammon · 2017-10-31T19:36:59Z

src/core_plugins/kibana/public/home/components/home.js

+
+Home.propTypes = {
+  addBasePath: PropTypes.func.isRequired,
+  directories: PropTypes.object.isRequired,


I think what might help decouple this from our angular registry format is to pass directories.inTitleOrder into all these components, and have them just take an array instead of an object.

e.g. down in index.js change
$scope.directories = Private(FeatureCatalogueRegistryProvider);
to
$scope.directories = Private(FeatureCatalogueRegistryProvider).inTitleOrder;

What do you think? Then it'd be easier to express the shape better and you could change PropTypes.object to

PropTypes.arrayOf(PropTypes.shape({ id: PropTypes.string.isRequired, title: PropTypes.string.isRequired, description: PropTypes.string.isRequired, icon... showOnHomePage... category..... })),

stacey-gammon · 2017-10-31T19:48:13Z

src/core_plugins/kibana/public/home/components/home_app.js

+HomeApp.propTypes = {
+  addBasePath: PropTypes.func.isRequired,
+  directories: PropTypes.object.isRequired,
+  directoryCategories: PropTypes.object.isRequired,


I like how this is dynamic so you can easily add categories but I don't think having it as an object will play nicely when typescript comes along (eventually). If you make it an array of strings it would be less flexible in terms of what could get passed and clearer to the reader what the requirements are. Thoughts?

Not sure an array will work. I need to have a named variable to access certain constants. For example, I need to get the directoryCategories.DATA to filter the featureCatalogue by category 'data'

I was thinking something like this:

$scope.directoryCategories = [ FeatureCatalogueCategory.ADMIN, FeatureCatalogueCategory.DATA, FeatureCatalogueCategory.OTHER, ];

But now that I look again, the dynamic aspect of doing it this way wouldn't even work because in FeatureDirectory you check for these three values specifically. I was originally thinking, well if a plugin wants to add a new category, so they register an app with category 'foo' and then you'd pass in 'foo' here, it would automatically create a new tab filled with those categories. Though if you really wanted it dynamic you'd have to have a category name, and id, for display purposes. And now I think I'm just overcomplicating it with these thoughts.

To make it simpler, why pass this as a prop at all, and not just access FeatureCatalogueCategory enum directly?

no reason at all. I will update to just bring these in with an include

alexfrancoeur · 2017-10-31T20:24:22Z

Looks good @nreese! A couple of comments, feedback and questions below.

We should probably get some icons from design for each feature before merging. We're currently using the dashboards icon for saved objects, index patterns, etc. (as per the mocks)

What was the expectation for clicking into a feature? It looks as if navigation is via a link. I would have expected clicking into a full card here to navigate. Thoughts @snide @formgeist?

The latest mock I've seen for the directory can be found here (https://github.com/elastic/x-pack-kibana/issues/2549#issuecomment-334756645). I had always thought these would be represented as "cards" rather than providing the full white space as seen in the directory here. Just curious around the decision process here, is this a limitation of the components?

This

vs this

How difficult is it to add search to the feature directory? During usability testing the feature aspect received a lot of positive feedback from the power users. Search would need to support both the title and short descriptions and filter on the cards that contain them. For example, search for "alerts" would filter on Watcher in X-Pack.

We'll need to finalize the descriptive text for each feature. This was not done for the mockups. I can take this as an action item to provide better descriptions for each feature if it's helpful.
Should we handle responsiveness with the feature directory and force a cell size for each feature? As the window resizes, things get weird. Maybe not a priority for the first phase but thought it'd be something we get out of react for free.

I'll keep digging to provide feedback. I still need to try this out in X-Pack to see how it works.

snide · 2017-10-31T20:37:49Z

@alexfrancoeur

We should probably get some icons from design for each feature before merging.

Agreed. Who has the final list? I can bug marketing to make them for us.

What was the expectation for clicking into a feature?

As cards I would have had the whole "card" clickable. In the presentation here I think at least the icon + title should be clickable. If you do make the surrounding div clickable here, make sure to add proper hover / active states.

I had always thought these would be represented as "cards" rather than providing the full white space as seen in the directory here.

Part of this is due to K6ifying the layout with the components we have. While probably no one more than me would love to use the K7 styling for some of these elements (ex: carding things out), I don't know that we want to maintain two tab styles in the short term to get there. The silver lining is the styling shown in the mocks will be a 5-minute transition once the K7 components are available.

Should we handle responsiveness with the feature directory and force a cell size for each feature?

Yes. @cjcenizal and I have talked a bit about setting min/max sizing to the underlying component used here. SInce @nreese is using the components here, he shouldn't need to do that work now, we'll take care of it in a separate PR.

stacey-gammon · 2017-11-01T13:43:07Z

src/core_plugins/timelion/public/register_feature.js

@@ -0,0 +1,13 @@
+import { FeatureCatalogueRegistryProvider, FeatureCatalogueCategory } from 'ui/registry/feature_catalogue';


hmm, I have this code pulled down locally, but no timelion app is showing up

Do you have the latest? There was a commit or two that had problems with registering timelion but those have been resolved. Let me know if a fresh pull and kibana restart do not fix these.

ah, yes it was the latter step I was missing. Forgot to restart the sever. All good!

formgeist · 2017-11-01T13:49:25Z

@nreese Just spotted a minor bug in the directory button link;

Seems the link doesn't work on the button but only on the text link inside it.

stacey-gammon · 2017-11-01T14:01:22Z

src/core_plugins/kibana/public/home/components/feature_directory.js

+        name: 'Administrative',
+      });
+    }
+    if (props.directories.some(directory => directory.category === props.directoryCategories.OTHER)) {


I think we should consider apps for the 'OTHER' tab as anything not in ADMIN or DATA, so if a plugin registers a directory in category 'stuff', it'll show up under the 'OTHER' tab. Right now it would only show up under the 'All' tab.

stacey-gammon · 2017-11-01T14:02:07Z

src/core_plugins/kibana/public/home/components/feature_directory.js

+        name: 'Explore & Visualize'
+      });
+    }
+    if (props.directories.some(directory => directory.category === props.directoryCategories.ADMIN)) {


Might be overkill to make these conditional. I don't think we'd ever not have stuff in ADMIN and DATA. Up to you.

alexfrancoeur · 2017-11-01T15:11:37Z

Thanks for the quick feedback @snide, I'm starting a list for the feature directory and will share with the group.

After speaking with @nreese & @stacey-gammon, we'll push search functionality for both the feature directory and add data list view to another issue (#14701)

stacey-gammon · 2017-11-01T16:17:24Z

src/core_plugins/kibana/public/home/components/feature_directory.js

+      });
+    }
+
+    this.tabs = tabs;


any reason not to just do this.tabs = [... above and remove this line?

stacey-gammon · 2017-11-01T16:21:45Z

src/core_plugins/kibana/public/home/components/home_app.js

+
+HomeApp.propTypes = {
+  addBasePath: PropTypes.func.isRequired,
+  directories: PropTypes.arrayOf(PropTypes.shape({


I wonder if there is a way to refactor these shapes out. I'm curious because I'm running into something similar and want to do like a custom jsDoc type rather than redefine the shape everywhere. Maybe I'll ask around to see if anyone has any ideas. (no changes required, just thinking out loud).

stacey-gammon

LGTM! Nice work on making this happen in such short time.

cjcenizal · 2017-11-01T16:35:16Z