Added common label formatting (as chips) and unified across the project. #223

floreks · 2016-01-05T16:33:38Z

This is how it looks:

I've changed default chips styling to fit application style. I think that similar look can be applied to deploy page labels if needed. Only 1 key/value input row would be left and all added labels would be displayed below as list of chips.

@bryk @maciaszczykm what do you think?

codecov-io · 2016-01-07T08:45:47Z

Current coverage is `76.06%`

Merging #223 into master will increase coverage by +0.61% as of d1b50a8

@@            master    #223   diff @@
======================================
  Files           54      56     +2
  Stmts          493     493       
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit            372     375     +3
  Partial          0       0       
+ Missed         121     118     -3

Review entire Coverage Diff as of d1b50a8

Powered by Codecov. Updated on successful CI builds.

bryk · 2016-01-07T09:25:28Z

src/app/frontend/common/components/label/label_controller.js

+   * @ngInject
+   */
+  constructor() {
+    /** @export {!Array<Object.<string,string>>} Initialized from the scope and transformed to


No need for the dot: Object.<.

bryk · 2016-01-07T09:31:24Z

src/app/frontend/common/components/label/label_controller.js

+   * @return {!Array<Object.<string,string>>}
+   * @private
+   */
+  transformLabels_() {


I believe you dont need the transformation. In md-chip-template you should have $chip and $index variables. Please verify.

See docs: https://material.angularjs.org/latest/api/directive/mdChips

The reason why I'm doing this is because md-chips needs array and what we get from backend is 1 object with lots of properties like {app: 'name', version: 'v1'}. I'm transforming object into array and md-chips iterates over this array.

It's based on this demo:
https://material.angularjs.org/latest/demo/chips

How about using static chips, as at the bottom of the demo list: https://material.angularjs.org/latest/demo/chips ?

The reason I'm pushing this is the fact that this logic is not needed and non trivial. I.e., the controller is initialized with labels of type Foo which are then converted to type Bar. Meaning that labels are of two types, depending on when you look at them.

This was not as easy as I thought. I've had to fix karma ng-html2js configuration. It was not working. There were 2 issues:

stripPrefix path was wrong. It is merged with karma basePath and as result it was ../../src/app/frontend/. I've changed basePath to point to current folder as all our conf.paths have basePath in them.

moduleName was pointing to non existing variable. It was undefined and not loaded. I've changed it to be loaded with ng module so it doesn't have to be mocked manually while writing tests.

Additionally in order to make md-chips work without additional label transformation I've had to use workaround. There are issues reported with making ng-repeat and md-chips work.

angular/material#3482
angular/material#2829

Last thing is that even after fixing karma and compiling directive it didn't show any md-chip in html code. It looks like another issue with md-chips directive. I've only checked if outer loop works and x md-chips tags are rendered.

I'm not sure if you have seen above comment because it's hidden. Can you confirm? @bryk

bryk · 2016-01-07T09:39:33Z

Looks really nice. I've got a few comments on style. PTAL.

maciaszczykm · 2016-01-07T09:45:01Z

src/app/frontend/common/components/label/label.scss

+    border-radius: 0;
+    font-size: $caption-font-size-base;
+    height: $caption-font-size-base + 6;
+    line-height: $caption-font-size-base + 6;


This value (+ 6) repeats, can we export it to variable?

How should it be called because I don't know. This is something like custom margin that we have in other files without extracting to variable.

I was thinking about local variable, which will show, that these values are somehow connected (equal). However, it's not mandatory.

Sure. It can be refactored later when some appropriate name is proposed. Feel free to change it later in some PR. :)

maciaszczykm · 2016-01-07T09:50:18Z

Got only two minor comments regarding stylesheets. LGTM

floreks · 2016-01-07T10:11:04Z

PTAL.

Also what do you think about trying to apply similar look to labels in deploy page.

bryk · 2016-01-07T10:37:25Z

PTAL

maciaszczykm · 2016-01-07T10:50:55Z

LGTM

floreks · 2016-01-07T14:23:44Z

PTAL

bryk · 2016-01-07T16:01:23Z

CC @kenmoore

bryk · 2016-01-07T16:01:43Z

CC Ken, if you have any comments.

bryk · 2016-01-08T08:41:54Z

build/karma.conf.js

@@ -49,7 +49,7 @@ function getFileList() {
 */
 module.exports = function(config) {
  let configuration = {
-    basePath: conf.paths.base,
+    basePath: '.',


Thank you a lot for fixing this!

bryk · 2016-01-08T09:54:29Z

Sorry for long review, but I'm experimenting with look and feel. I've a couple of questions/notes.

The problem I see is that the labels are too prominent and eye-catching. They are, kind of, secondary element of a card, yet they are bold and very visible. So I've attempted to make them less prominent. What I did is I reverted to rounded edges. And changed background to the same body has. Also, changed space between key and value to =.

Take a look and tell me what you think.

bryk · 2016-01-08T09:54:49Z

@maciaszczykm @floreks See my previous comment.

bryk · 2016-01-08T09:56:26Z

src/app/frontend/replicasetdetail/replicasetdetail.scss

@@ -26,7 +27,8 @@ $replicasetdetails-table-cell: #777;

 .kd-replicasetdetail-sidebar {
  background-color: $replicasetdetails-sidebar-bg;
-  min-width: 315px;
+  max-width: $replicasetdetails-sidebar-width;


I believe this caused a bug:

Ops actually I've set $replicasetdetails-sidebar-width variable to 315 instead of 315px.

maciaszczykm · 2016-01-08T10:06:27Z

@bryk I agree, that your proposal should be used. It's true, that bold label names were more visible than different (and more important) parts of view.

floreks · 2016-01-08T10:10:22Z

Makes sense to me. They should not overwhelm user. I've changed it. PTAL

bryk · 2016-01-08T11:01:26Z

I did pre-merge verification of this and found troubling issues. Basically, md-chips is not designed for our use case. It is designed to work on input elements. Static chips work pretty bad, and even the docs say that this component is likely to change.

First thing I noticed is that every md-chips load asynchronously. See that in serve:prod the page does not load in one shot. Instead, the page loads and then chips load after a short while. Please test it out. I did an investigation in chrome and see what's happening:

See more at: https://github.com/angular/material/blob/d1b7f79e92beced635190d23c307435aabcde9c5/src/components/chips/js/chipsDirective.js#L316

How about just styling our class to look like chips? In my tests it is actually less code and less CSS than current solution.

The template:

<div ng-repeat="(key, value) in ::labelsCtrl.labels">
  <div class="kd-labels-chip">
    {{key}}={{value}}
  </div>
</div>

CSS copied and adjusted from chips.

What do you think?

floreks · 2016-01-08T12:05:56Z

Definitely. I didn't notice that. Anyway loading chips async does not fit our use case. I agree. Styling this to look as a chip should not be a problem. I'll change this. Thanks for taking a closer look at this.

floreks · 2016-01-08T12:50:41Z

PTAL

bryk · 2016-01-08T13:37:46Z

src/app/frontend/common/components/labels/labels.scss

+.kd-labels {
+  background-color: $body;
+  border-radius: $label-height / 2;
+  color: $muted;


You made the color $muted. How about removing color attribute here and just use default. $muted over $body is very hard to read. Normal over $body is easier to read.

What do you think?

I was thinking about this. Pure black seemed a bit too distracting comparing to the label background. Font color has to be set anyway because it will be overriden by other components if it's not set. Maybe something in the middle like #444?

If you omit color here it would be application default. Which is not full black, but dark grey.

md-content.md-default-theme, md-content { color: rgba(0,0,0,0.87); }

Yes, but f.e. if I remove color at all from css then on replica set list page it will be black and on replica set details gray, because it inherits from more detailed classes like kd-replicasetdetail-sidebar-subline

I see. Can you take something darker from variables.scss? Or define there a variable for rgba(0,0,0,0.87). Or apply md-content class.

bryk · 2016-01-08T13:38:16Z

Looks great. I have last comment, I promise :)

floreks · 2016-01-08T14:24:55Z

I've surrounded it with md-content and rgba(0,0,0,0.87) color is inherited. PTAL

bryk · 2016-01-08T14:26:19Z

Awesome. Thank you. Will merge soon.

Added common label formatting (as chips) and unified across the project.

Fixed Object listing page to match changed Backend response

floreks force-pushed the label-format branch from 7d8543c to 5135ca9 Compare January 7, 2016 08:11

googlebot added the cla: yes label Jan 7, 2016

floreks force-pushed the label-format branch 2 times, most recently from 3c30d0c to 45867b2 Compare January 7, 2016 08:40

floreks force-pushed the label-format branch 2 times, most recently from 06bf501 to 747fc5f Compare January 7, 2016 09:22

bryk reviewed Jan 7, 2016
View reviewed changes

floreks force-pushed the label-format branch from 747fc5f to 7b701ba Compare January 7, 2016 09:31

bryk reviewed Jan 7, 2016
View reviewed changes

maciaszczykm reviewed Jan 7, 2016
View reviewed changes

floreks force-pushed the label-format branch from 7b701ba to 2216d5c Compare January 7, 2016 10:04

floreks force-pushed the label-format branch 2 times, most recently from 9f53b28 to e14a69e Compare January 7, 2016 10:26

floreks force-pushed the label-format branch from e14a69e to 619f6c6 Compare January 7, 2016 13:47

bryk reviewed Jan 8, 2016
View reviewed changes

floreks force-pushed the label-format branch from 619f6c6 to 52d22ce Compare January 8, 2016 09:39

bryk reviewed Jan 8, 2016
View reviewed changes

floreks force-pushed the label-format branch from 52d22ce to 0971aa8 Compare January 8, 2016 10:04

floreks force-pushed the label-format branch from 0971aa8 to 3c4ec29 Compare January 8, 2016 10:12

floreks force-pushed the label-format branch 3 times, most recently from 746e650 to d2b0026 Compare January 8, 2016 12:47

bryk reviewed Jan 8, 2016
View reviewed changes

Added common label formatting (as chips) and unified across the project.

3c47f67

floreks force-pushed the label-format branch from d2b0026 to 3c47f67 Compare January 8, 2016 14:23

bryk added a commit that referenced this pull request Jan 8, 2016

Merge pull request #223 from floreks/label-format

d1ac65e

Added common label formatting (as chips) and unified across the project.

bryk merged commit d1ac65e into kubernetes:master Jan 8, 2016

bryk deleted the label-format branch January 8, 2016 14:35

anvithks pushed a commit to anvithks/k8s-dashboard that referenced this pull request Apr 10, 2020

Merge pull request kubernetes#223 from anvithks/development

4a14daa

Fixed Object listing page to match changed Backend response

Added common label formatting (as chips) and unified across the project. #223

Added common label formatting (as chips) and unified across the project. #223

Conversation

floreks commented Jan 5, 2016

codecov-io commented Jan 7, 2016

Current coverage is 76.06%

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

bryk commented Jan 7, 2016

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

maciaszczykm commented Jan 7, 2016

floreks commented Jan 7, 2016

bryk commented Jan 7, 2016

maciaszczykm commented Jan 7, 2016

floreks commented Jan 7, 2016

bryk commented Jan 7, 2016

bryk commented Jan 7, 2016

Choose a reason for hiding this comment

bryk commented Jan 8, 2016

bryk commented Jan 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maciaszczykm commented Jan 8, 2016

floreks commented Jan 8, 2016

bryk commented Jan 8, 2016

floreks commented Jan 8, 2016

floreks commented Jan 8, 2016

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

bryk commented Jan 8, 2016

floreks commented Jan 8, 2016

bryk commented Jan 8, 2016

Current coverage is `76.06%`