UI: External Source markers #4640

johncowen · 2018-09-06T14:14:26Z

Addition of external source icons for services marked as such.
New %with-tooltip CSS component (WIP)
New 'no healthcheck' icon as external sources might not have
healthchecks
If a service doesn't have healthchecks, then the [Health Checks] tab in
the Service detail page is hidden, we use the [Services] tabs as the
default instead
css-var helper. The idea here is that it will eventually be
replaced with pure css custom properties instead of having to use JS. It
would be nice to be able to build the css variables into the JS at build
time (you'd probably still want to specify in config which variables you
wanted available in JS), but that's possible future work.

Lastly there is probably a tiny bit more testing edits here than usual,
I noticed that there was an area where the dynamic mocking wasn't
happening, it was just using the mocks from consul-api-double, the mocks
I was 'dynamically' setting happened to be the same as the ones in
consul-api-double. I've fixed this here also but it wasn't effecting
anything until actually made certain values dynamic.

I'll try and add some more comments inline to help with review.

Grabs:

1. Addition of external source icons for services marked as such. 2. New %with-tooltip css component (wip) 3. New 'no healthcheck' icon as external sources might not have healthchecks 4. If a service doesn't have healthchecks, then the [Health Checks] tab in the Service detail page is hidden, we use the [Services] tabs as the default instead 5. `css-var` helper. The idea here is that it will eventually be replaced with pure css custom properties instead of having to use JS. It would be nice to be able to build the css variables into the JS at build time (you'd probably still want to specify in config which variables you wanted available in JS), but that's possible future work. Lastly there is probably a tiny bit more testing edits here than usual, I noticed that there was an area where the dynamic mocking wasn't happening, it was just using the mocks from consul-api-double, the mocks I was 'dynamically' setting happened to be the same as the ones in consul-api-double. I've fixed this here also but it wasn't effecting anything until actually made certain values dynamic.

johncowen · 2018-09-06T14:23:07Z

ui-v2/app/controllers/dc/services/index.js

