List mobile service to mobile client connections by sedroche · Pull Request #3021 · openshift/origin-web-console

sedroche · 2018-06-12T07:23:51Z

Displaying a list of mobile services that can be bound to a mobile client.

A binding can be created from within the component.

Each service will have configuration created during a binding which will be displayed to the user in this component.

NOTE: This PR makes use of openshift/origin-web-common#334 to identify specific bindings between client <-> service
and openshift/origin-web-common#320
and openshift/origin-web-common#341

benjaminapetersen · 2018-06-15T18:11:48Z

@openshift/team-ux-review

benjaminapetersen · 2018-06-15T20:41:06Z

@cshinn I think similar items related to button placement, etc.

spadgett · 2018-06-18T13:03:31Z

/ok-to-test

cshinn · 2018-06-18T13:45:45Z

Separating the unbound services more strongly would help get them out of the way for people who don't necessarily want to bind all of their services to a given mobile client. They wouldn't need to expand, but a short message like the PR screenshot could be useful if it's easier for them to expand. In that case, I think it would look better for the button to be fixed width relative to the text inside rather than sized based on the grid.

sedroche · 2018-06-19T12:01:02Z

@cshinn Thanks Chris. I'll start on these changes now.

benjaminapetersen · 2018-06-20T14:18:45Z

