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

feat(provider/kubernetes): v2 resize modal #4274

Closed
wants to merge 1 commit into from

Conversation

lwander
Copy link
Member

@lwander lwander commented Oct 16, 2017

resize

@@ -18,6 +19,7 @@ export interface IServerGroup {
asg?: IAsg;
buildInfo?: any;
category?: string;
capacity?: ICapacity;
Copy link
Member Author

Choose a reason for hiding this comment

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

Seemed OK to add since it's in clouddriver's server group model.

Copy link
Contributor

@danielpeach danielpeach left a comment

Choose a reason for hiding this comment

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

Nice.

title: `Resizing ${this.serverGroup.name}`,
application: application,
modalInstance: $uibModalInstance,
} as any);
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for the any cast? I think you gave $uibModalInstance the wrong type (which might by why) - should be IModalServiceInstance instead of IModalInstanceService.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh that would explain it - I thought typescript was mishandling the optional properties.

this.current = this.serverGroup.capacity;
this.command = {
capacity: copy(this.current),
} as IResizeCommand;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can initialize reason as null rather than cast like this.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

class="form-control input-sm highlight-pristine"
ng-model="ctrl.current.desired"
readonly/>
<span class="input-group-addon">pod<span ng-if="ctrl.current.desired != 1">s</span></span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing is going to go wrong here, since the input type is number, but should be a !== here and below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah good catch

</div>
<div class="modal-footer">
<user-verification account="ctrl.serverGroup.account" verification="ctrl.verification"></user-verification>
<button type="submit" ng-click="ctrl.resize()" style="display:none"></button> <!-- Allows form submission via enter keypress-->
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is everywhere...but can't you put ng-submit="ctrl.resize()" as a form attribute? Maybe I'm wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure thing - why do you prefer ng-submit?

Copy link
Contributor

Choose a reason for hiding this comment

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

The hidden button feels a little hacky. Or not. Who knows.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure why, but the ng-submit form attribute causes the modal to close (but still submit the task) when you hit enter. With the hacky button you at get to see the task monitor do its thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh. That's probably why it's being used. Now I know.

@lwander
Copy link
Member Author

lwander commented Oct 17, 2017

I can't seem to get github to update this PR. the branch in my lwander/deck repo is up-to-date... closing & reopening.

@lwander lwander closed this Oct 17, 2017
@lwander
Copy link
Member Author

lwander commented Oct 17, 2017

#4279

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

Successfully merging this pull request may close these issues.

2 participants