Add 'Select from Project' wizard to allow project templatesto be imported #1966

jeff-phillips-18 · 2017-08-25T11:30:52Z

Requires openshift/origin-web-catalog#394

cc @openshift/team-ux-review

rhamilto

Code changes look ok to me.

I find the spacing between the project picker and the horizontal rule to be a bit tight and uneven.

cshinn

Looks good! I agree with @rhamilto, it doesn't seem like the spacing on the left and right sides of the h.rule are the same

beanh66

LGTM @jeff-phillips-18!

benjaminapetersen · 2017-08-25T17:24:33Z

app/scripts/directives/processTemplateDialog.js

    var ctrl = this;
    var validityWatcher;
+    var projectListWatcher;
+    var MAX_PROJECTS_TO_WATCH = 250;


Interested in this constant, wondering if it should be configurable via constants.js?

Curious if we will actually be watching 250 projects at one time. Thats a lot of watches 😄

I followed the lead from https://github.com/openshift/origin-web-console/blob/master/app/scripts/controllers/projects.js#L22

We should probably define a constant somewhere. I will add a comment for now.

benjaminapetersen · 2017-08-25T17:26:46Z

app/scripts/directives/processTemplateDialog.js

+      id: 'projectTemplates',
+      label: 'Selection',
+      view: 'views/directives/process-template-dialog/process-template-select.html',
+      hidden: ctrl.useProjectTemplate !== true,


ctrl.useProjectTemplate !== true vs !ctrl.useProjectTemplate? Non-boolean options possible?

It's a passed in parameter, I am defaulting to false unless explicitly set to true.

benjaminapetersen · 2017-08-25T17:29:42Z

app/scripts/directives/processTemplateDialog.js

+      return KeywordService.filterForKeywords(items, ['name', 'tags'], KeywordService.generateKeywords(searchText));
+    }
+
+    function filterChange (filters) {


Tiny nit: some inconsistent parens spacing in this file function filterChange (filters) vs function filterItems()

benjaminapetersen · 2017-08-25T17:31:43Z

app/scripts/directives/processTemplateDialog.js

+    }
+
+    function updateFilterControls() {
+      $timeout(function() {


Always have to ask if $ is needed/should be used or if an "angular native" way can be achieved via template bindings.

I've removed the $timeout as it doesn't appear to be necessary.

benjaminapetersen · 2017-08-25T17:33:06Z

app/scripts/directives/processTemplateDialog.js

+          ctrl.filteredItems = filterForKeywords(filter.value, ctrl.filteredItems);
+        });
+      }
+


Curious if order of flow here matters, since what is inside updateFilterControls() is wrapped in $timeout...ie, could this call be the last line in this fn since its contexts will execute later?

Adjusted, not sure what you were getting at but the timeout was unnecessary.

Great! I just have a tendency to pause and ask a question when I see $timeout, they are good at hiding bugs. Wasn't sure about your code here, but figured I'd mention just in case.

jeff-phillips-18 · 2017-08-25T19:07:26Z

I've updated the less to fix the spacing issue for the horizontal rule

jeff-phillips-18 · 2017-08-25T19:08:08Z

spadgett

What happens when I click a builder image. It looks like we're only handling templates?

Templates are probably the 80% case, although we would need to hide image streams if we don't handle builder images in this dialog.

spadgett · 2017-08-25T19:58:08Z

app/scripts/directives/processTemplateDialog.js

