Render SerializedReference annotations in a more userfriendly way. #1325

rf232 · 2016-10-11T13:39:58Z

Render SerializedReference annotations in a more userfriendly way.

part of #1306, this one just makes the SerializedReference annotations into links to the referenced object

see the attachment for a preview of how this looks.

Some rendering errors and perhaps not the most beautiful code, but hey it works.

Somewhere during the coding there existed an actual AnnotationsController that derived from LabelsController, but in the end it had no own logic, so just reuse the LabelsController

codecov-io · 2016-10-11T13:59:41Z

Current coverage is 93.83% (diff: 100%)

No coverage report found for master at 7a9e35a.

Powered by Codecov. Last update 7a9e35a...7dc41eb

olekzabl · 2016-10-11T14:20:45Z

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

@@ -0,0 +1,43 @@
+// Copyright 2015 Google Inc. All Rights Reserved.


Since you explained to me that this is a copy of another CSS file (with some changes), I'd prefer to avoid copying that much (for easier maintenance of changes in the future), use the original CSS in your new template, and restrict this CSS file only to the real changes. But these are only my feelings, ignore them if yours are different.

olekzabl · 2016-10-11T14:23:06Z

src/app/frontend/common/components/serializedreference/serializedreference_controller.js

+    /** @export {string} */
+    this.reference;
+
+    /** {!ui.router.$state} */


Is this annotation valid? (So far, I've only seen ones with "@Private")

olekzabl · 2016-10-11T14:23:37Z

src/app/frontend/common/components/serializedreference/serializedreference_controller.js

+    $scope.$watch(() => this.reference, () => this.recalculateDerivedProperties());
+  }
+
+  recalculateDerivedProperties() {


Can you add a comment?

olekzabl

Minor comments arrived, PTAL

Made a overarching set of class names for the currently used styles in labels and annotations, name refers back to the md-chip origin of the visual design of them.

rf232 · 2016-10-17T08:18:36Z

Minor rework arrived, PTAL :)

olekzabl · 2016-10-18T12:51:37Z

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

@@ -1,4 +1,4 @@
-// Copyright 2015 Google Inc. All Rights Reserved.
+// Copyright 201666666 Google Inc. All Rights Reserved.


It's great to plan for such a long period ;)

olekzabl · 2016-10-18T13:00:20Z

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

-  display: block;
-  padding: ($baseline-grid / 4) $baseline-grid;
-}
-
 kd-annotations {


Just curiousity: why is this CSS out of unification?

olekzabl · 2016-10-19T07:20:49Z

src/test/frontend/common/components/annotations/annotations_directive_test.js

+    // then
+    let labels = element.find('kd-middle-ellipsis');
+    expect(labels.length).toEqual(3);
+    angular.forEach((key, value, index) => {


Missing first argument? (What are you iterating over?)
Same in 1 place below.

hmm, I stole that snippet from the label_directive tests, lets see if those are broken too :)

olekzabl · 2016-10-19T07:22:40Z

src/test/frontend/common/components/annotations/annotations_directive_test.js

+      expect(labels.eq(index).text()).toBe(`${key}=${value}`);
+    });
+  });
+  it('should render 1 annotation of created-by kind as serialized reference', () => {


Separate "it"s by a newline.

olekzabl · 2016-10-19T07:32:07Z

src/test/frontend/common/components/serializedreference/serializedreference_controller_test.js

+    expect(ctrl.valid).toBe(false);
+    expect(ctrl.state_.href).not.toHaveBeenCalled();
+  });
+  it('should set the state to invalid on a non SerializedReference object', () => {


Separate "it"s by a newline.

olekzabl · 2016-10-19T07:34:31Z

src/test/frontend/common/components/serializedreference/serializedreference_controller_test.js

+    });
+
+    // when
+    ctrl.recalculateDerivedProperties();


Won't "scope.$digest()" have the same effect?
If it will, I'd prefer to call "scope.$digest()", as it shows more clearly that you're testing what will happen anyway, without further interactions with the component.
(Same in 2 places below).

olekzabl

A few more comments.

olekzabl · 2016-10-19T07:38:26Z

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

@@ -0,0 +1,20 @@
+// Copyright 201666666 Google Inc. All Rights Reserved.


It's good to plan for such a long time :)

olekzabl · 2016-10-19T07:38:47Z

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

