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

Add slider component for adding the disk size #1206

Merged
merged 3 commits into from Oct 8, 2019

Conversation

andresmgot
Copy link
Contributor

@andresmgot andresmgot commented Oct 4, 2019

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 simplified Slider component. That boilerplate code is not covered by unit tests and doesn't require a thoughtful review.

Screenshot from 2019-10-04 13-19-56

Copy link
Contributor

@absoludity absoludity left a 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", () => {
Copy link
Contributor

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>;
Copy link
Contributor

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, ""));
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@andresmgot andresmgot merged commit 32d70fd into vmware-tanzu:master Oct 8, 2019
Copy link
Member

@Angelmmiguel Angelmmiguel left a 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 :)

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.

None yet

3 participants