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

Add breadcrumb navigation at the top of linkerd dashboard #1613

Merged
merged 10 commits into from
Sep 12, 2018

Conversation

dadjeibaah
Copy link
Contributor

@dadjeibaah dadjeibaah commented Sep 7, 2018

This PR adds a breadcrumb style navigation to the Linkerd dashboard. Each "crumb" links to its corresponding page in the UI.

This PR also includes a small UI fix in the sidebar. The select box always seems to revert to the All Namespaces option when ever there is a state change on the react side. The fix ensures that the select box always displays the namespace filter if it is available and revert to All Namespaces when no namespace is selected.

screen shot 2018-09-11 at 3 33 16 pm

fixes #1464
fixes #1543
fixes #1627

Signed-off-by: Dennis Adjei-Baah dennis@buoyant.io

@siggy
Copy link
Member

siggy commented Sep 7, 2018

awesome!

following:

DOCKER_TRACE=1 bin/fast-build && bin/linkerd install | kubectl apply -f - && curl https://run.linkerd.io/emojivoto.yml | bin/linkerd inject - | kubectl apply -f -

... then...

bin/linkerd dashboard

i get some breadcrumbs...

screen shot 2018-09-07 at 4 22 06 pm

... if i click on the last breadcrumb, Service mesh, i get...

screen shot 2018-09-07 at 4 22 13 pm

@dadjeibaah
Copy link
Contributor Author

Oh that's right I forgot about the Kubernetes proxy URL. Thanks for checking that. I will get that fixed.

Copy link

@rmars rmars left a comment

Choose a reason for hiding this comment

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

😻 this looks really cool. I like the navigation that this provides!

I think we need more visual distinction between the top bar and the sidebar, otherwise the page feels un-symmetric.
I also think we should make the header bigger and bolder, more like the page title, kind of like how it is in the mocks. I think this should be doable!
Also echoing Siggy's comment, we'll need to test that this works with the path prefix.

web/app/js/components/Sidebar.jsx Show resolved Hide resolved
@@ -0,0 +1,17 @@
@import 'styles.css';

.ant-layout-header {
Copy link

Choose a reason for hiding this comment

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

some visual feedback:
I think this would look more like a part of the header if it were 84px tall ( same height as the linkerd logo container).

@import 'styles.css';

.ant-layout-header {
background-color: var(--header-black);
Copy link

Choose a reason for hiding this comment

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

opinion: I think it would look less weird if the header colour here and the sidebar were different. Like maybe a gray or green for this part? The mocks use #26e99d which I think doesn't look too bad. (Though it would be really really nice if we could also change our green/reds we use for the status dots to also match this.)

screen shot 2018-09-07 at 4 18 38 pm


export default withContext(BreadcrumbHeader);


Copy link

Choose a reason for hiding this comment

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

nit: extra newline

}
}

renderBreadcrumbSegment(segment, index) {
Copy link

Choose a reason for hiding this comment

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

we have few enough routes that we can get away with a straight up map of routes to title we want to display rather than special casing a bunch of things in this way, e.g.

  const routeToCrumbTitle = {
    "servicemesh": "Service Mesh",
    "replicationcontroller": "Replication Controller"
  }

let titleToDisplay = routeToCrumbTitle[routeSegment] || _.startCase(routeSegment)

}
}