-  display: block;
-  padding: ($baseline-grid / 4) $baseline-grid;
-}
-
 kd-labels {


Just out of curiosity: why does this not participate in the unification?

because this is the element name and not a class name that i could unify. perhaps I could add this to a class and apply class to both. Not sure if that is worth it though

I don't fully understand - but you have a reason, so I'm ok with this :)

olekzabl

Sorry, I'm still often fooled by those tools.
I've had two more comments which apparently disappeared, so I'm sending them once again.

rf232 · 2016-10-21T08:52:24Z

@floreks @maciaszczykm @bryk any of you want to bless this?

bryk

Look and feel is great! :) A few cosmetic comments on coding practices.

bryk · 2016-10-21T14:03:08Z

src/app/frontend/common/components/annotations/annotations.html

+       ng-if="annotationsCtrl.isVisible($index)" class="kd-chips">
+    <div ng-switch on="key">
+      <div ng-switch-when="kubernetes.io/created-by" class="kd-chip">
+        Created by: <kd-serialized-reference reference="value">


We need i18n here :)

bryk · 2016-10-21T14:03:44Z

src/app/frontend/common/components/annotations/annotations.html

+  <div class="kd-chips kd-chips-switch" ng-show="annotationsCtrl.isMoreAvailable()"
+       ng-click="annotationsCtrl.switchLabelsView()">
+    {{annotationsCtrl.isShowingAll() ?
+      annotationsCtrl.i18n.MSG_LABELS_SHOW_LESS_TOOLTIP : annotationsCtrl.i18n.MSG_LABELS_SHOW_ALL_TOOLTIP}}


Should we use the new i18n here?

bryk · 2016-10-21T14:05:52Z

src/app/frontend/common/components/serializedreference/serializedreference_directive.js

+ *
+ * @return {!angular.Directive}
+ */
+export default function serializedReferenceDirective() {


Can you make this a component? This has been our standard practice so far. Everything is a component, unless it uses some of the directive features.

There are examples in the codebase to inspire on. Basically, you turn this into an object instead of a function returning an object. Delete some lines and change referencectrl to $ctrl.

bryk

Last trivial comment and let's merge! :)

bryk · 2016-10-24T09:28:08Z

src/app/frontend/common/components/annotations/annotations.html

@@ -19,7 +19,7 @@
       ng-if="annotationsCtrl.isVisible($index)" class="kd-chips">
    <div ng-switch on="key">
      <div ng-switch-when="kubernetes.io/created-by" class="kd-chip">
-        Created by: <kd-serialized-reference reference="value">
+        [[Created by:|Friendly name of the kubernetest.io/created-by annotation]] <kd-serialized-reference reference="value">


s/kubernetest/kubernetes

bryk · 2016-10-24T09:53:37Z

LGTM, please merge when travis is happy :)

dropping my i18n messages for easier resolution

rf232 added 4 commits October 7, 2016 14:05

Fix minor typo in resourcedetail infocard

2185b1c

way to show 'created-by' annotations

f8b675a

Some rendering errors and perhaps not the most beautiful code, but hey it works.

rendering looks nice now

49242a9

LabelsController is final again.

bd57460

Somewhere during the coding there existed an actual AnnotationsController that derived from LabelsController, but in the end it had no own logic, so just reuse the LabelsController

rf232 assigned mijajasinski and olekzabl Oct 11, 2016

googlebot added the cla: yes label Oct 11, 2016

rf232 changed the title ~~1306 creator info pod~~ Render SerializedReference annotations in a more userfriendly way. Oct 11, 2016

olekzabl reviewed Oct 11, 2016

View reviewed changes

rf232 added 3 commits October 11, 2016 17:29

Aligned scss for labels and annotations

e437b0d

Made a overarching set of class names for the currently used styles in labels and annotations, name refers back to the md-chip origin of the visual design of them.

serialized reference now signals if the derived properties are valid

4109a26

added tests for the new directives and controller

091806d

olekzabl reviewed Oct 18, 2016

View reviewed changes

olekzabl reviewed Oct 19, 2016

View reviewed changes

rf232 added 3 commits October 20, 2016 11:42

Cleaning up test code

6f36420

Switch to scope.digest instead of manually calling the function

95a2b80

make the code less futuristic

be7050b

fix label tests to actually test something

ecb4f58

olekzabl approved these changes Oct 20, 2016

View reviewed changes

