Add bindings list to resource pages #1912

jeff-phillips-18 · 2017-08-04T11:58:10Z

Requires origin-web-common release including openshift/origin-web-common#131

Add resourceServiceBindings component.
Use common utility functions for bindings
Add utility SERVICE_CATALOG_ENABLED to CatalogService

@openshift/team-ux-review

ncameronbritt · 2017-08-04T12:19:34Z

Looks good to me with one exception, and it’s my fault for not documenting it clearly: if there are no bindings, we should not show the bindings section for applications (deployments etc) in the overview expanded state. So in the cases of your 2nd and 3rd screen shots, the "Service Bindings" section shouldn't be there. That is the current behavior.

jeff-phillips-18 · 2017-08-04T12:21:01Z

@ncameronbritt OK, I thought we might want it there to allow creation of bindings from the page.

ncameronbritt · 2017-08-04T12:38:28Z

For applications in the overview, "create binding" is available in the kebab if there is a bindable service.

jeff-phillips-18 · 2017-08-04T13:23:45Z

@ncameronbritt updated to not show bindings section on the overview page sections when there are no bindings

dtaylor113 · 2017-08-04T13:27:09Z

app/scripts/services/catalog.js

@@ -2,10 +2,19 @@

 angular.module("openshiftConsole")
  .factory("CatalogService", function($filter,


Should we be concerned that there is already a "CatalogService" in origin-web-catalog? https://github.com/openshift/origin-web-catalog/blob/master/src/services/catalog.service.ts#L4

One is Catalog the other CatalogService, not great but not conflicting.

ah, ok, I see that now in index.ts.

spadgett · 2017-08-07T12:48:45Z

app/scripts/directives/resourceServiceBindings.js

+    DataService.list("secrets", context, null, { errorNotification: false }).then(function(secretData) {
+      ctrl.secrets = secretData.by("metadata.name");
+    });
+  }, 300);


We can build the secret URL from just the name without loading all of the secrets (if that's how this is used). I'd rather not load them if not necessary.

Remove from resourceServiceBindings.js and from overview.js

spadgett · 2017-08-07T12:53:22Z

app/scripts/directives/resourceServiceBindings.js

+      }, {poll: limitWatches, pollInterval: DEFAULT_POLL_INTERVAL}));
+    }
+
+    if (CatalogService.SERVICE_CATALOG_ENABLED && canI({resource: 'instances', group: 'servicecatalog.k8s.io'}, 'watch')) {


This canI check looks wrong

Added comment for this, basically no point watching the serviceclasses if we can't watch serviceinstances (removed the extra check and did it all under the same if)

spadgett · 2017-08-07T13:08:44Z

app/scripts/directives/resourceServiceBindings.js

+  ctrl.serviceClasses = [];
+  ctrl.serviceInstances = [];
+  ctrl.secrets = [];
+  ctrl.showBindings = CatalogService.SERVICE_CATALOG_ENABLED;


I think we should be checking for ENABLE_TECH_PREVIEW_FEATURE.pod_presets as well. If pos presets aren't enabled, we shouldn't try to get the bindings for a resource.

This might be a problem on the overview already as well.

Adding check here and in listRow.js

spadgett · 2017-08-07T13:12:51Z

app/views/directives/resource-service-bindings.html

+  <div ng-if="!($ctrl.bindableServiceInstances | size)">
+    <span>You must have a bindable service in your namespace in order to create bindings.</span>
+    <div>
+      <a href="/">Browse Catalog</a>


I believe this should be

<a href="./">Browse Catalog</a>

at least according to

https://github.com/openshift/origin-web-console#navigation

spadgett · 2017-08-07T13:14:10Z

app/views/directives/resource-service-bindings.html

+      Create Binding
+    </a>
+  </div>
+  <div ng-if="!($ctrl.bindableServiceInstances | size)">


It looks like this section will be totally empty if

There are no bindings

There are bindable instances

I don't have authority to create bindings

We should show a message like "No bindings." in this case.

Added a message "There are no service bindings."

spadgett · 2017-08-07T13:24:33Z

app/views/directives/resource-service-bindings.html

+  </overview-service-binding>
+  <div ng-if="($ctrl.bindableServiceInstances | size) && ({resource: 'bindings', group: 'servicecatalog.k8s.io'} | canI : 'create')">
+    <a href="" ng-click="$ctrl.createBinding()" role="button">
+      <span class="pficon pficon-add-circle-o"></span>


If we add the circle here, we'll need to add it for other things like "Create Autoscaler" etc on these pages.

Needs an aria-hidden="true".

Added aria-hidden. I will check with uxd about using the circle.

spadgett · 2017-08-07T13:25:11Z

app/views/overview/_service-instance-row.html

@@ -19,7 +19,10 @@
            <span ng-if="!row.bindings.length
                        && row.isBindable
                        && ({resource: 'bindings', group: 'servicecatalog.k8s.io'} | canI : 'create')">
-              <a href="" ng-click="row.showOverlayPanel('bindService', {target: row.apiObject})">Create Binding</a>
+              <a href="" ng-click="row.showOverlayPanel('bindService', {target: row.apiObject})">
+                <span class="pficon pficon-add-circle-o"></span>


aria-hidden="true"

Might have missed this one?

spadgett · 2017-08-07T13:25:21Z

app/views/overview/_service-bindings.html

+  <div ng-if="!($ctrl.bindableServiceInstances | size)">
+    <span>You must have a bindable service in your namespace in order to create bindings.</span>
+    <div>
+      <a href="/">Browse Catalog</a>


href="./"

spadgett · 2017-08-07T13:26:40Z

app/views/overview/_service-bindings.html

+      Create Binding
+    </a>
+  </div>
+  <div ng-if="!($ctrl.bindableServiceInstances | size)">


Same comment as above: need a message when there are no bindings, bindable services, and you don't have authority to create a binding.

On the overview page, the service bindings section is not shown at all when there are no service bindings. The create action is in the kebab.

spadgett · 2017-08-07T13:27:09Z

app/views/overview/_service-instance-row.html

@@ -144,7 +149,10 @@
        </div>
        <div class="row" ng-if="row.isBindable && ({resource: 'bindings', group: 'servicecatalog.k8s.io'} | canI : 'create')">
          <div class="col-sm-12">
-            <a href="" ng-click="row.showOverlayPanel('bindService', {target: row.apiObject})">Create Binding</a>
+            <a href="" ng-click="row.showOverlayPanel('bindService', {target: row.apiObject})">
+              <span class="pficon pficon-add-circle-o"></span>


aria-hidden="true"

serenamarie125 · 2017-08-07T19:53:54Z

@spadgett @jeff-phillips-18 Agreed from a consistency perspective.

Although putting this on pages with so many font colors, sizes and weight makes it even more distracting. I'd suggest you add the + and then we have a follow on Visual Design story to look at these pages and see what we can do to make them look less distracting.

jeff-phillips-18 · 2017-08-08T11:59:35Z

Removed WIP status

jeff-phillips-18 · 2017-08-09T15:25:17Z

I believe all review comments have been addressed. @spadgett @dtaylor113 please take another look.

spadgett

LGTM

spadgett · 2017-08-09T16:50:07Z

app/views/overview/_service-instance-row.html

@@ -19,7 +19,10 @@
            <span ng-if="!row.bindings.length
                        && row.isBindable
                        && ({resource: 'bindings', group: 'servicecatalog.k8s.io'} | canI : 'create')">
-              <a href="" ng-click="row.showOverlayPanel('bindService', {target: row.apiObject})">Create Binding</a>
+              <a href="" ng-click="row.showOverlayPanel('bindService', {target: row.apiObject})">
+                <span class="pficon pficon-add-circle-o"></span>


Might have missed this one?

spadgett · 2017-08-09T16:50:23Z

app/views/overview/_service-instance-row.html

@@ -110,7 +115,7 @@
                <span ng-if="(binding | isBindingReady) && ('secrets' | canI : 'get')">
                  <a
                    ng-if="(binding | isBindingReady) && ('secrets' | canI : 'get')"
-                    ng-href="{{row.getSecretForBinding(binding) | navigateResourceURL}}">
+                        ng-href="{{binding.spec.secretName | navigateResourceURL : 'Secret' : row.apiObject.metadata.namespace}}">


nit: indentation looks off?

Fixed. Thanks @spadgett

Add resourceServiceBindings component. Use common utility functions for bindings Add utility SERVICE_CATALOG_ENABLED to CatalogService

spadgett · 2017-08-09T17:40:32Z

[merge]

openshift-bot · 2017-08-09T17:47:14Z

Evaluated for origin web console merge up to eea7670

openshift-bot · 2017-08-09T17:47:21Z

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin_web_console/70/) (Base Commit: e5867ae) (PR Branch Commit: eea7670)

