AppNew: allow users to select a version in the appNew component #185
Conversation
There was a problem hiding this comment.
I think rather than having an internal state of what chart version will be deployed, we should just reuse the chartVersion from the props. Selecting a chartVersion in this form should update the URL with the selected version and componentWillReceiveProps can be used to call getChartVersion and getChartValues. This is similar to how the ChartView works when selecting a version.
I was of the view the version field in the component should be reflected in the component state. |
IMO it's weird that the props may have a different chartVersion than the state. Also, it means the URL will not always be correct. The nice thing about using the URL to determine the selected chart version is that you can share the link with someone else to install the same chart and version. |
sameersbn
force-pushed
the
appnew-select-version
branch
from
7ef691a to
94834e2
Compare
| } | ||
| } else { | ||
| const { version, values } = nextProps.selected; | ||
| if (version && values && !this.state.valuesModified) { |
There was a problem hiding this comment.
We could make this else an else if and move this condition up, we can destructure version and values after line 69. That might be easier to read/follow.
| @@ -200,6 +243,18 @@ class AppNew extends React.Component<IAppNewProps, IAppNewState> { | |||
| public handleReleaseNameChange = (e: React.FormEvent<HTMLInputElement>) => { | |||
| this.setState({ releaseName: e.currentTarget.value }); | |||
| }; | |||
| public handleChartVersionChange = (e: React.FormEvent<HTMLSelectElement>) => { | |||
| const { versions } = this.props.selected; | |||
| const cv = versions.find(v => v.attributes.version === e.currentTarget.value); | |||
There was a problem hiding this comment.
I think this could be made simpler, looks like a lot of work to just get the repoName and chartName. We should be able to from the URL from this.props.chartID and e.currentTarget.value alone.
| <option | ||
| key={v.id} | ||
| value={v.attributes.version} | ||
| selected={v.attributes.version === this.props.chartVersion} |
There was a problem hiding this comment.
warning from js console: Warning: Use the `defaultValue` or `value` props on <select> instead of setting `selected` on <option>.
So we need to change line 154 to add <select value={this.props.chartVersion}>.
| @@ -111,6 +135,11 @@ class AppNew extends React.Component<IAppNewProps, IAppNewState> { | |||
| <form className="container" onSubmit={this.handleDeploy}> | |||
There was a problem hiding this comment.
not really related to this PR, but while your on this view can we add the margin-b-bigger class to the form, right now there is no spacing underneath the form.
There was a problem hiding this comment.
fixed! i assumed this was specific to firefox 😂
|
thanks for the review @prydonius. |
render should only proceed if a version is currently selected, the appValues have been fetched and the versions array is populated.
sameersbn
force-pushed
the
appnew-select-version
branch
from
a757482 to
d75c889
Compare
|
Before we commit this change, there is one other thing i want to check with you. |
|
@sameersbn I noticed it, and actually checked to see what would happen specifically in that case. IMO it makes sense to persist the user's values, but we could maybe add a reset button in the future if people want to use the chart's values. |
@sameersbn please, add a description here that explains a bit how you have implemented this