-
Notifications
You must be signed in to change notification settings - Fork 705
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 slider component for adding the disk size #1206
Conversation
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.
Great, thanks!
]); | ||
}); | ||
|
||
it("changes only the value of the state when the slider is being updated", () => { |
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.
This confused me until I read the actual test details... but I can't see an obviously better test title. Perhaps: "updates state but does not change param value during slider update" .
I think was confused me was "changes only the value of the state", because it seemed odd that we'd be updating the component state but not updating handling the form param change. After reading below, I see it's because the component encapsulates both the slider and the text input - we want to update both state and handle the change for the input, but only trigger the change for the slider when it is "dropped", which makes sense.
expect(wrapper.state("Gi")).toBe(10); | ||
|
||
const input = wrapper.find("input#disk"); | ||
const event = { currentTarget: { value: "foo20*#@$" } } as React.FormEvent<HTMLInputElement>; |
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.
Hmm... might be some corner cases... see below.
|
||
function toNumber(value: string) { | ||
// Force to return a Number from a string removing any character that is not a digit | ||
return Number(value.replace(/[^\d]/g, "")); |
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.
So if I enter "125.5"? Currently will this set the state to 1255 Gi?
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.
True, I thought that decimals were not allowed (but they are according to https://github.com/kubernetes/community/blob/master/contributors/design-proposals/scheduling/resources.md#resource-quantities) so I will add a handle for that.
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.
Caveat: Since the minimal step in the slide is 1 unit, copying 125.5
will result in 126
since the slider round it to the closest int.
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.
Sorry I didn't check it before! The code LGTM :)
Ref #1182
This PR adds a slider to select the disk size in the simplified form.
It's possible to modify it either dragging the slider or adding a custom number in the input field close to it.
Note that apart than the component
react-compound-slider
it's necessary to add some boilerplate code to make the slider work so I have created a wrapper of that component with the code avaliable here to expose a simplifiedSlider
component. That boilerplate code is not covered by unit tests and doesn't require a thoughtful review.