jeff-phillips-18 requested review from rhamilto and spadgett August 4, 2017 11:58

jeff-phillips-18 force-pushed the bind branch from 8aec065 to 84ede00 Compare August 4, 2017 13:22

dtaylor113 reviewed Aug 4, 2017

View reviewed changes

spadgett requested changes Aug 7, 2017

View reviewed changes

jeff-phillips-18 force-pushed the bind branch from 84ede00 to 5e79f0c Compare August 7, 2017 19:11

jeff-phillips-18 force-pushed the bind branch from 5e79f0c to e4dbd7f Compare August 8, 2017 11:24

jeff-phillips-18 changed the title ~~WIP: Add bindings list to resource pages~~ Add bindings list to resource pages Aug 8, 2017

spadgett mentioned this pull request Aug 8, 2017

Service Instance Details Pages #1906

Merged

jeff-phillips-18 force-pushed the bind branch from e4dbd7f to 38fd844 Compare August 8, 2017 19:33

spadgett approved these changes Aug 9, 2017

View reviewed changes

Add bindings list to resource pages

eea7670

Add resourceServiceBindings component. Use common utility functions for bindings Add utility SERVICE_CATALOG_ENABLED to CatalogService

jeff-phillips-18 force-pushed the bind branch from 38fd844 to eea7670 Compare August 9, 2017 17:27

openshift-bot merged commit 215a1b5 into openshift:master Aug 9, 2017

		@@ -2,10 +2,19 @@

		angular.module("openshiftConsole")
		.factory("CatalogService", function($filter,

Add bindings list to resource pages #1912

Add bindings list to resource pages #1912

Uh oh!

Conversation

jeff-phillips-18 commented Aug 4, 2017 • edited by spadgett Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ncameronbritt commented Aug 4, 2017

Uh oh!

jeff-phillips-18 commented Aug 4, 2017

Uh oh!

ncameronbritt commented Aug 4, 2017

Uh oh!

jeff-phillips-18 commented Aug 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serenamarie125 commented Aug 7, 2017

Uh oh!

jeff-phillips-18 commented Aug 8, 2017

Uh oh!

jeff-phillips-18 commented Aug 9, 2017

Uh oh!

spadgett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spadgett commented Aug 9, 2017

Uh oh!

jeff-phillips-18 commented Aug 4, 2017 •

edited by spadgett

Loading

openshift-bot commented Aug 9, 2017 •

edited

Loading