Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Preselect current project in 'Select from Project' #2361

Merged
merged 1 commit into from
Oct 24, 2017
Merged

Preselect current project in 'Select from Project' #2361

merged 1 commit into from
Oct 24, 2017

Conversation

jhadvig
Copy link
Member

@jhadvig jhadvig commented Oct 24, 2017 •
edited
Loading

Preselecting the current project in the Selection step will remove the selectbox from the Configuration step.

7fee4eaa-e28f-44fd-y677-2e1bf20d1eeb

@spadgett PTAL

Closes #2335

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 24, 2017
spadgett
spadgett reviewed Oct 24, 2017
View reviewed changes
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jhadvig Thanks, looks good. Just one comment on the name change.

app/scripts/directives/processTemplateDialog.js Outdated
@@ -184,7 +185,7 @@
};

ctrl.templateProjectChange = function () {
ctrl.templateProjectName = _.get(ctrl.templateProject, 'metadata.name');
ctrl.templateProjectName = _.get(ctrl.selectedProject, 'metadata.name');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of like the original templateProject name better. selectedProject sounds like the project you are instantiating the template into.

@jhadvig
Copy link
Member Author

jhadvig commented Oct 24, 2017

@spadgett comment addressed :)

spadgett
spadgett requested changes Oct 24, 2017
View reviewed changes
app/scripts/directives/processTemplateDialog.js Outdated
@@ -137,7 +138,7 @@
};

$scope.$on('templateInstantiated', function(event, message) {
ctrl.selectedProject = message.project;
ctrl.templateProject = message.project;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jhadvig There are two different projects. We need to make sure we're not getting them mixed up:

  • templateProject -- the source project the template is from
  • selectedProject -- the target project the template is instantiated into

You can selected a project to instantiate into on the landing page, but current project is always used when showing the dialog inside the project.

I think this is probably a two line fix: calling ctrl.templateProjectChange() on init and setting currentProject as the project in the view.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spadgett we need also the change the process-template-select so it uses the ctrl.selectedProject not to use the ctrl.templateProject, cause we are not even initiating that var

@jhadvig jhadvig force-pushed the preselect branch 2 times, most recently from 61bbb8a to 0480c55 Compare October 24, 2017 16:44
@jhadvig
Copy link
Member Author

jhadvig commented Oct 24, 2017

@spadgett updated the PR so it covers all the cases, in the landing page and also in projects.
PTAL

spadgett
spadgett approved these changes Oct 24, 2017
View reviewed changes
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jhadvig

@spadgett
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 24, 2017
@spadgett
Copy link
Member

@jhadvig dist mismatch :/

@spadgett
Copy link
Member

You might need to rebase, install assets, and rebuild?

@openshift-merge-robot openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 24, 2017
@spadgett
Copy link
Member

/lgtm
/hold

Need to wait for #2367 or merge will fail

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Oct 24, 2017
@spadgett
Copy link
Member

/hold cancel
/retest

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 24, 2017
@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit 77f200e into openshift:master Oct 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spadgett spadgett spadgett approved these changes

Assignees

@spadgett spadgett

Labels
lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jhadvig @spadgett @openshift-merge-robot @openshift-ci-robot