feat: Group immutable and mutable rich parameters #5975
Conversation
There was a problem hiding this comment.
This is a nice change! I think splitting the array into two (mut/immut) would clean up the code a bunch but feel free to think of it as an optional change.
I can't quite put my finger on it, but titles feel a bit verbose. Perhaps I'd prefer a heading with "Workspace parameters" and subheadings "Mutable" and "Immutable". But don't know if that would look better tbh.
| @@ -150,7 +151,9 @@ export const WorkspaceBuildParametersPageView: FC< | |||
|
|
|||
| <Maybe | |||
| condition={Boolean( | |||
| props.templateParameters && props.templateParameters.length === 0, | |||
| !props.isLoading && | |||
| props.templateParameters && | |||
There was a problem hiding this comment.
Usage could be nicer here if we defaulted to an empty array for the prop.
There was a problem hiding this comment.
Before introducing isLoading it was not possible, but I can retake a look.
There was a problem hiding this comment.
Usually, it is not a good idea to use an empty array in the FE because we usually use undefined to identify the "loading data" state. However, if this data is provided internally and not from an external source that depends on an effect like network calls, file reading, etc. Providing an empty array is ok 👍
There was a problem hiding this comment.
Also, we could introduce isLoading and set it as an empty array but I don't see what is the value on it if compared to just letting the value as undefined.
| @@ -104,4 +105,18 @@ const getDefaultParameterValues = ( | |||
| return paramValues | |||
| } | |||
|
|
|||
| export const orderedTemplateParameters = ( | |||
There was a problem hiding this comment.
Is it used in more than one place? If not, I think we can remove the export or move this function to live in the file where it is used.
There was a problem hiding this comment.
It is used in 2 places as it's also in WorkspaceBuildParametersPage, so I will keep it as is, but thanks for raising this!
Related: #5574
This PR introduces a grouping of immutable and mutable rich parameters in Create.... and Update... forms.