-  thWidth: computed('totalWidth', function() {
-    return widthDeclaration(get(this, 'totalWidth'));
+  totalWidth: computed('maxWidth', function() {
+    return widthDeclaration(get(this, 'maxWidth'));
  }),


The name thWidth no longer makes sense (I was using it just to set the width of the <th> but now I'm also using it to set a <td>, changed the name to totalWidth as thats what it is (move the old totalWidth to maxWidth)

johncowen · 2018-09-06T14:24:38Z

ui-v2/app/helpers/css-var.js

+  '--terraform-color-svg': `url('data:image/svg+xml;charset=UTF-8,<svg viewBox="0 0 16 18" xmlns="http://www.w3.org/2000/svg"><g fill="none" fill-rule="evenodd"><path fill="%235C4EE5" d="M5.51 3.15l4.886 2.821v5.644L5.509 8.792z"/><path fill="%234040B2" d="M10.931 5.971v5.644l4.888-2.823V3.15z"/><path fill="%235C4EE5" d="M.086 0v5.642l4.887 2.823V2.82zM5.51 15.053l4.886 2.823v-5.644l-4.887-2.82z"/></g></svg>')`,
+  '--nomad-color-svg': `url('data:image/svg+xml;charset=UTF-8,<svg viewBox="0 0 16 18" xmlns="http://www.w3.org/2000/svg"><g fill-rule="nonzero" fill="none"><path fill="%231F9967" d="M11.569 6.871v2.965l-2.064 1.192-1.443-.894v7.74l.04.002 7.78-4.47V4.48h-.145z"/><path fill="%2325BA81" d="M7.997 0L.24 4.481l5.233 3.074 1.06-.645 2.57 1.435v-2.98l2.465-1.481v2.987l4.314-2.391v-.011z"/><path fill="%2325BA81" d="M7.02 9.54v2.976l-2.347 1.488V8.05l.89-.548L.287 4.48.24 4.48v8.926l7.821 4.467v-7.74z"/></g></svg>')`,
+  '--consul-color-svg': `url('data:image/svg+xml;charset=UTF-8,<svg viewBox="0 0 18 18" xmlns="http://www.w3.org/2000/svg"><g fill="none" fill-rule="evenodd"><path d="M8.693 10.707a1.862 1.862 0 1 1-.006-3.724 1.862 1.862 0 0 1 .006 3.724" fill="%23961D59"/><path d="M12.336 9.776a.853.853 0 1 1 0-1.707.853.853 0 0 1 0 1.707M15.639 10.556a.853.853 0 1 1 .017-.07c-.01.022-.01.044-.017.07M14.863 8.356a.855.855 0 0 1-.925-1.279.855.855 0 0 1 1.559.255c.024.11.027.222.009.333a.821.821 0 0 1-.642.691M17.977 10.467a.849.849 0 1 1-1.67-.296.849.849 0 0 1 .982-.692c.433.073.74.465.709.905a.221.221 0 0 0-.016.076M17.286 8.368a.853.853 0 1 1-.279-1.684.853.853 0 0 1 .279 1.684M16.651 13.371a.853.853 0 1 1-1.492-.828.853.853 0 0 1 1.492.828M16.325 5.631a.853.853 0 1 1-.84-1.485.853.853 0 0 1 .84 1.485" fill="%23D62783"/><path d="M8.842 17.534c-4.798 0-8.687-3.855-8.687-8.612C.155 4.166 4.045.31 8.842.31a8.645 8.645 0 0 1 5.279 1.77l-1.056 1.372a6.987 6.987 0 0 0-7.297-.709 6.872 6.872 0 0 0 0 12.356 6.987 6.987 0 0 0 7.297-.709l1.056 1.374a8.66 8.66 0 0 1-5.279 1.77z" fill="%23D62783" fill-rule="nonzero"/></g></svg>')`,
+};


Ideally I'd pull these in at build time from the CSS so they are in one place, possible work for the future.

johncowen · 2018-09-06T14:25:34Z

ui-v2/app/styles/base/icons/index.scss

@@ -0,0 +1,3 @@
+$consul-color-svg: url('data:image/svg+xml;charset=UTF-8,<svg viewBox="0 0 18 18" xmlns="http://www.w3.org/2000/svg"><g fill="none" fill-rule="evenodd"><path d="M8.693 10.707a1.862 1.862 0 1 1-.006-3.724 1.862 1.862 0 0 1 .006 3.724" fill="%23961D59"/><path d="M12.336 9.776a.853.853 0 1 1 0-1.707.853.853 0 0 1 0 1.707M15.639 10.556a.853.853 0 1 1 .017-.07c-.01.022-.01.044-.017.07M14.863 8.356a.855.855 0 0 1-.925-1.279.855.855 0 0 1 1.559.255c.024.11.027.222.009.333a.821.821 0 0 1-.642.691M17.977 10.467a.849.849 0 1 1-1.67-.296.849.849 0 0 1 .982-.692c.433.073.74.465.709.905a.221.221 0 0 0-.016.076M17.286 8.368a.853.853 0 1 1-.279-1.684.853.853 0 0 1 .279 1.684M16.651 13.371a.853.853 0 1 1-1.492-.828.853.853 0 0 1 1.492.828M16.325 5.631a.853.853 0 1 1-.84-1.485.853.853 0 0 1 .84 1.485" fill="%23D62783"/><path d="M8.842 17.534c-4.798 0-8.687-3.855-8.687-8.612C.155 4.166 4.045.31 8.842.31a8.645 8.645 0 0 1 5.279 1.77l-1.056 1.372a6.987 6.987 0 0 0-7.297-.709 6.872 6.872 0 0 0 0 12.356 6.987 6.987 0 0 0 7.297-.709l1.056 1.374a8.66 8.66 0 0 1-5.279 1.77z" fill="%23D62783" fill-rule="nonzero"/></g></svg>');
+$nomad-color-svg: url('data:image/svg+xml;charset=UTF-8,<svg viewBox="0 0 16 18" xmlns="http://www.w3.org/2000/svg"><g fill-rule="nonzero" fill="none"><path fill="%231F9967" d="M11.569 6.871v2.965l-2.064 1.192-1.443-.894v7.74l.04.002 7.78-4.47V4.48h-.145z"/><path fill="%2325BA81" d="M7.997 0L.24 4.481l5.233 3.074 1.06-.645 2.57 1.435v-2.98l2.465-1.481v2.987l4.314-2.391v-.011z"/><path fill="%2325BA81" d="M7.02 9.54v2.976l-2.347 1.488V8.05l.89-.548L.287 4.48.24 4.48v8.926l7.821 4.467v-7.74z"/></g></svg>');
+$terraform-color-svg: url('data:image/svg+xml;charset=UTF-8,<svg viewBox="0 0 16 18" xmlns="http://www.w3.org/2000/svg"><g fill="none" fill-rule="evenodd"><path fill="%235C4EE5" d="M5.51 3.15l4.886 2.821v5.644L5.509 8.792z"/><path fill="%234040B2" d="M10.931 5.971v5.644l4.888-2.823V3.15z"/><path fill="%235C4EE5" d="M.086 0v5.642l4.887 2.823V2.82zM5.51 15.053l4.886 2.823v-5.644l-4.887-2.82z"/></g></svg>');


None of these are being used right now, they are just here for completeness sake. As they aren't being used they aren't adding any weight to the CSS.

johncowen · 2018-09-06T14:27:35Z

ui-v2/app/styles/components/icons/index.scss

+  background-repeat: no-repeat;
+  background-size: contain;
+  width: 18px;
+  height: 18px;
 }


Whilst doing this I realised my %pseudo-icon was doing too much. So for the moment I've split it out without changing where I'm actually using it. I'll probably sift through and change all the places I'm using %pseudo-icon to %pseudo-icon-css in a separate PR.

johncowen · 2018-09-06T14:28:06Z

ui-v2/app/styles/components/icons/index.scss

+%with-no-healthchecks::before {
+  @extend %with-minus;
+  border-radius: 20%;
+}


This is the extra little minus icon for when there are no healthchecks for a service

johncowen · 2018-09-06T14:33:09Z

ui-v2/app/templates/dc/nodes/-services.hbs

@@ -15,7 +15,10 @@
        {{/block-slot}}
        {{#block-slot 'row'}}
            <td data-test-service-name="{{item.Service}}">
-              <a href={{href-to 'dc.services.show' item.Service }}>{{item.Service}}{{#if (not-eq item.ID item.Service) }} <em data-test-service-id="{{item.ID}}">({{item.ID}})</em>{{/if}}</a>
+              <a href={{href-to 'dc.services.show' item.Service}}>
+                <span data-test-external-source="{{service/external-source item}}" style="background-image: {{css-var (concat '--' (service/external-source item) '-color-svg') 'none'}}"></span>


The icons themselves are inline CSS. 2 reasons for this:

I want the icons themselves to potentially be configurable from the HTML (not CSS - so no .external-source-consul, .external-source-nomad etc).

The logo itself is actually a decorative/decorational 'thing' - its a design decision that we show an icon not just the word 'consul' - therefore it's done via CSS not an inline SVG. I did um and ahh about this a bit, but this is the side I fell on.

johncowen · 2018-09-06T14:35:48Z

ui-v2/app/templates/dc/nodes/show.hbs

+     (if (gt item.Checks.length 0)  (hash id=(slugify 'Health Checks') partial='dc/nodes/healthchecks') '')
+                                    (hash id=(slugify 'Services') partial='dc/nodes/services')
+     (if tomography                 (hash id=(slugify 'Round Trip Time') partial='dc/nodes/rtt') '')
+                                    (hash id=(slugify 'Lock Sessions') partial='dc/nodes/sessions')


I thought the Health Check tab was disappearing if there were no health checks, but I think we are going to show a 'no health checks' message instead - so there'll be another commit or 2 on top of this yet 😂 . If you get here before I've done that, you can probably ignore a lot of this.

The commits on top of this are done now so the above has gone, please ignore!

johncowen · 2018-09-06T14:37:13Z

ui-v2/tests/acceptance/components/catalog-filter.feature

+    - ID: 'service-2'
+      Port: 1
+      Service: 'service-2'
+      Tags: ['one', 'two', 'three']


This is one place where I noticed I was relying on the mocked data form consul-api-double, so I added properly dynamically configured mocks here.

johncowen · 2018-09-06T14:38:16Z

ui-v2/tests/acceptance/dc/nodes/services/list.feature

+    - terraform
+    - kubernetes
+    - ~
+    ---


Makes sure the externalSource icon is there and correct (~ checks for when it isn't an external source)

johncowen · 2018-09-06T14:39:28Z

ui-v2/tests/helpers/type-to-url.js

@@ -8,7 +8,7 @@ export default function(type) {
      requests = ['/v1/internal/ui/services', '/v1/health/service/'];
      break;
    case 'node':
-      requests = ['/v1/internal/ui/nodes'];
+      requests = ['/v1/internal/ui/nodes', '/v1/internal/ui/node/'];
      break;


This is where it wasn't dynamically setting mocks, this fixes that

When there are no checks, still show the healthchecks tab in the node detail page, but default to the Services tab. The healthcheck tab panel now shows a 'no healthchecks' message

meirish

Nice! Looks really great.

I did notice that the detail page always seems to show the k8s icon (at least on the hosted link) - even for items that don't have an external service meta.

Other than that - what do you think of bumping up the size in the Show header? Just thought that looked a bit small, but not fussed by it too much.

Gonna approve as I don't want to block you, but that seems like a thing that should get sorted before merge (and I wasn't sure if it's different code on the link). Ping me again if you want me to take another look!

meirish · 2018-09-06T21:31:34Z

ui-v2/app/controllers/dc/nodes/show.js

@@ -15,7 +15,7 @@ export default Controller.extend(WithFiltering, {
  },
  setProperties: function() {
    this._super(...arguments);
-    set(this, 'selectedTab', 'health-checks');
+    set(this, 'selectedTab', get(this.item, 'Checks.length') > 0 ? 'health-checks' : 'services');


This seems odd to have side effects in an EmberObject function - maybe drop a comment here about where this gets called so the data flow is clear.

It might be worth looking at setupController hook on routes where you can set things on the controller too (though depending on how your model hooks work that may not always fire, so it might not work for your usecase).

Good point, added in some comments here. From what I understand this method literally called just after I do my setProperties in the route. The big thinking for doing it here and not in the route, is because this variable is only there for frontend/view layer reasons, therefore it should live/be set on the Controller/ViewController. If the View/design should change, then the Route shouldn't be changed, the Controller should.

meirish · 2018-09-06T21:34:44Z

ui-v2/app/helpers/service/external-source.js

@@ -0,0 +1,13 @@
+import { helper } from '@ember/component/helper';


~~Huh cool - never thought to nest helpers - and would've thought that would affect their usage in the app, but from the name of the fn, it looks like it doesn't.~~ Just saw the usage and it does, so never mind this!

Yeah to be honest I was a little undecided about whether to do it like this or not, but I read somewhere LinkedIn was doing something similar with helpers. I went one step further and tried to nest it in with the model name, that way its clear that this is a helper just for service objects/datamodels. {{service-external-source}} would have been just as good, but I suppose this way you have a clear picture from looking at the filesystem also. I might end up doing some more of this later on.

meirish · 2018-09-06T21:52:50Z

ui-v2/app/styles/components/with-tooltip/layout.scss

+  bottom: calc(100% + 5px);
+  text-align: center;
+  white-space: nowrap; //temp
+  content: attr(data-tooltip);


There may be some negative accessibility implications for pseudo element and data attribute based tooltips, but honestly it's been a while since I've looked so I'm not sure if the situation is better now. Just something to be aware of and maybe change in the future if it's not optimal.

meirish · 2018-09-06T21:54:46Z

ui-v2/app/styles/components/with-tooltip/skin.scss

+  display: none;
+}
+%with-tooltip:hover::after,
+%with-tooltip:hover::before {


What do you think about adding :focus too? I'm not sure if things will be in the tab flow of the document (it'll depend on what element you're using these styles with or if you add a tab-index), but I find it nice when you can tab through things on the page and when they get focus you can either toggle or it triggers the tooltip.

meirish · 2018-09-11T04:43:50Z

ui-v2/app/templates/components/healthchecked-resource.hbs

        <strong>{{address}}</strong>
        <a href={{href}}>
            <span>{{name}}</span>
            <em>{{service}}</em>
        </a>
    </header>
-    <ul>
-{{#if status }}
+    {{! its important to keep this <ul> with no whitespace so we can use :empty in css }}


👌Nice comment

Hehe yeah, I could see I future self trying to clean up the markup and adding some nice helpful spacing indenting here and then going ....ooops!! a couple of weeks later 😁

meirish · 2018-09-11T04:50:53Z

ui-v2/tests/integration/helpers/css-var-test.js

+});
+
+// Replace this with your real tests.
+test("it renders nothing if the variable doesn't exist", function(assert) {


Ha I didn't know that the generators create component integration tests for helpers - makes sense though.

It's now only a tooltip because the design says so, which I would guess would make for a better a11y experience

johncowen · 2018-09-11T10:41:22Z

Hey! Cheers @meirish !

I did notice that the detail page always seems to show the k8s icon (at least on the hosted link) - even for items that don't have an external service meta.

Cool yeah, that's just how the mocks work for that page, I guess the salt will be keeping it as the same icon here. I've checked it with the other icon values and all is good when it's on the real thing.

Other than that - what do you think of bumping up the size in the Show header? Just thought that looked a bit small, but not fussed by it too much.

@opihana , what do you think on this? I'm easy either way. I think it's lined in with the text height right now?

Will make a couple more comments inline, and I might shout you later.

Thanks!

John

johncowen added the theme/ui Anything related to the UI label Sep 6, 2018

johncowen requested a review from a team September 6, 2018 14:14

johncowen commented Sep 6, 2018

View reviewed changes

mitchellh mentioned this pull request Sep 6, 2018

agent: aggregate service instance meta for UI purposes #4642

Merged

John Cowen added 3 commits September 7, 2018 10:58

ui: Tweak tooltip text for external sources

cc441f2

ui: Continue to show the healthcheck tab but select the Services one

bf0f01f

When there are no checks, still show the healthchecks tab in the node detail page, but default to the Services tab. The healthcheck tab panel now shows a 'no healthchecks' message

ui: Add minus icons to node cards if there are no healthchecks

ddb6d7f

meirish approved these changes Sep 11, 2018

View reviewed changes

John Cowen added 3 commits September 11, 2018 10:40

ui: Switch to a special case ExternalSources array for the summary

9107485

ui: Add some explanatory comments for the location of selectedTab

38eb8d6

ui: Add the text of the tooltip to the textContent of the page

523323b

It's now only a tooltip because the design says so, which I would guess would make for a better a11y experience

pearkes added this to the 1.2.3 milestone Sep 11, 2018

johncowen merged commit 5ea7480 into master Sep 12, 2018

johncowen deleted the feature/ui-external-sources branch September 12, 2018 19:23

pearkes mentioned this pull request Oct 3, 2018

external-source for UI Integration hashicorp/terraform-provider-consul#68

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI: External Source markers #4640

UI: External Source markers #4640

johncowen commented Sep 6, 2018 •

edited

Loading

johncowen Sep 6, 2018

johncowen Sep 6, 2018

johncowen Sep 6, 2018

johncowen Sep 6, 2018

johncowen Sep 6, 2018

johncowen Sep 6, 2018

johncowen Sep 6, 2018

johncowen Sep 7, 2018

johncowen Sep 6, 2018

johncowen Sep 6, 2018

johncowen Sep 6, 2018

meirish left a comment

meirish Sep 6, 2018

johncowen Sep 11, 2018

meirish Sep 6, 2018

johncowen Sep 11, 2018 •

edited

Loading

meirish Sep 6, 2018

meirish Sep 6, 2018

meirish Sep 11, 2018

johncowen Sep 11, 2018

meirish Sep 11, 2018

johncowen commented Sep 11, 2018

		@@ -0,0 +1,13 @@
		import { helper } from '@ember/component/helper';

UI: External Source markers #4640

UI: External Source markers #4640

Conversation

johncowen commented Sep 6, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meirish 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

johncowen Sep 11, 2018 • 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

johncowen commented Sep 11, 2018

johncowen commented Sep 6, 2018 •

edited

Loading

johncowen Sep 11, 2018 •

edited

Loading