Refactor current basic form #1212

andresmgot · 2019-10-09T15:35:09Z

This PR contains several improvements:

The hardcoded keys that Kubeapps expects has been reduced to the minimum possible (for the moment) and moved to the file schema.ts so any component can read those.
Instead of hardcoding titles, we will be using the ones used in the schema for all the parameters.
The component DatabaseSection has been refactored to Subsection to make it generic. The idea is to reuse this component for the hostname section.
The component DiskSize has been refactored to SliderParam to make it generic. The idea is to reuse this component for the resource params.

andresmgot · 2019-10-09T15:37:15Z

dashboard/src/components/DeploymentForm/BasicDeploymentForm/Subsection.tsx

@@ -0,0 +1,85 @@
+import * as React from "react";


Sorry, I don't know why this is detected as a new file. The changes here from the DatabaseSection are:

andresmgot · 2019-10-09T15:37:39Z

dashboard/src/components/DeploymentForm/BasicDeploymentForm/Subsection.tsx

+  enablerChildrenParam: string;
+  enablerCondition: boolean;


These two parameters are used to know which of the children params is used to enable/disable the section.

Why do you need enablerCondition? Ah, because some charts might use enablePersistence while others disablePersistence?

Exactly. A real example is the externalDatabase that to be enabled require the property mariadb.enabled to be false.

andresmgot · 2019-10-09T15:38:06Z

dashboard/src/components/DeploymentForm/BasicDeploymentForm/Subsection.tsx

+    const { label, param, name, enablerChildrenParam, enablerCondition } = this.props;
+    return (
+      <div className="subsection margin-v-normal">
+        {param.children && param.children[enablerChildrenParam] && (


only show the enabler section if that param exists

andresmgot · 2019-10-09T15:38:45Z

dashboard/src/components/DeploymentForm/BasicDeploymentForm/Subsection.tsx

+    };
+  };
+
+  private renderParam(name: string, param: IBasicFormParam, index: number) {


Generically render sub params as in the BasicDeploymentForm component.

Can one implementation be shared for both cases then? (Assuming a subsection will generally be able to support all the same fields - though we don't need to support further subsections etc.)

yes, I thought about that but I didn't know where to place the function. Probably just the one in the BasicDeploymentForm should be enough.

andresmgot · 2019-10-09T16:20:24Z

dashboard/src/components/DeploymentForm/BasicDeploymentForm/BasicDeploymentForm.tsx

              />
            );
-          default:


I have removed this case to assume that if the type is not set, it refers to a string

absoludity

+1 - a bunch of comments, but fine to land.

So we're still enforcing a certain structure on the schema now right? Or did I miss that here? (The reason I worry about enforcing a certain structure is that it might make people less likely to use it. If I have a chart with values where the db values are just at the top level, I'd have to break my chart compatability to use this - I think?)

absoludity · 2019-10-09T23:45:14Z

dashboard/src/components/DeploymentForm/BasicDeploymentForm/SliderParam.tsx

-export interface IDiskSizeParamState {
-  Gi: number;
+export interface ISliderParamState {
+  amount: number;


value is a bit more generic, but amount fine too.

absoludity · 2019-10-09T23:45:58Z

dashboard/src/components/DeploymentForm/BasicDeploymentForm/SliderParam.tsx

-    Gi: toNumber(this.props.param.value) || 10,
+class SliderParam extends React.Component<ISliderParamProps, ISliderParamState> {
+  public state: ISliderParamState = {
+    amount: toNumber(this.props.param.value) || this.props.min,


Doesn't this mean it can be set initially outside min/max?

yes, the default value comes from the values.yaml or the schema. If there is a problem with the configuration, the default value may not be valid

absoludity · 2019-10-09T23:48:21Z

dashboard/src/components/DeploymentForm/BasicDeploymentForm/Subsection.test.tsx

  expect(defaultProps.param.children!.useSelfHostedDatabase.value).toBe(true);
-  expect(wrapper.find(".margin-t-normal").prop("hidden")).toBe(true);
+  expect(wrapper.find("div").findWhere(d => d.prop("hidden"))).toExist();


This change is because there are more than one div ? So it's "expect at least one div with a hidden prop". Not sure if that should be more specific to match the test label.

Also - why are you creating a div with a hidden prop, rather than a className or similar? Also, does this return true for <div hidden={false}> as the prop is still passed (which is what the code below does, I think?)

This change is because there are more than one div ? So it's "expect at least one div with a hidden prop". Not sure if that should be more specific to match the test label.

Yes. Taking a look to the functional code, I couldn't find something better to identify the div I am interested in.

Also - why are you creating a div with a hidden prop, rather than a className or similar?

Well, I am interested in hiding the div, why are you suggesting using a className instead? Is there something I am missing?

Also, does this return true for
as the prop is still passed (which is what the code below does, I think?)

No, since the property of hidden is false, the find iteration return false so it's not returned. It only returns true for <div hidden={true}>.

Well, I am interested in hiding the div, why are you suggesting using a className instead? Is there something I am missing?

No - I was just unsure whether the hidden attribute should be used in this case. See the MDN doc, I was thinking that what we're doing here is equivalent to hiding panels in a tabbed dialog (where it should not be used), but the use-case here is different in that the elements are only relevant if the self-hosted option is enabled (across all presentations) so I think you're right to use it.

absoludity · 2019-10-09T23:54:53Z

dashboard/src/components/DeploymentForm/BasicDeploymentForm/Subsection.tsx

+  enablerChildrenParam: string;
+  enablerCondition: boolean;


Why do you need enablerCondition? Ah, because some charts might use enablePersistence while others disablePersistence?

absoludity · 2019-10-10T00:00:24Z

dashboard/src/components/DeploymentForm/BasicDeploymentForm/Subsection.tsx

+    };
+  };
+
+  private renderParam(name: string, param: IBasicFormParam, index: number) {


Can one implementation be shared for both cases then? (Assuming a subsection will generally be able to support all the same fields - though we don't need to support further subsections etc.)

absoludity · 2019-10-10T00:04:04Z

dashboard/src/shared/schema.ts

@@ -90,7 +92,8 @@ export function setValue(values: string, path: string, newValue: any) {
 export function getValue(values: string, path: string, defaultValue?: any) {
  const doc = YAML.parseDocument(values);
  const splittedPath = path.split(".");
-  return (doc as any).getIn(splittedPath) || defaultValue;
+  const value = (doc as any).getIn(splittedPath);
+  return value === undefined || value === null ? defaultValue : value;


Not really important, but should the default also be limited to min/max? Completely ignorable :)

the default is set by the user (the same way they set the min/max) so that shouldn't be a case (and their tests should catch that)

andresmgot · 2019-10-10T09:06:06Z

So we're still enforcing a certain structure on the schema now right? Or did I miss that here? (The reason I worry about enforcing a certain structure is that it might make people less likely to use it. If I have a chart with values where the db values are just at the top level, I'd have to break my chart compatability to use this - I think?)

Yes (that't not changed in this PR), but the logic is a bit different. If you want to have a subsection, you need to follow a certain structure. You still can set the db values at the top level but you won't see them grouped in a section, just as plain arguments. I think it makes sense to have this kind of conversion from structured objects to subsections (that's what automatic parsers do for JSON schemas).

Apart from that, we have the hardcoded change in which we are moving mariadb.enabled to externalDatabase to show it in the subsection. I am not very happy with that but better that than changing the values.yaml for the moment.

andresmgot · 2019-10-10T10:30:56Z

Merging this to send another PR. If there is something unresolved please let me know!

absoludity · 2019-10-10T23:15:50Z

Merging this to send another PR. If there is something unresolved please let me know!

Yep, I often +1 with some comments, meaning that you should feel free to land it once you've looked at the comments and made a decision either way. Thanks!

Andres Martinez Gotor added 2 commits October 9, 2019 17:31

Refactor current basic form

b048daa

Fix tests

d1fadc8

andresmgot commented Oct 9, 2019

View reviewed changes

Add more unit tests

904fe21

absoludity approved these changes Oct 10, 2019

View reviewed changes

Apply feedback

b6d2ec6

Fix/disable test

83d737a

andresmgot merged commit 9a3e8b4 into master Oct 10, 2019

absoludity deleted the basicFormRefactor branch October 10, 2019 23:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor current basic form #1212

Refactor current basic form #1212

andresmgot commented Oct 9, 2019

andresmgot Oct 9, 2019

andresmgot Oct 9, 2019 •

edited

Loading

absoludity Oct 9, 2019

andresmgot Oct 10, 2019

andresmgot Oct 9, 2019

andresmgot Oct 9, 2019

absoludity Oct 10, 2019

andresmgot Oct 10, 2019

andresmgot Oct 9, 2019

absoludity left a comment

absoludity Oct 9, 2019

absoludity Oct 9, 2019

andresmgot Oct 10, 2019

absoludity Oct 9, 2019

andresmgot Oct 10, 2019

absoludity Oct 10, 2019

absoludity Oct 9, 2019

absoludity Oct 10, 2019

absoludity Oct 10, 2019

andresmgot Oct 10, 2019

andresmgot commented Oct 10, 2019 •

edited

Loading

andresmgot commented Oct 10, 2019

absoludity commented Oct 10, 2019

Refactor current basic form #1212

Refactor current basic form #1212

Conversation

andresmgot commented Oct 9, 2019

Choose a reason for hiding this comment

andresmgot Oct 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

absoludity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresmgot commented Oct 10, 2019 • edited Loading

andresmgot commented Oct 10, 2019

absoludity commented Oct 10, 2019

andresmgot Oct 9, 2019 •

edited

Loading

andresmgot commented Oct 10, 2019 •

edited

Loading