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.

Sign up for GitHub

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

Make the slider param customizable via the schema #1223

Merged
merged 2 commits into from
Oct 16, 2019
Merged

Make the slider param customizable via the schema #1223

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
37bd529
Make the slider param customizable via the schema
Oct 15, 2019
5c18441
Merge master
Oct 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion dashboard/src/components/DeploymentFormBody/BasicDeploymentForm/BasicDeploymentForm.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,12 @@ const defaultProps = {
{
description: "renders a basic deployment with a disk size",
params: {
diskSize: { path: "size", value: "10Gi", type: "string" } as IBasicFormParam,
diskSize: {
path: "size",
value: "10Gi",
type: "string",
render: "slider",
} as IBasicFormParam,
},
},
{
Expand Down
62 changes: 17 additions & 45 deletions dashboard/src/components/DeploymentFormBody/BasicDeploymentForm/BasicDeploymentForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,9 @@ import { IBasicFormParam } from "shared/types";
import TextParam from "./TextParam";

import {
CPU_REQUEST,
DISK_SIZE,
ENABLE_INGRESS,
EXTERNAL_DB,
INGRESS,
MEMORY_REQUEST,
RESOURCES,
USE_SELF_HOSTED_DB,
} from "../../../shared/schema";
Expand Down Expand Up @@ -73,20 +70,6 @@ class BasicDeploymentForm extends React.Component<IBasicDeploymentFormProps> {
enablerCondition={false}
/>
);
case DISK_SIZE:
return (
<SliderParam
label={param.title || "Disk Size"}
handleBasicFormParamChange={handleBasicFormParamChange}
key={id}
id={id}
name={name}
param={param}
min={1}
max={100}
unit="Gi"
/>
);
case RESOURCES:
return (
<Subsection
Expand All @@ -99,34 +82,6 @@ class BasicDeploymentForm extends React.Component<IBasicDeploymentFormProps> {
param={param}
/>
);
case MEMORY_REQUEST:
return (
<SliderParam
label={param.title || "Memory Request"}
handleBasicFormParamChange={handleBasicFormParamChange}
key={id}
id={id}
name={name}
param={param}
min={10}
max={2048}
unit="Mi"
/>
);
case CPU_REQUEST:
return (
<SliderParam
label={param.title || "CPU Request"}
handleBasicFormParamChange={handleBasicFormParamChange}
key={id}
id={id}
name={name}
param={param}
min={10}
max={2000}
unit="m"
/>
);
case INGRESS:
return (
<Subsection
Expand Down Expand Up @@ -154,6 +109,23 @@ class BasicDeploymentForm extends React.Component<IBasicDeploymentFormProps> {
param={param}
/>
);
case "string": {
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is a string in the schema because it includes the units right? (ie. "10Gi") And it is parsed by helm as a string with units?

Makes this a bit awkward but I don't see a better option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly, that's why it's a string. You can actually go to the Advance tab and write 10000Mi if you want.

if (param.render === "slider") {
return (
<SliderParam
label={param.title || name}
handleBasicFormParamChange={handleBasicFormParamChange}
key={id}
id={id}
name={name}
param={param}
min={param.sliderMin || 1}
max={param.sliderMax || 1000}
unit={param.sliderUnit || ""}
/>
);
}
}
default:
return (
<TextParam
Expand Down
42 changes: 21 additions & 21 deletions ...ts/DeploymentFormBody/BasicDeploymentForm/__snapshots__/BasicDeploymentForm.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ exports[`renders a basic deployment with a disk size 1`] = `
Object {
"diskSize": Object {
"path": "size",
"render": "slider",
"type": "string",
"value": "10Gi",
},
Expand All @@ -25,18 +26,19 @@ exports[`renders a basic deployment with a disk size 1`] = `
handleBasicFormParamChange={[Function]}
id="diskSize-0"
key="diskSize-0"
label="Disk Size"
max={100}
label="diskSize"
max={1000}
min={1}
name="diskSize"
param={
Object {
"path": "size",
"render": "slider",
"type": "string",
"value": "10Gi",
}
}
unit="Gi"
unit=""
>
<div>
<label
Expand All @@ -51,7 +53,7 @@ exports[`renders a basic deployment with a disk size 1`] = `
<div
className="centered"
>
Disk Size
diskSize
</div>
</div>
<div
Expand All @@ -65,7 +67,7 @@ exports[`renders a basic deployment with a disk size 1`] = `
>
<Slider
default={10}
max={100}
max={1000}
min={1}
onChange={[Function]}
onUpdate={[Function]}
Expand All @@ -77,7 +79,7 @@ exports[`renders a basic deployment with a disk size 1`] = `
domain={
Array [
1,
100,
1000,
]
}
flatten={false}
Expand Down Expand Up @@ -123,7 +125,7 @@ exports[`renders a basic deployment with a disk size 1`] = `
Array [
Object {
"id": "$$-0",
"percent": 9.090909090909092,
"percent": 0.9009009009009009,
"value": 10,
},
]
Expand All @@ -133,7 +135,7 @@ exports[`renders a basic deployment with a disk size 1`] = `
LinearScale {
"domain": Array [
1,
100,
1000,
],
"interpolator": [Function],
"range": Array [
Expand Down Expand Up @@ -168,7 +170,7 @@ exports[`renders a basic deployment with a disk size 1`] = `
Array [
Object {
"id": "$$-0",
"percent": 9.090909090909092,
"percent": 0.9009009009009009,
"value": 10,
},
]
Expand All @@ -178,7 +180,7 @@ exports[`renders a basic deployment with a disk size 1`] = `
LinearScale {
"domain": Array [
1,
100,
1000,
],
"interpolator": [Function],
"range": Array [
Expand All @@ -195,21 +197,21 @@ exports[`renders a basic deployment with a disk size 1`] = `
domain={
Array [
1,
100,
1000,
]
}
getHandleProps={[Function]}
handle={
Object {
"id": "$$-0",
"percent": 9.090909090909092,
"percent": 0.9009009009009009,
"value": 10,
}
}
key="$$-0"
>
<div
aria-valuemax={100}
aria-valuemax={1000}
aria-valuemin={1}
aria-valuenow={10}
onKeyDown={[Function]}
Expand All @@ -223,7 +225,7 @@ exports[`renders a basic deployment with a disk size 1`] = `
"boxShadow": "1px 1px 1px 1px rgba(0, 0, 0, 0.2)",
"cursor": "pointer",
"height": 24,
"left": "9.090909090909092%",
"left": "0.9009009009009009%",
"marginLeft": "-11px",
"marginTop": "-6px",
"position": "absolute",
Expand All @@ -245,7 +247,7 @@ exports[`renders a basic deployment with a disk size 1`] = `
Array [
Object {
"id": "$$-0",
"percent": 9.090909090909092,
"percent": 0.9009009009009009,
"value": 10,
},
]
Expand All @@ -257,7 +259,7 @@ exports[`renders a basic deployment with a disk size 1`] = `
LinearScale {
"domain": Array [
1,
100,
1000,
],
"interpolator": [Function],
"range": Array [
Expand All @@ -283,7 +285,7 @@ exports[`renders a basic deployment with a disk size 1`] = `
target={
Object {
"id": "$$-0",
"percent": 9.090909090909092,
"percent": 0.9009009009009009,
"value": 10,
}
}
Expand All @@ -299,7 +301,7 @@ exports[`renders a basic deployment with a disk size 1`] = `
"height": 14,
"left": "0%",
"position": "absolute",
"width": "9.090909090909092%",
"width": "0.9009009009009009%",
"zIndex": 1,
}
}
Expand All @@ -322,9 +324,7 @@ exports[`renders a basic deployment with a disk size 1`] = `
/>
<span
className="margin-l-normal"
>
Gi
</span>
/>
</div>
</div>
</div>
Expand Down
4 changes: 4 additions & 0 deletions dashboard/src/components/DeploymentFormBody/DeploymentFormBody.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ describe("when there are changes in the selected version", () => {
.state as IDeploymentFormBodyState;
const basicFormParameters = {
foo: {
form: "foo",
path: "foo",
value: "bar",
type: "string",
Expand All @@ -130,6 +131,7 @@ describe("when there are changes in the selected version", () => {
});
const basicFormParameters = {
foo: {
form: "foo",
path: "foo",
value: "notBar",
type: "string",
Expand All @@ -154,6 +156,7 @@ describe("when there are changes in the selected version", () => {
});
const basicFormParameters = {
foo: {
form: "foo",
path: "foo",
value: "notBar",
type: "string",
Expand Down Expand Up @@ -188,6 +191,7 @@ describe("when there are changes in the selected version", () => {
});
const basicFormParameters = {
foo: {
form: "foo",
path: "foo",
value: "notBar",
type: "string",
Expand Down
10 changes: 2 additions & 8 deletions dashboard/src/shared/schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,6 @@ nullOptions.nullStr = "";
// Form keys that require pre-definition. This list should be kept as small as possible
export const EXTERNAL_DB = "externalDatabase";
export const USE_SELF_HOSTED_DB = "useSelfHostedDatabase";
export const DISK_SIZE = "diskSize";
export const MEMORY_REQUEST = "memoryRequest";
export const CPU_REQUEST = "cpuRequest";
export const RESOURCES = "resources";
export const INGRESS = "ingress";
export const ENABLE_INGRESS = "enableIngress";
Expand All @@ -35,19 +32,16 @@ export function retrieveBasicFormParams(
Object.keys(properties).map(propertyKey => {
// The param path is its parent path + the object key
const itemPath = `${parentPath || ""}${propertyKey}`;
const { type, title, description, form, minimum, maximum } = properties[propertyKey];
const { type, form } = properties[propertyKey];
// If the property has the key "form", it's a basic parameter
if (form) {
// Use the default value either from the JSON schema or the default values
const value = getValue(defaultValues, itemPath, properties[propertyKey].default);
const param: IBasicFormParam = {
...properties[propertyKey],
path: itemPath,
type: String(type),
value,
title,
description,
minimum,
maximum,
children:
properties[propertyKey].type === "object"
? retrieveBasicFormParams(defaultValues, properties[propertyKey], `${itemPath}.`)
Expand Down
4 changes: 4 additions & 0 deletions dashboard/src/shared/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,10 @@ export interface IBasicFormParam {
title?: string;
minimum?: number;
maximum?: number;
render?: string;
sliderMin?: number;
sliderMax?: number;
Comment on lines +387 to +388
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need these in addition to the already-present minimum and maximum?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that but the problem is that if a parameter contains minimum or maximum, it has precedence over the type. This means that it will always fail because it expect the field to be a integer instead of a string. Also the meaning is a bit different. This value is used to set the min and max values for the slider but you are actually allowed to write a bigger number if you want in the input box.

sliderUnit?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

So the awkwardness is that we have a generic param interface which is having fields added for specifics. Is it possible instead to compose a new IBasicFormNumberWithUnitsStringParam which includes IBasicFormParam as well as minimum, maximum, `units'?

My head isn't in this code deeply enough to see if that would avoid the awkwardness or make it more convoluted - so just see what you think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that would be more difficult to handle from the code point of view. In that case, the type of the parameters will be IBasicFormParam | IBasicFormNumberWithUnitsStringParam, which make it more difficult because it require type conversions and assumptions. IMO, it's easier to handle as optional parameters.

description?: string;
children?: { [name: string]: IBasicFormParam };
}