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

Make the slider param customizable via the schema #1223

Merged
merged 2 commits into from
Oct 16, 2019

Conversation

andresmgot
Copy link
Contributor

Ref: #1182

In order to avoid dependencies with hardcoded references to key names in the logic, I have set some new properties that an element can define in order to be rendered as a slider (instead of a string). For example, the disk size can be represented as:

        "size": { 
          "type": "string",
          "title": "Disk Size",
          "form": "diskSize",
          "render": "slider",
          "sliderMin": 1,
          "sliderMax": 100,
          "sliderUnit": "Gi"
        }

@@ -145,6 +100,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.

Comment on lines +387 to +388
sliderMin?: number;
sliderMax?: number;
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.

render?: string;
sliderMin?: number;
sliderMax?: number;
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.

@andresmgot andresmgot merged commit 9718f6d into vmware-tanzu:master Oct 16, 2019
@acim
Copy link

acim commented May 18, 2020

It would be nice to be able define a step here as well so that one may select 1, 3 or 5 instances of some lead election service.

@andresmgot
Copy link
Contributor Author

agree, can you open a separate issue @acim so we don't forget?

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.

3 participants