.ant-layout-header:last-child {
Copy link

Choose a reason for hiding this comment

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

I think it would be cool if breadcrumbs were super big, like h1 sized, and bold. And then we could remove the page headers from each page!

Something like:

@import 'styles.css';

.ant-layout-header {
  background-color: #26e99d;
  height: 84px;
  z-index: 100;
  position: fixed;
  width: 100vw;
  padding-top: 20px;

  & span.ant-breadcrumb-link, & span.ant-breadcrumb-separator, & :first-child {
     color: black;
     font-size: 24px;
     font-weight: 700;
  }
}

@dadjeibaah
Copy link
Contributor Author

@rmars, I made the changes you suggested. experimented with reducing the font weight of the breadcrumbs and kept the font relatively large. Let me know if we need to do more experimentation. @siggy fixed the URL issue as well.

Copy link
Member

@siggy siggy left a comment

Choose a reason for hiding this comment

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

awesome! shipit modulo ci

Copy link

@rmars rmars left a comment

Choose a reason for hiding this comment

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

niceee! just had a couple more comments

z-index: 100;
position: fixed;
width: 100vw;
padding:16px 50px;
Copy link

Choose a reason for hiding this comment

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

you'll want this padding to be 16px 40px to match that on main-content

@@ -23,7 +23,7 @@
--font-weight-regular: 400;
--font-weight-bold: 700;
--font-weight-extra-bold: 900;
--base-width: 8px;
--base-width: 5px;
Copy link

Choose a reason for hiding this comment

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

this is the base unit of all the grid spacing we use in the app... so changing this results in a lot of things in the UI being spaced differently. is there a particular adjustment you wanted to make with this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This must have been some experiment I was trying to do. I don't think I actually need this. I will revert this back to its old value.

let PrefixedLink = this.api.PrefixedLink;
let prefix = this.props.pathPrefix;

let breadcrumbs = _.isNil(prefix) ?
Copy link

Choose a reason for hiding this comment

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

pathPrefix has a default value of "" so you could just do

let breadcrumbs = this.convertURLToBreadcrumbs(this.props.location.pathname.replace(prefix, ""));

without the isNil check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh nice. Thanks!

let singular = singularResource(name);
// If we end up getting the same resource name after running it through
// toShortResourceName, the resource is not in the shortNameLookup.
return toShortResourceName(singular) !== singular;
Copy link

Choose a reason for hiding this comment

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

I think return _.has(shortNameLookup, singular) is easier to understand here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that much better! I think if we use _.has then we no longer need the comment.

@@ -0,0 +1,33 @@
import Adapter from 'enzyme-adapter-react-16';
import {Breadcrumb} from 'antd';
Copy link

Choose a reason for hiding this comment

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

spacing nit { Breadcrumb }

import {Breadcrumb} from 'antd';
import BreadcrumbHeader from '../js/components/BreadcrumbHeader.jsx';
import { BrowserRouter } from 'react-router-dom';
import {expect} from 'chai';
Copy link

Choose a reason for hiding this comment

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

similar spacing nit (it's more obvious when we have a mixture of { ... } and {...} :P)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So sorry about these import spacing issues, my editor tries to be nice by removing the spaces. I will see how I can get it to not do this.

import { BrowserRouter } from 'react-router-dom';
import {expect} from 'chai';
import React from 'react';
import Enzyme, {mount} from 'enzyme';
Copy link

Choose a reason for hiding this comment

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

same here

let isMeshResource = isResource(segment);

if (isMeshResource) {
return routeToCrumbTitle[segment] || friendlyTitle(segment).singular;
Copy link

Choose a reason for hiding this comment

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

I think you wanted the || singular to show, for example Namespace > emojivoto rather than Namespaces > emojivoto, but looking at the implementation of friendlyTitle, it assumes the resource you pass it is the singular version... so friendlyTitle("namespaces") results in {singular: "Namespaces", plural: "Namespacess"}....

It's also kind of complicated because if you're on the Namespaces list page you do want it to say Namespaces rather than the singular Namespace.
This could be solved by not trying to get the singular resource name if there is only one item in breadcrumbs, and otherwise looking up the singular version.
It could also be solved by adding a check to friendlyTitle that the resource is always singular (and modifying it if it's not).

Dennis Adjei-Baah added 8 commits September 11, 2018 15:38
Signed-off-by: Dennis Adjei-Baah <dennis@buoyant.io>
Signed-off-by: Dennis Adjei-Baah <dennis@buoyant.io>
Signed-off-by: Dennis Adjei-Baah <dennis@buoyant.io>
Signed-off-by: Dennis Adjei-Baah <dennis@buoyant.io>
Signed-off-by: Dennis Adjei-Baah <dennis@buoyant.io>
Signed-off-by: Dennis Adjei-Baah <dennis@buoyant.io>
Signed-off-by: Dennis Adjei-Baah <dennis@buoyant.io>
Signed-off-by: Dennis Adjei-Baah <dennis@buoyant.io>
Signed-off-by: Dennis Adjei-Baah <dennis@buoyant.io>
Copy link

@rmars rmars left a comment

Choose a reason for hiding this comment

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

Nice colour choices! Had a few small comments, but after that, should be good to go!

height: 80px;
padding: calc(var(--base-width)*2) 14px;
height: 60px;
padding: calc((var(--base-width) - 3)*2) 14px;
Copy link

Choose a reason for hiding this comment

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

can we make this padding: var(--base-width) 14px;?

@@ -12,9 +12,9 @@
--coldgrey: #c9c9c9;
--neutralgrey: #828282;
--silver: #BDBDBD;
--green: #27AE60;
--siennared: #EB5757;
Copy link

Choose a reason for hiding this comment

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

can we change the name of siennared to red? While it is a rather arbitrary name, I doubt it matches the original sianna red now anyway :)

let singular = _.startCase(resource);
if (resource === "replicationcontroller") {
singular = _.startCase("replication controller");
let singleResource = singularResource(resource);
Copy link

Choose a reason for hiding this comment

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

naming nit, how about naming singleResource resource instead, and calling the function argument inputResource or singularOrPluralResource or something to make its purpose clearer? The fact that it's a single resource doesn't really matter much throughout the rest of the function?

Signed-off-by: Dennis Adjei-Baah <dennis@buoyant.io>
@dadjeibaah dadjeibaah merged commit b1181e5 into master Sep 12, 2018
@dadjeibaah dadjeibaah deleted the dad/follow-the-breadcrumbs branch September 12, 2018 16:22
@@ -90,7 +90,7 @@ ul.ant-menu.ant-menu-sub {

& .sidebar-menu-header {
height: 60px;
padding: calc((var(--base-width) - 3)*2) 14px;
padding: calc(var(--base-width)) 14px;
Copy link

Choose a reason for hiding this comment

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

there's no need for the calc here... since we're not doing any calculations withvar(--base-width)... it's doesn't do anything, but this should preferrably be padding: var(--base-width) 14px;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, sorry, I should have picked that out. 😦

dadjeibaah pushed a commit that referenced this pull request Sep 12, 2018
This PR is a result of a change request that was missed in PR #1613. This change removes an unnecessary calc() function in the sidebar.css

Signed-off-by: Dennis Adjei-Baah <dennis@buoyant.io>
zachalbert added a commit to zachalbert/linkerd2 that referenced this pull request Sep 13, 2018
* master:
  Update CHANGES.md for v18.9.1 release (linkerd#1631)
  Cleanly shutdown tap stream to data plane proxies (linkerd#1624)
  Change breadcrumb header to default font in styles.css (linkerd#1633)
  Improve top table to better cope with high RPS traffic (linkerd#1634)
  Add small success rate chart to table, misc web tweaks (linkerd#1628)
  Consolidate the source and destination columns in the Tap and Top tables (linkerd#1620)
  remove extraneous calc function in sidebar.css (linkerd#1632)
  Display more helpful websocket errors (linkerd#1626)
  Add breadcrumb navigation at the top of linkerd dashboard (linkerd#1613)
  Introduce inject check for known sidecars (linkerd#1619)
  Bump Prometheus to v2.4.0, Grafana to 5.2.4 (linkerd#1625)
  Improve performance of tap table by throttling updates (linkerd#1623)
  Add with-source flag to top (linkerd#1614)

Conflicts:
	web/app/css/styles.css
	web/app/js/components/ResourceDetail.jsx
	web/app/js/index.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants