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.

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

Angularize the VmCloud resize form #919

Merged
merged 1 commit into from
Apr 18, 2017

Conversation

mansam
Copy link
Contributor

@mansam mansam commented Apr 4, 2017

Angularizing this form is generally helpful, and it also resolves https://bugzilla.redhat.com/show_bug.cgi?id=1437587 by fixing a bug with the select box when only one item is available to be selected.

@miq-bot miq-bot added the wip label Apr 4, 2017
@himdel
Copy link
Contributor

himdel commented Apr 4, 2017

@mansam Since this is a new form, please use the newer controllerAs syntax :).

(There's already some examples in our code, possible issues are that you will need to add "miq-form" => true and model and model-copy attributes on the %form element ("model" => "vm.vmCloudModel", "model-copy" => "vm.modelCopy"), and that you may need to keep cancelClicked and submitClicked on $scope, depending on the buttons partial you pick.)

Also, the submit and cancel buttons should come from a shared partial .. app/views/layouts/angular/_generic_form_buttons.html.haml is the one you can use without leaving those methods on $scope

$scope.submitClicked = function() {
miqService.sparkleOn();
var url = '/vm_cloud/resize_vm/' + vmCloudResizeFormId + '?button=submit';
miqService.miqAjaxButton(url, true);
Copy link
Contributor

@himdel himdel Apr 4, 2017

Choose a reason for hiding this comment

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

Please submit the angular model instead of relying on implicit form data serialization .. this should be miqService.miqAjaxButton(url, vm.vmCloudModel)

var data = response.data;

$scope.flavors = data.flavors;
$scope.modelCopy = angular.copy( $scope.vmCloudModel );
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't make sense since you don't touch vmCloudModel when handling the reponse.

%div_for_paging{'ng-controller' => "pagingDivButtonGroupController",
'paging_div_buttons_id' => "angular_paging_div_buttons",
'paging_div_buttons_type' => "Submit"}

- unless @explorer
Copy link
Contributor

@himdel himdel Apr 4, 2017

Choose a reason for hiding this comment

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

Probably can't use this anymore (edit: never mind :D)

@mansam mansam force-pushed the port-vm-resize-form-to-angular branch 4 times, most recently from c0e778a to 91225ef Compare April 6, 2017 17:04
@mansam mansam force-pushed the port-vm-resize-form-to-angular branch from 91225ef to a2372d7 Compare April 6, 2017 17:39
@mansam mansam changed the title [wip] Angularize the VmCloud resize form Angularize the VmCloud resize form Apr 6, 2017
@miq-bot
Copy link
Member

miq-bot commented Apr 6, 2017

Checked commit mansam@a2372d7 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 0 offenses detected
Everything looks good. ⭐

@miq-bot miq-bot removed the wip label Apr 6, 2017
@himdel
Copy link
Contributor

himdel commented Apr 18, 2017

LGTM, works in the UI.. 👍

@himdel himdel merged commit 7047f19 into ManageIQ:master Apr 18, 2017
@himdel himdel added this to the Sprint 59 Ending Apr 24, 2017 milestone Apr 18, 2017
@himdel
Copy link
Contributor

himdel commented Apr 18, 2017

@mansam Should this go in fine?

Also, out of curiosity, how did you fix the buttons not refreshing issue?

@mansam
Copy link
Contributor Author

mansam commented Apr 18, 2017

@himdel I needed to add the view to the list here: https://github.com/ManageIQ/manageiq-ui-classic/pull/919/files#diff-5910cce3bec4cd624ba288de86215b4cL1339

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants