Adjust radio buttons to drop down selector on create storage page #997

zherman0 · 2016-12-07T16:00:03Z

Removed radio buttons for storage classes in favor of drop down selector in case the number of storage classes is very large

spadgett

Hi @zherman0, a few comments. Can you add a screenshot?

spadgett · 2016-12-07T16:48:05Z

app/views/directives/osc-persistent-volume-claim.html

-        <div class="help-block">
-          Type: {{sclass.parameters.type}} | Zone: {{sclass.parameters.zone}}
-          <span ng-if="sclass.metadata.annotations.description"> | {{sclass.metadata.annotations.description}}</span>
+          <a ng-href="{{'storage_classes' | helpLink}}" target="_blank">Learn more&nbsp;<i class="fa fa-external-link" aria-hidden="true"> </i></a>


We've switched the learn more links to headline case, "Learn More"

spadgett · 2016-12-07T16:49:41Z

app/views/directives/osc-persistent-volume-claim.html

+          </span>
+        </ui-select-match>
+        <ui-select-choices
+          repeat="sclass as sclass in storageClasses | orderBy: 'metadata.name'">


We should enable type ahead filtering. Here's an example:

https://github.com/spadgett/origin-web-console/blob/8fd9c66ca45a40d134a941448b1718d80b998f3a/app/views/modals/link-service.html#L10-L18

spadgett · 2016-12-07T16:52:30Z

app/scripts/directives/oscPersistentVolumeClaim.js

+                description: "No storage class will be assigned unless a default class has been assigned by the system administrator."
+                }
+            }
+          };


My understanding is the default storage class has an annotation, so we should know if there is one and which one it is. I think we can improve the user experience here. Rather than a dummy "no storage class" option, we might just let users leave the field blank and then say in the help text what the default is (when there is a default).

cc @jwforres

@spadgett - how do I create a none option in the drop down. Currently, after you select an option, you cannot unselect. This why I added the "No Storage Class" option. I would like to remove it if the user can choose blank or none in the drop down.

Yeah it's a quirk with ui-select. There is an issue in their repo to add this, but it's not supported natively in their widget. We solved it in osc-secrets.html by adding an x beside the field to clear, although it made more sense there because you could have more than one secret.

I'm OK with "No Storage Class", but we might want to change the label to "Use Default Storage Class" when there is a default.

spadgett · 2016-12-07T16:54:02Z

app/views/directives/osc-persistent-volume-claim.html

+          repeat="sclass as sclass in storageClasses | orderBy: 'metadata.name'">
+          <div>{{ sclass.metadata.name }}</div>
+          <div ng-if="sclass | annotation : 'description'">
+            <small>&nbsp;&nbsp;</small>


Rather than non-breaking spaces, we should use CSS margins.

This can probably all be one small tag.

spadgett · 2016-12-07T16:55:17Z

app/scripts/directives/oscPersistentVolumeClaim.js

+                }
+            }
+          };
+          storageClasses._data["No Storage Class"] = noclass;


You should not be changing _data directly. That is a private field.

It's also cached by DataService and shared across controllers, so anyone else asking for storage classes would get the "no storage class" item you added.

zherman0 · 2016-12-07T20:52:47Z

There are two paths now, one for if a default Storage Class has been defined and one for no default.
Without a default set:

With a default:

spadgett · 2016-12-07T20:57:10Z

Suggest we keep the description on the same line and use class="text-muted" in addition to small. That way every item is the height. Looks like there is enough width.

We might want to stack the description at mobile resolutions, though.

zherman0 · 2016-12-07T23:09:05Z

Made all updated suggestions and created slightly different UX for whether a default storage class was set by admin or not, please note the newly attach images.

spadgett · 2016-12-08T13:10:17Z

app/views/directives/osc-persistent-volume-claim.html

+          repeat="sclass as sclass in storageClasses | toArray | filter : { metadata: { name: $select.search } } | orderBy: 'metadata.name'">
+          <div>{{ sclass.metadata.name }}
+            <span ng-if="sclass | annotation : 'description'" class="text-muted">
+              <small>&nbsp;-


