-
Notifications
You must be signed in to change notification settings - Fork 188
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
Add option to also bounce when scaling #795
Conversation
return unless data | ||
bounce = $('.vex #bounce').is ':checked' | ||
incremental = $('.vex #incremental-bounce').is ':checked' | ||
@scale(data, bounce, incremental).done => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how come you're passing bounce
and incremental
to @scale()
?
@tpetr updated form your comments, also the checkbox label just says "Bounce after scaling" now |
Added a field in the request so that you can default that box to checked if you want |
@@ -229,6 +232,10 @@ public ScheduleType getScheduleTypeSafe() { | |||
return readOnlyGroups; | |||
} | |||
|
|||
public boolean isBounceAfterScale() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need one that is the Json getter that just returns the actual value.
If you want to add a helper, you should @JsonIgnore it.
Is the request option only used by the UI? |
Yes, only used by the ui. Developers wanted a way to have the checkbox default to checked for some requests but not others |
This could be addressed in a subsequent PR, but I think it'd be worth packing the bounce + scale functionality into the backend. The "dumber" the UI is, the better. |
<label for="bounce" id="bounce-label"> | ||
<input type="checkbox" id="bounce"{{#if bounceAfterScale}} checked{{/if}}> | ||
Bounce after scaling | ||
</label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd consider moving the bounce checkbox to after the scale input to fit in with the "bounce after scaling" wording.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bonus points for indenting the incremental bounce option + greying it out if the bounce checkmark is unchecked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right now the incremental option isn't show if bounce checkmark is unchecked and appears when you check it. would you rather see it greyed out instead of having it hidden?
Add option to also bounce when scaling
(extra options for incremental bounce only appear when bounce is checked)