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

UI: External Source markers #4640

Merged
merged 7 commits into from
Sep 12, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ui-v2/app/controllers/dc/nodes/show.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Copy link
Collaborator

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

},
filter: function(item, { s = '' }) {
const term = s.toLowerCase();
Expand Down
10 changes: 5 additions & 5 deletions ui-v2/app/controllers/dc/services/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,17 +37,17 @@ export default Controller.extend(WithHealthFiltering, {
item.hasStatus(status)
);
},
totalWidth: computed('{maxPassing,maxWarning,maxCritical}', function() {
maxWidth: computed('{maxPassing,maxWarning,maxCritical}', function() {
const PADDING = 32 * 3 + 13;
return ['maxPassing', 'maxWarning', 'maxCritical'].reduce((prev, item) => {
return prev + width(get(this, item));
}, PADDING);
}),
thWidth: computed('totalWidth', function() {
return widthDeclaration(get(this, 'totalWidth'));
totalWidth: computed('maxWidth', function() {
return widthDeclaration(get(this, 'maxWidth'));
}),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

remainingWidth: computed('totalWidth', function() {
return htmlSafe(`width: calc(50% - ${Math.round(get(this, 'totalWidth') / 2)}px)`);
remainingWidth: computed('maxWidth', function() {
return htmlSafe(`width: calc(50% - ${Math.round(get(this, 'maxWidth') / 2)}px)`);
}),
maxPassing: computed('items', function() {
return max(get(this, 'items'), 'ChecksPassing');
Expand Down
12 changes: 12 additions & 0 deletions ui-v2/app/helpers/css-var.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 13 additions & 0 deletions ui-v2/app/helpers/service/external-source.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { helper } from '@ember/component/helper';
Copy link
Collaborator

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

@johncowen johncowen Sep 11, 2018

Choose a reason for hiding this comment

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

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.

import { get } from '@ember/object';

export function serviceExternalSource(params, hash) {
const source = get(params[0], 'Meta.external-source');
const prefix = typeof hash.prefix === 'undefined' ? '' : hash.prefix;
if (source) {
return `${prefix}${source}`;
}
return;
}

export default helper(serviceExternalSource);
1 change: 1 addition & 0 deletions ui-v2/app/models/service.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ export default Model.extend({
},
}),
Kind: attr('string'),
Meta: attr(),
Address: attr('string'),
Port: attr('number'),
EnableTagOverride: attr('boolean'),
Expand Down
1 change: 1 addition & 0 deletions ui-v2/app/styles/app.scss
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
@import 'components/confirmation-dialog';
@import 'components/feedback-dialog';
@import 'components/notice';
@import 'components/with-tooltip';

@import 'core/typography';
@import 'core/layout';
Expand Down
3 changes: 3 additions & 0 deletions ui-v2/app/styles/base/icons/index.scss
Original file line number Diff line number Diff line change
@@ -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>');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

1 change: 1 addition & 0 deletions ui-v2/app/styles/base/index.scss
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
@import './decoration/index';
@import './color/index';
@import './typography/index';
@import './icons/index';
42 changes: 35 additions & 7 deletions ui-v2/app/styles/components/icons/index.scss
Original file line number Diff line number Diff line change
@@ -1,15 +1,35 @@
%pseudo-icon {
width: 1em;
height: 1em;
position: absolute;
top: 50%;
margin-top: -0.6em;
/*TODO: The old pseudo-icon was to specific */
/* make a temporary one with the -- prefix */
/* to make it more reusable temporarily */
%--pseudo-icon {
display: block;
content: '';
visibility: visible;
position: absolute;
top: 50%;
background-repeat: no-repeat;
background-position: center center;
}
%pseudo-icon-bg-img {
@extend %--pseudo-icon;
background-size: contain;
background-color: transparent;
}
%pseudo-icon-css {
@extend %--pseudo-icon;
width: 1em;
height: 1em;
margin-top: -0.6em;
background-color: currentColor;
visibility: visible;
}
%pseudo-icon {
@extend %pseudo-icon-css;
}
%with-external-source-icon {
background-repeat: no-repeat;
background-size: contain;
width: 18px;
height: 18px;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

%with-dot {
content: '';
Expand Down Expand Up @@ -135,6 +155,10 @@
@extend %pseudo-icon;
background-image: url('data:image/svg+xml;charset=UTF-8,<svg width="8" height="8" xmlns="http://www.w3.org/2000/svg"><path d="M4 5.064L1.064 8 0 6.936 2.936 4 0 1.064 1.064 0 4 2.936 6.936 0 8 1.064 5.064 4 8 6.936 6.936 8 4 5.064z" fill="%23FFF"/></svg>');
}
%with-minus {
@extend %pseudo-icon;
background-image: url('data:image/svg+xml;charset=UTF-8,<svg width="9" height="2" viewBox="0 0 9 2" xmlns="http://www.w3.org/2000/svg"><path fill="%23FFF" fill-rule="nonzero" d="M0 0h8v2H0z"/></svg>');
}
%with-warning-icon-orange {
@extend %pseudo-icon;
background-image: url('data:image/svg+xml;charset=UTF-8,<svg width="14" height="14" xmlns="http://www.w3.org/2000/svg"><path d="M13.645 10.092c.24.409.365.88.365 1.37 0 1.392-1.027 2.527-2.294 2.538H2.322c-.824 0-1.592-.487-2.004-1.27a2.761 2.761 0 0 1 0-2.538l4.686-8.904C5.416.505 6.184.018 7.008.018c.824 0 1.592.487 2.004 1.27l4.633 8.804zm-5.989 1.264V9.607H6.344v1.749h1.312zm0-3.048v-4.37H6.344v4.37h1.312z" fill="%23fa8f37"/></svg>');
Expand Down Expand Up @@ -183,3 +207,7 @@
@extend %with-cross;
border-radius: 20%;
}
%with-no-healthchecks::before {
@extend %with-minus;
border-radius: 20%;
}
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 is the extra little minus icon for when there are no healthchecks for a service

3 changes: 3 additions & 0 deletions ui-v2/app/styles/components/product/app-view.scss
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@
display: flex;
align-items: flex-start;
}
%app-view h1 span {
@extend %with-external-source-icon;
}
%app-view {
margin-top: 50px;
}
Expand Down
6 changes: 6 additions & 0 deletions ui-v2/app/styles/components/table.scss
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,12 @@ td dt.warning {
td dt.critical {
@extend %with-critical;
}
td span.zero {
@extend %with-no-healthchecks;
display: block;
text-indent: 20px;
color: $ui-gray-400;
}
table:not(.sessions) tr {
cursor: pointer;
}
Expand Down
12 changes: 11 additions & 1 deletion ui-v2/app/styles/components/tabular-collection.scss
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,17 @@ table tr > * {
tr > * dl {
float: left;
}
/* TODO: putting this here is less than ideal */
/* but this is another area where I am specifically */
/* targetting table-like things. This is now a prime */
/* area for a bit of refactoring/reorganizing */
html.template-service.template-list td:first-child a span,
html.template-node.template-show #services td:first-child a span {
@extend %with-external-source-icon;
float: left;
margin-right: 10px;
margin-top: 2px;
}
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 is going to be all split out elsewhere, next time I do a full CSS review/refactor. I'll probably make a '%service-row' component or similar, again best done in a future PR.

html.template-service.template-list main table tr {
@extend %services-row;
}
Expand All @@ -34,7 +45,6 @@ html.template-node.template-show main table tr {
html.template-node.template-show main table.sessions tr {
@extend %node-sessions-row;
}

@media #{$--horizontal-session-list} {
%node-sessions-row > * {
// (100% / 7) - (300px / 6) - (120px / 6)
Expand Down
4 changes: 4 additions & 0 deletions ui-v2/app/styles/components/with-tooltip.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
@import './with-tooltip/index';
%app-view h1 span {
@extend %with-tooltip;
}
2 changes: 2 additions & 0 deletions ui-v2/app/styles/components/with-tooltip/index.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
@import './skin';
@import './layout';
25 changes: 25 additions & 0 deletions ui-v2/app/styles/components/with-tooltip/layout.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
%with-tooltip {
position: relative;
display: inline-flex;
justify-content: center;
}
%with-tooltip::before,
%with-tooltip::after {
position: absolute;
}
%with-tooltip::before {
padding: 10px;
bottom: calc(100% + 5px);
text-align: center;
white-space: nowrap; //temp
content: attr(data-tooltip);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

}
%with-tooltip::after {
content: '';
left: 50%;
margin-left: -5px;
top: -10px;
width: 10px;
height: 10px;
transform: rotate(45deg);
}
17 changes: 17 additions & 0 deletions ui-v2/app/styles/components/with-tooltip/skin.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
%with-tooltip::before,
%with-tooltip::after {
color: $ui-white;
background-color: $ui-gray-800;
}
%with-tooltip::before {
border-radius: $decor-radius-200;
box-shadow: 0 3px 1px 0 rgba($ui-black, 0.12);
}
%with-tooltip::after,
%with-tooltip::before {
display: none;
}
%with-tooltip:hover::after,
%with-tooltip:hover::before {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

display: block;
}
4 changes: 3 additions & 1 deletion ui-v2/app/styles/core/typography.scss
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ td a {
th,
%breadcrumbs a,
%action-group a,
%tab-nav {
%tab-nav,
%with-tooltip::before {
font-weight: $typo-weight-medium;
}
main label a[rel*='help'],
Expand Down Expand Up @@ -86,6 +87,7 @@ td {
font-size: $typo-size-600;
}
th,
%with-tooltip::before,
%healthchecked-resource strong,
%footer {
font-size: $typo-size-700;
Expand Down
5 changes: 4 additions & 1 deletion ui-v2/app/templates/dc/nodes/-services.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

  1. I want the icons themselves to potentially be configurable from the HTML (not CSS - so no .external-source-consul, .external-source-nomad etc).
  2. 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.

{{item.Service}}{{#if (not-eq item.ID item.Service) }}<em data-test-service-id="{{item.ID}}">({{item.ID}})</em>{{/if}}
</a>
</td>
<td data-test-service-port="{{item.Port}}" class="port">
{{item.Port}}
Expand Down
21 changes: 13 additions & 8 deletions ui-v2/app/templates/dc/nodes/show.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,18 @@
{{/block-slot}}
{{#block-slot 'header'}}
<h1>
{{ item.Node }}
{{ item.Node }}
</h1>
{{tab-nav
items=(compact
(array 'Health Checks' 'Services' (if tomography 'Round Trip Time' '') 'Lock Sessions')
(array
(if (gt item.Checks.length 0) 'Health Checks' '')
'Services'
(if tomography 'Round Trip Time' '')
'Lock Sessions'
)
)
selected=(if selectedTab selectedTab 'health-checks')
selected=selectedTab
}}
{{/block-slot}}
{{#block-slot 'actions'}}
Expand All @@ -42,14 +47,14 @@
{{#each
(compact
(array
(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')
(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')
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 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

)
) as |panel|
}}
{{#tab-section id=panel.id selected=(eq (if selectedTab selectedTab 'health-checks') panel.id) onchange=(action "change")}}
{{#tab-section id=panel.id selected=(eq (if selectedTab selectedTab '') panel.id) onchange=(action "change")}}
{{partial panel.partial}}
{{/tab-section}}
{{/each}}
Expand Down
14 changes: 11 additions & 3 deletions ui-v2/app/templates/dc/services/index.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,21 @@
}}
{{#block-slot 'header'}}
<th style={{remainingWidth}}>Service</th>
<th style={{thWidth}}>Node Health</th>
<th style={{totalWidth}}>Node Health</th>
<th style={{remainingWidth}}>Tags</th>
{{/block-slot}}
{{#block-slot 'row'}}

<td data-test-service="{{item.Name}}" style={{remainingWidth}}>
<a href={{href-to 'dc.services.show' item.Name}}>{{item.Name}}</a>
<a href={{href-to 'dc.services.show' item.Name}}>
<span data-test-external-source="{{service/external-source item}}" style="background-image: {{css-var (concat '--' (service/external-source item) '-color-svg') 'none'}}"></span>
{{item.Name}}
</a>
</td>
<td>
<td style={{totalWidth}}>
{{#if (and (lt item.ChecksPassing 1) (lt item.ChecksWarning 1) (lt item.ChecksCritical 1) )}}
<span title="No Healthchecks" class="zero">0</span>
{{else}}
<dl>
<dt title="Passing" class="passing{{if (lt item.ChecksPassing 1) ' zero'}}">Healthchecks Passing</dt>
<dd title="Passing" class={{if (lt item.ChecksPassing 1) 'zero'}} style={{passingWidth}}>{{format_number item.ChecksPassing}}</dd>
Expand All @@ -38,6 +45,7 @@
<dt title="Critical" class="critical{{if (lt item.ChecksCritical 1) ' zero'}}">Healthchecks Critical</dt>
<dd title="Critical" class={{if (lt item.ChecksCritical 1) 'zero'}} style={{criticalWidth}}>{{format_number item.ChecksCritical}}</dd>
</dl>
{{/if}}
</td>
<td class="tags" style={{remainingWidth}}>
{{#if (gt item.Tags.length 0)}}
Expand Down
Loading