Use &ndash (or maybe —)

<small ng-if="sclass | annotation : 'description'" class="text-muted"> – ... </small>

If there's not enough margin, you could use

<small ng-if="sclass | annotation : 'description'" class="text-muted mar-left-sm"> – ... </small>

spadgett · 2016-12-08T13:14:40Z

app/views/directives/osc-persistent-volume-claim.html

      <div id="claim-storage-class-help" class="help-block mar-bottom-lg">
+        <span ng-if="defaultStorageClass">"{{defaultStorageClass}}" is currently set as the default storage class.<br/></span>


I'd remove the <br> and move this after the description to be consistent with help text we use on other forms.

<div id="claim-storage-class-help" class="help-block mar-bottom-lg"> Storage classes are set by the administrator to define types of storage the users can select. <span ng-if="defaultStorageClass">If not set, default storage class <var>{{defaultStorageClass}}</var> will be used.</span> </div>

I'm curious why mar-bottom-lg is needed. I thought the help-block and learn-more-block styles had appropriate margins. It's better not to set it if we can avoid it so that spacing is consistent across pages.

spadgett · 2016-12-08T13:22:31Z

app/scripts/directives/oscPersistentVolumeClaim.js

+            if ("annotations" in sclass.metadata && defaultClassAnnotation in sclass.metadata.annotations && sclass.metadata.annotations[defaultClassAnnotation] === "true") {
+              scope.defaultStorageClass = sclass.metadata.name;
+            }
+          });


This can be

var annotation = $filter('annotation'); scope.defaultStorageClass = _.find(scope.storageClasses, function(storageClass) { return annotation(storageClass, 'storageclass.beta.kubernetes.io/is-default-class') === 'true'; });

You'll need to add dependency $filter above when declaring the the directive.

Even better to add a isDefaultStorageClass method to StorageService, though. Then this becomes

scope.defaultStorageClass = _.find(scope.storageClasses, StorageService.isDefaultStorageClass);

Note that this sets it to the object rather than the name.

spadgett · 2016-12-08T13:23:37Z

app/scripts/directives/oscPersistentVolumeClaim.js