fix lint errors

34a64ba

bryk suggested changes Oct 21, 2016

View reviewed changes

rf232 added 6 commits October 21, 2016 17:54

i18n for Created By:

4a3feba

serialized reference from directive to component

1f7e1ca

annotations use 'newstyle' i18n

838711f

lint is my friend

534c90c

Merge branch 'master' into 1306-creatorInfoPod

ceaf624

Merge branch 'master' into 1306-creatorInfoPod

7dc41eb

bryk suggested changes Oct 24, 2016

View reviewed changes

s/kubernetest/kubernetes

6f3d9ad

bryk approved these changes Oct 24, 2016

View reviewed changes

rf232 added 2 commits October 24, 2016 15:07

Merge branch 'master' into 1306-creatorInfoPod

ce7a24b

dropping my i18n messages for easier resolution

back from a short break, my i18n messages

8f4415b

rf232 merged commit cc43ffe into kubernetes:master Oct 24, 2016

rf232 deleted the 1306-creatorInfoPod branch October 24, 2016 13:38

maciaszczykm mentioned this pull request May 20, 2021

Restore serialized reference component #6090

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Render SerializedReference annotations in a more userfriendly way. #1325

Render SerializedReference annotations in a more userfriendly way. #1325

rf232 commented Oct 11, 2016 •

edited

Loading

codecov-io commented Oct 11, 2016 •

edited

Loading

olekzabl Oct 11, 2016 •

edited

Loading

olekzabl Oct 11, 2016

olekzabl Oct 11, 2016

olekzabl left a comment

rf232 commented Oct 17, 2016

olekzabl Oct 18, 2016

olekzabl Oct 18, 2016 •

edited

Loading

olekzabl Oct 19, 2016 •

edited

Loading

rf232 Oct 20, 2016

olekzabl Oct 19, 2016 •

edited

Loading

rf232 Oct 20, 2016

olekzabl Oct 19, 2016

rf232 Oct 20, 2016

olekzabl Oct 19, 2016 •

edited

Loading

rf232 Oct 20, 2016

olekzabl left a comment

olekzabl Oct 19, 2016

olekzabl Oct 19, 2016 •

edited

Loading

rf232 Oct 20, 2016

olekzabl Oct 20, 2016

olekzabl left a comment

rf232 commented Oct 21, 2016

bryk left a comment

bryk Oct 21, 2016

rf232 Oct 21, 2016

bryk Oct 21, 2016

rf232 Oct 21, 2016

bryk Oct 21, 2016

rf232 Oct 21, 2016

bryk left a comment

bryk Oct 24, 2016

bryk commented Oct 24, 2016

		@@ -0,0 +1,43 @@
		// Copyright 2015 Google Inc. All Rights Reserved.

		@@ -1,4 +1,4 @@
		// Copyright 2015 Google Inc. All Rights Reserved.
		// Copyright 201666666 Google Inc. All Rights Reserved.

		@@ -0,0 +1,20 @@
		// Copyright 201666666 Google Inc. All Rights Reserved.

Render SerializedReference annotations in a more userfriendly way. #1325

Render SerializedReference annotations in a more userfriendly way. #1325

Conversation

rf232 commented Oct 11, 2016 • edited Loading

codecov-io commented Oct 11, 2016 • edited Loading

Current coverage is 93.83% (diff: 100%)

olekzabl Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olekzabl left a comment

Choose a reason for hiding this comment

rf232 commented Oct 17, 2016

Choose a reason for hiding this comment

olekzabl Oct 18, 2016 • edited Loading

Choose a reason for hiding this comment

olekzabl Oct 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olekzabl Oct 19, 2016 • 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

olekzabl Oct 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olekzabl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olekzabl Oct 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olekzabl left a comment

Choose a reason for hiding this comment

rf232 commented Oct 21, 2016

bryk 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryk commented Oct 24, 2016

rf232 commented Oct 11, 2016 •

edited

Loading

codecov-io commented Oct 11, 2016 •

edited

Loading

olekzabl Oct 11, 2016 •

edited

Loading

olekzabl Oct 18, 2016 •

edited

Loading

olekzabl Oct 19, 2016 •

edited

Loading

olekzabl Oct 19, 2016 •

edited

Loading

olekzabl Oct 19, 2016 •

edited

Loading

olekzabl Oct 19, 2016 •

edited

Loading