-
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
use two-step form for provisioning service instances #373
use two-step form for provisioning service instances #373
Conversation
A two step form allows us to cleanly separate input that Kubeapps needs from the generalised input from the Service Broker's schema. The first form simply asks for the name of the Service Instance, and the second form renders the parameters using the broker-provided instanceCreateParameterSchema.
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.
lgtm
@@ -116,13 +120,23 @@ class ProvisionButton extends React.Component<IProvisionButtonProps, IProvisionB | |||
}); | |||
}; | |||
|
|||
public handleBackButton = (e: React.MouseEvent<HTMLButtonElement>) => { | |||
e.preventDefault(); | |||
this.setState({ ...this.state, displayNameForm: true }); |
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.
do you need the ...this.state
?
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.
oops no, thanks for catching that, I was debugging something and left that in!
{displayLabel && ( | ||
<label htmlFor={id}> | ||
{label} | ||
{required && " *"} |
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.
is quite obvious but a line at the end saying * Mandatory fields
would be nice
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.
What about changing it to (required)
? I want to avoid adding an extra thing at the end of the form, the markup is already fairly messy.
If you're worried about users simply missing the required fields, the required
html attribute is set so browsers should catch the error before forms are submitted.
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.
👍 that's okay
@prydonius Can we make the form that just asks for the name smaller, please? |
the changes lgtm |
A two step form allows us to cleanly separate input that Kubeapps needs
from the generalised input from the Service Broker's schema. The first
form simply asks for the name of the Service Instance, and the second
form renders the parameters using the broker-provided instanceCreateParameterSchema.
fixes #362