+
+      return {
+        generateName: consumerId.toLowerCase() + '-' + _.get(serviceClass, 'spec.externalMetadata.serviceName').toLowerCase() + '-',
+        annotations: {


I believe the annotations $filter can be used to get & set annotations.

Yes, I will update this.

Annotations are already ready to go -> openshift/origin-web-common#341

benjaminapetersen · 2018-06-20T14:23:17Z


      var serviceClass = BindingService.getServiceClassForInstance(svcToBind, ctrl.serviceClasses);
-      BindingService.bindService(svcToBind, application, serviceClass, ctrl.parameterData).then(function(binding){
+


I don't know if other kinds of services would cause this function to bloat over time, but it may be wise to branch early:

// call separate functions early, which can share another function if necessary to deduplicate. ctrl.bindService = function() { if (ctrl.isMobileEnabled && isMobileService(serviceClass)) { bindMobileService(); } else { bindService(); } }

benjaminapetersen · 2018-06-20T14:25:09Z

+      Navigate.toProjectCatalog($routeParams.project, {category: 'mobile', subcategory: 'services'});
+    };  
+
+    watches.push(DataService.watch(APIService.getPreferredVersion("clusterserviceclasses"), {namespace: $routeParams.project}, function(serviceClasses){


Should this be within $onInit?

var watchClusterServiceClasses = _.once(function() { ... }); ctrl.$onInit = function() { watchClusterServiceClasses(); // etc. }

benjaminapetersen · 2018-06-20T14:26:12Z

+
+    watches.push(DataService.watch(APIService.getPreferredVersion("clusterserviceclasses"), {namespace: $routeParams.project}, function(serviceClasses){
+      ctrl.serviceClasses = serviceClasses.by("metadata.name");
+      watches.push(DataService.watch(APIService.getPreferredVersion("serviceinstances"), {namespace: $routeParams.project}, function(serviceInstances){


I would probably suggest putting the var serviceInstanceVersion = APIService.getPreferredVersion('serviceinstances') at the top. For the most part we do that everywhere.

Yep, I will update these

benjaminapetersen · 2018-06-20T14:27:24Z

+    watches.push(DataService.watch(APIService.getPreferredVersion("clusterserviceclasses"), {namespace: $routeParams.project}, function(serviceClasses){
+      ctrl.serviceClasses = serviceClasses.by("metadata.name");
+      watches.push(DataService.watch(APIService.getPreferredVersion("serviceinstances"), {namespace: $routeParams.project}, function(serviceInstances){
+        ctrl.serviceInstances = _.filter(serviceInstances.by('metadata.name'), function(serviceInstance){


try running grunt jshint, just to ensure spacing & what not won't fail CI. I think there may be a rule about function() {.

There doesn't seem to be a rule but i have updated this style anyway

benjaminapetersen · 2018-06-20T14:28:21Z

+    var extendedAnnotationSpace = "org.aerogear.binding-ext." + this.apiObject.metadata.name;
+    var watches = [];
+    var bindingsWatch = false;
+    row.bindingsLimit = _.get(row.serviceClass, "spec.externalMetadata.bindingsLimit", 1);


row.$onInit? Not sure if there is a risk on a race condition here, for things like row.bindingsLimit.

Possibly applies to anything that deals with this.apiObject as well.

Even if not a race condition issue, its probably a better convention.

yes, makes sense. Will update.

benjaminapetersen · 2018-06-20T14:38:21Z

+    };
+
+    row.showOverlayPanel = function(panelName, state) {
+      row.parameterData = _.reduce(row.bindData, function(acc, current) {


parameterData is passed in as an attribute, just want to check & verify you do need 2-way data binding here.

No, I believe one way is correct. The child component only uses parameterData

benjaminapetersen · 2018-06-20T14:39:50Z

+    };
+
+    row.showOverlayPanel = function(panelName, state) {
+      row.parameterData = _.reduce(row.bindData, function(acc, current) {


Nit: i think we typically use result for the accumulator var in a _.reduce. That, or a name that describes the object being built, in this case something like paramData.

benjaminapetersen · 2018-06-20T14:40:54Z

+  .mobile-service-instance-row {
+    .icon {
+      clear: both;
+      color: #888;


Any $pf- var for the color?

benjaminapetersen · 2018-06-20T14:41:16Z

+          margin: 0 10px;
+          display: block;
+          float: left;
+          font-size: 1.6em;


$pf- var for font-size?

Could you point to where the $pf- vars are defined please?

The console's own vars are in styles/_variables.less. The paths to the PF files we are importing can be found in styles/main.less. That will be in bower_components/patternfly/. A good start would be patternfly/src/less/variables.less.

benjaminapetersen · 2018-06-20T14:41:48Z

+      <div class="list-pf-name">
+        <h3>
+          <span class="logo" ng-if="row.serviceClass.spec.externalMetadata.imageUrl">
+            <img src="{{row.serviceClass.spec.externalMetadata.imageUrl}}">


title or alt on the img for accessibility purposes?

sedroche · 2018-06-22T13:36:32Z

@cshinn I have updated this view with your suggested changes. Hopefully everything looks good, let me know if you require any more changes.

Unbound Services

Bound and unbound

Both Bound

Creating binding flow

Deleting binding flow

sedroche · 2018-06-22T13:42:13Z

@benjaminapetersen I have applied some feedback here if you can take a second look. Cheers.

cshinn · 2018-06-22T17:33:08Z

@sedroche looks good! I'm not sure if this is the correct PR to address it, but it'd probably be good to get mobile service icons with transparent backgrounds so that they don't look off when hovered/selected. Could also save time to skip the modal when there's only one choice of binding.

beanh66 · 2018-06-25T13:01:20Z

@cshinn We removed the section header called "DETAILS" for the CNV flows. I believe @rhamilto pointed out that this style is reserved for specific items such as BUILDS, NETWORKING, etc. Wondering if the same applies here.
See CNV design:

cshinn · 2018-06-25T14:01:41Z

@beanh66 that's a good point. @sedroche in cases where there's only one set of data under a bound service, it probably makes sense to leave that out to be consistent

openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 12, 2018

sedroche mentioned this pull request Jun 12, 2018

Add base mobile client view #3022

Closed

sedroche force-pushed the connected-services-list branch from 8f04aa9 to a4fff6f Compare June 14, 2018 16:06

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 14, 2018

benjaminapetersen self-requested a review June 15, 2018 18:15

openshift-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 18, 2018

beanh66 mentioned this pull request Jun 18, 2018

Assign metadata on binding creation openshift/origin-web-common#334

Closed

benjaminapetersen reviewed Jun 20, 2018

View reviewed changes

benjaminapetersen suggested changes Jun 20, 2018

View reviewed changes

sedroche added 2 commits June 22, 2018 13:30

List mobile service to mobile client connections

2f0d790

Initial review feedback

f9fc32d

sedroche force-pushed the connected-services-list branch from a4fff6f to 77357ea Compare June 22, 2018 12:53

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 22, 2018

sedroche added 2 commits June 22, 2018 14:20

Missed annotation

221fab5

build

3b3e6a0

sedroche force-pushed the connected-services-list branch from 77357ea to 3b3e6a0 Compare June 22, 2018 13:22

sedroche closed this Jun 22, 2018

sedroche reopened this Jun 22, 2018

sedroche closed this Jun 28, 2018

sedroche deleted the connected-services-list branch June 29, 2018 09:01


		var serviceClass = BindingService.getServiceClassForInstance(svcToBind, ctrl.serviceClasses);
		BindingService.bindService(svcToBind, application, serviceClass, ctrl.parameterData).then(function(binding){

Conversation

sedroche commented Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benjaminapetersen commented Jun 15, 2018

Uh oh!

benjaminapetersen commented Jun 15, 2018

Uh oh!

spadgett commented Jun 18, 2018

Uh oh!

cshinn commented Jun 18, 2018

Uh oh!

sedroche commented Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

sedroche commented Jun 22, 2018

Unbound Services

Bound and unbound

Both Bound

Creating binding flow

Deleting binding flow

Uh oh!

sedroche commented Jun 22, 2018

Uh oh!

cshinn commented Jun 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

beanh66 commented Jun 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cshinn commented Jun 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

sedroche commented Jun 12, 2018 •

edited

Loading

sedroche commented Jun 19, 2018 •

edited

Loading

cshinn commented Jun 22, 2018 •

edited

Loading

beanh66 commented Jun 25, 2018 •

edited

Loading

cshinn commented Jun 25, 2018 •

edited

Loading