+              scope.defaultStorageClass = sclass.metadata.name;
+            }
+          });
+          if (scope.defaultStorageClass === "")  { //if there is no default, set a no storage class option


if (!scope.defaultStorageClass) { ... }

I think we can leave the "no storage class" option even if there is a default as long as we say the default will be used in the help text. cc @jwforres opinion?

i havent been following the whole thread, but if we are able to know there is a default then 'no storage class' doesn't really make sense as the option. If we know which storage class is the default I would rather see us pre-select that option and have an indicator like (Default) next to that name.

If we know there is a default but don't know which one is the default, then I would expect "No Storage Class" just becomes "Default storage class"

If we have no way at all to know if there is a default, then yeah we are stuck with "No storage class" and explaining it in the help text that it may select the default if one exists.

If we know which storage class is the default I would rather see us pre-select that option and have an indicator like (Default) next to that name.

I'm fine with that. I wasn't sure if there is ever a reason to leave it blank instead of explicitly setting the default.

spadgett · 2016-12-08T13:25:12Z

app/scripts/directives/oscPersistentVolumeClaim.js

+                name: "No Storage Class",
+                labels: {},
+                annotations: {
+                  description: "No storage class will be assigned unless a default class has been assigned by the system administrator."


This description doesn't make sense if we're only showing the option when there's no default.

spadgett · 2016-12-08T13:29:14Z

app/views/directives/osc-persistent-volume-claim.html

+        </ui-select-match>
+        <ui-select-choices
+          repeat="sclass as sclass in storageClasses | toArray | filter : { metadata: { name: $select.search } } | orderBy: 'metadata.name'">
+          <div>{{ sclass.metadata.name }}


So that the filter match is highlighted:

<div> <span ng-bind-html="sclass.metadata.name | highlight : $select.search"><span> ... </div>

the match is highlighted with the code I have in place, but your suggestion truncates the descriptions

the match is highlighted with the code

I pulled down your changes, and it's not highlighting the matching text.

The truncation is because of this CSS style.

https://github.com/spadgett/origin-web-console/blob/f2c009c13249762921f94576a39b223d5fca3320/app/styles/_select.less#L44

@rhamilto Can we remove or change that style? It's caused problems more than once :/

Sorry, @rhamilto I'm wrong. The problem is a typo because the span is not closed.

<span ng-bind-html="sclass.metadata.name | highlight : $select.search"></span>

spadgett · 2017-01-04T20:59:00Z

@zherman0 ping

zherman0 · 2017-01-04T21:03:13Z

@spadgett - I am getting back to this now, should have the latest round of requests done by tomorrow

zherman0 · 2017-01-11T21:18:24Z

@spadgett - how does this look now?

spadgett · 2017-01-11T21:22:48Z

@zherman0 I don't think I see the updates. Did you forget to git add?

zherman0 · 2017-01-11T22:36:30Z

Try now please

…

On Wed, Jan 11, 2017 at 2:22 PM, Sam Padgett ***@***.***> wrote: @zherman0 <https://github.com/zherman0> I don't think I seeing the updates. Did you forget to git add? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#997 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AR3HmZeS-IVZGYvN2o7qYcQQ8XA3PfAdks5rRUgqgaJpZM4LGw1j> .

-- Zac Herman <[email protected]> Emerging Tech - Senior Software Engineer Red Hat Inc

spadgett · 2017-01-12T13:19:53Z

app/scripts/directives/oscPersistentVolumeClaim.js

+          if (!scope.defaultStorageClass)  { //if there is no default, set a no storage class option
+            var noclass = {
+              metadata: {
+                name: "\u0020No Storage Class",  //unicode char to make this first alphabetically


unicode char to make this first alphabetically

I'd rather sort the list in the controller and prepend the no class option.

DataService.list({group: 'storage.k8s.io', resource: 'storageclasses'}, {}, function(storageClassData) { var storageClasses = storageClassData.by('metadata.name'); scope.storageClasses = _.orderBy(storageClasses, 'metadata.name'); // ... if (!scope.defaultStorageClass) { // ... scope.storageClasses.unshift(noclass); }

Then remove the orderBy from the view.

This will perform better as well since you're not resorting the list on every Angular digest loop, just once when we request it.

hey @spadgett - I am trying to do the change with the _orderBy, but it keeps throwing an exception and I cannot figure out why. The call works fine. I need to do the orderBy to get the object to an array so I can do the unshift call.

Sorry, _.orderBy is lodash v4. We have v3, which uses _.sortBy. Try

scope.storageClasses = _.sortBy(storageClasses, 'metadata.name');

spadgett · 2017-01-12T13:24:29Z

app/views/directives/osc-persistent-volume-claim.html

        Storage classes are set by the administrator to define types of storage the users can select.
+        <span ng-if="defaultStorageClass">  If not set, default storage class <var>{{defaultStorageClass.metadata.name}}</var> will be used.</span>


If I'm reading the code right, there's no way to NOT set a storage class when there's a default since there's no empty option. In which case we don't need this message. But we should make sure the default is initially selected in the controller.

I think the message is still important so users understand why they do not have the option to chose no storage class.

That's fair, but I think we need to adjust the wording still since "If not set" isn't possible. And we should preselect the default.

@spadgett - can you help me set the default? For some reason I cannot get it to work.

spadgett · 2017-01-16T17:07:11Z

app/scripts/directives/oscPersistentVolumeClaim.js

+          if (!scope.defaultStorageClass)  { //if there is no default, set a no storage class option
+            var noclass = {
+              metadata: {
+                name: "No Storage Class",  //unicode char to make this first alphabetically


Remove the comment since it no longer applies

spadgett · 2017-01-16T17:07:20Z

app/scripts/directives/oscPersistentVolumeClaim.js

+                labels: {},
+                annotations: {
+                  description:  "No storage class will be assigned"
+                  }


indentation

spadgett · 2017-01-16T17:07:37Z

app/views/directives/osc-persistent-volume-claim.html

-      <div id="claim-storage-class-help" class="help-block mar-bottom-lg">
+      <label>Storage Class</label>
+      <div>
+      <ui-select


indentation

spadgett · 2017-01-16T17:08:16Z

app/views/directives/osc-persistent-volume-claim.html

+        search-enabled="true"
+        title="Select a storage class"
+        class="select-role"
+        flex>


Missed this on previous review, but I don't think flex is necessary here. Can you try without?

spadgett · 2017-01-16T17:10:34Z

app/views/directives/osc-persistent-volume-claim.html

        Storage classes are set by the administrator to define types of storage the users can select.
+        <span ng-if="defaultStorageClass">  If not set, default storage class <var>{{defaultStorageClass.metadata.name}}</var> will be used.</span>


That's fair, but I think we need to adjust the wording still since "If not set" isn't possible. And we should preselect the default.

spadgett · 2017-01-18T19:03:36Z

can you help me set the default?

Does this work?

scope.defaultStorageClass = _.find(scope.storageClasses, function(storageClass) {
  return annotation(storageClass, 'storageclass.beta.kubernetes.io/is-default-class') === 'true';
});

if (scope.defaultStorageClass) {
  scope.claim.storageClass = scope.defaultStorageClass;  
} else {
  // if there is no default, set a no storage class option
  // ...
}

spadgett · 2017-01-18T21:28:25Z

app/scripts/controllers/createPersistentVolumeClaim.js

@@ -95,7 +95,7 @@ angular.module('openshiftConsole')
            _.set(pvc, 'spec.selector.matchLabels', selectorLabel);
          }
        }
-        if ($scope.claim.storageClass ) {
+        if ($scope.claim.storageClass && $scope.claim.storageClass.metadata.name !== "No Storage Class") {


The controller probably shouldn't know this detail of the pvc directive's implementation, but we can fix in a follow on.

spadgett · 2017-01-18T21:30:01Z

[merge]

openshift-bot · 2017-01-18T21:31:31Z

Evaluated for origin web console merge up to 888a686

openshift-bot · 2017-01-18T21:47:15Z

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/933/) (Base Commit: 04dcf30)

spadgett requested changes Dec 7, 2016

View reviewed changes

spadgett self-assigned this Dec 7, 2016

spadgett requested changes Dec 8, 2016

View reviewed changes

spadgett reviewed Jan 12, 2017

View reviewed changes

spadgett reviewed Jan 16, 2017

View reviewed changes

Adjust radio buttons to drop down selector

888a686

spadgett reviewed Jan 18, 2017

View reviewed changes

spadgett approved these changes Jan 18, 2017

View reviewed changes

openshift-bot merged commit 2cb38e1 into openshift:master Jan 18, 2017

		<div id="claim-storage-class-help" class="help-block mar-bottom-lg">
		<span ng-if="defaultStorageClass">"{{defaultStorageClass}}" is currently set as the default storage class.<br/></span>

		Storage classes are set by the administrator to define types of storage the users can select.
		<span ng-if="defaultStorageClass"> If not set, default storage class <var>{{defaultStorageClass.metadata.name}}</var> will be used.</span>

Adjust radio buttons to drop down selector on create storage page #997

Adjust radio buttons to drop down selector on create storage page #997

Conversation

zherman0 commented Dec 7, 2016

spadgett 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zherman0 commented Dec 7, 2016 • edited Loading

spadgett commented Dec 7, 2016

zherman0 commented Dec 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

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

spadgett commented Jan 4, 2017

zherman0 commented Jan 4, 2017

zherman0 commented Jan 11, 2017

spadgett commented Jan 11, 2017 • edited Loading

zherman0 commented Jan 11, 2017 via email

spadgett Jan 12, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spadgett commented Jan 18, 2017

Choose a reason for hiding this comment

spadgett commented Jan 18, 2017

openshift-bot commented Jan 18, 2017

openshift-bot commented Jan 18, 2017 • edited Loading

zherman0 commented Dec 7, 2016 •

edited

Loading

spadgett commented Jan 11, 2017 •

edited

Loading

spadgett Jan 12, 2017 •

edited

Loading

openshift-bot commented Jan 18, 2017 •

edited

Loading