+      ctrl.projectEmptyState = {
+        icon: 'pficon pficon-info',
+        title: 'No Project Selected',
+        info: 'Please select a project from the dropdown to load Templates and Images from that project'


nit: we try to use punctuation at the end of sentences in the console when possible

spadgett · 2017-08-25T20:01:39Z

app/scripts/directives/processTemplateDialog.js

+          updateProjectData(projectData);
+
+          if (!ProjectsService.isProjectListIncomplete() && _.size(ctrl.unfilteredProjects) <= MAX_PROJECTS_TO_WATCH) {
+            projectListWatcher = ProjectsService.watch($scope, updateProjectData);


I'm not sure it's worth a watch here. I'd probably just list projects to populate the dropdown. This is consistent with what we do in other project selects in the console.

Removed the watch.

jeff-phillips-18 · 2017-08-25T20:44:38Z

@spadgett Updated to not pull the image streams into the selection list. This dialog will only handle the templates at this point.

benjaminapetersen · 2017-08-29T20:04:31Z

How does it look with lots of templates & images? If you give that user access to openshift namespace & can see a huge list?

Asking because it is a little awkward feeling with just one item, but I bet it looks better when there is more.

jeff-phillips-18 · 2017-08-29T20:05:47Z

@benjaminapetersen I can add a bunch of fake templates if you'd like to see.

spadgett · 2017-08-29T20:08:15Z

app/scripts/directives/processTemplateDialog.js

+      ctrl.templatesEmptyState = {
+        icon: 'pficon pficon-info',
+        title: 'No Templates or Images',
+        info: 'The selected project has no templates or images available to import.'


We'll want to remove "images" from these messages if there are only templates. Maybe

"The selected project has no templates available to instantiate."

(When we add images back, we'll want to say specifically "builder images.")

spadgett · 2017-08-29T20:08:47Z

app/scripts/directives/processTemplateDialog.js

+      ctrl.projectEmptyState = {
+        icon: 'pficon pficon-info',
+        title: 'No Project Selected',
+        info: 'Please select a project from the dropdown to load Templates and Images from that project.'


'Please select a project from the dropdown to load templates from that project.'

spadgett · 2017-08-29T20:10:07Z

app/scripts/directives/processTemplateDialog.js

+      // prevents applying an empty keyword filter
+      // TODO: can remove the following line of code after angular-patternfly issue is fixed:
+      //   https://github.com/patternfly/angular-patternfly/issues/509
+      filters = _.filter(filters, 'value');


I think this can be removed now?

jeff-phillips-18 · 2017-09-06T10:57:33Z

Removed WIP status, ready for final reviews.

… be imported

dtaylor113 · 2017-09-06T12:24:37Z

app/scripts/directives/processTemplateDialog.js

+
+    var updateProjects = function() {
+      var filteredProjects = _.reject(ctrl.unfilteredProjects, 'metadata.deletionTimestamp');
+      var projects = _.sortBy(filteredProjects, $filter('displayName'));


Hi, this _.sortBy(...) is unnecessary as RecentlyViewedProjectsService.orderByMostRecentlyViewed(projects) handles a case insensitive sort of displayName.

spadgett · 2017-09-06T15:17:48Z

[merge]

openshift-bot · 2017-09-06T15:24:45Z

Evaluated for origin web console merge up to 197c465

openshift-bot · 2017-09-06T15:32:49Z

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin_web_console/124/) (Base Commit: 2d2e297) (PR Branch Commit: 197c465)

jeff-phillips-18 requested review from rhamilto, spadgett and cshinn August 25, 2017 11:30

rhamilto approved these changes Aug 25, 2017

View reviewed changes

cshinn approved these changes Aug 25, 2017

View reviewed changes

beanh66 self-requested a review August 25, 2017 17:27

beanh66 approved these changes Aug 25, 2017

View reviewed changes

benjaminapetersen reviewed Aug 25, 2017

View reviewed changes

jeff-phillips-18 force-pushed the browse branch from 977c3aa to 01ac42f Compare August 25, 2017 18:13

spadgett reviewed Aug 25, 2017

View reviewed changes

jeff-phillips-18 force-pushed the browse branch from 01ac42f to 67ab911 Compare August 25, 2017 20:43

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 29, 2017

spadgett reviewed Aug 29, 2017

View reviewed changes

jeff-phillips-18 mentioned this pull request Aug 31, 2017

Bump origin-web-common to 0.0.51 #1992

Merged

jeff-phillips-18 force-pushed the browse branch from 67ab911 to 6ea5a84 Compare September 5, 2017 10:58

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 5, 2017

jeff-phillips-18 force-pushed the browse branch from 6ea5a84 to bf55372 Compare September 6, 2017 10:56

jeff-phillips-18 changed the title ~~[WIP] Add 'Select from Project' wizard to allow project templates/images to be imported~~ Add 'Select from Project' wizard to allow project templates/images to be imported Sep 6, 2017

Add 'Select from Project' wizard to allow project templates/images to…

197c465

… be imported

jeff-phillips-18 force-pushed the browse branch from bf55372 to 197c465 Compare September 6, 2017 10:59

dtaylor113 reviewed Sep 6, 2017

View reviewed changes

dtaylor113 approved these changes Sep 6, 2017

View reviewed changes

spadgett approved these changes Sep 6, 2017

View reviewed changes

spadgett changed the title ~~Add 'Select from Project' wizard to allow project templates/images to be imported~~ Add 'Select from Project' wizard to allow project templatesto be imported Sep 6, 2017

openshift-bot merged commit 73c7420 into openshift:master Sep 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'Select from Project' wizard to allow project templatesto be imported #1966

Add 'Select from Project' wizard to allow project templatesto be imported #1966

jeff-phillips-18 commented Aug 25, 2017 •

edited by spadgett

Loading

rhamilto left a comment

cshinn left a comment

beanh66 left a comment

benjaminapetersen Aug 25, 2017

jeff-phillips-18 Aug 25, 2017

benjaminapetersen Aug 25, 2017

jeff-phillips-18 Aug 25, 2017

benjaminapetersen Aug 25, 2017

jeff-phillips-18 Aug 25, 2017

benjaminapetersen Aug 25, 2017

jeff-phillips-18 Aug 25, 2017

benjaminapetersen Aug 25, 2017

jeff-phillips-18 Aug 25, 2017

benjaminapetersen Aug 25, 2017

jeff-phillips-18 commented Aug 25, 2017

jeff-phillips-18 commented Aug 25, 2017

spadgett left a comment

spadgett Aug 25, 2017

jeff-phillips-18 Aug 25, 2017

spadgett Aug 25, 2017

jeff-phillips-18 Aug 25, 2017

jeff-phillips-18 commented Aug 25, 2017

benjaminapetersen commented Aug 29, 2017

jeff-phillips-18 commented Aug 29, 2017

spadgett Aug 29, 2017

spadgett Aug 29, 2017

spadgett Aug 29, 2017

jeff-phillips-18 commented Sep 6, 2017

dtaylor113 Sep 6, 2017

spadgett commented Sep 6, 2017

openshift-bot commented Sep 6, 2017

openshift-bot commented Sep 6, 2017 •

edited

Loading

Add 'Select from Project' wizard to allow project templatesto be imported #1966

Add 'Select from Project' wizard to allow project templatesto be imported #1966

Conversation

jeff-phillips-18 commented Aug 25, 2017 • edited by spadgett Loading

rhamilto left a comment

Choose a reason for hiding this comment

cshinn left a comment

Choose a reason for hiding this comment

beanh66 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeff-phillips-18 commented Aug 25, 2017

jeff-phillips-18 commented Aug 25, 2017

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

jeff-phillips-18 commented Aug 25, 2017

benjaminapetersen commented Aug 29, 2017

jeff-phillips-18 commented Aug 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeff-phillips-18 commented Sep 6, 2017

Choose a reason for hiding this comment

spadgett commented Sep 6, 2017

openshift-bot commented Sep 6, 2017

openshift-bot commented Sep 6, 2017 • edited Loading

jeff-phillips-18 commented Aug 25, 2017 •

edited by spadgett

Loading

openshift-bot commented Sep 6, 2017 •

edited

Loading