feat(frontend) Support LIST, STRUCT type in RuntimeConfig parameters #7991
Conversation
…pporting num, bool, str)
Add TODO Move inputConvert out from function component. Check parameter type by PipelineSpec before converting.
Delete unnecessary console.log
…upport-MultiType-Param
Assign invalid input to null.
2. Add tests to validate the changes.
2. Added unit test for LIST and STRUCT type parameters.
…upport-List-Struct-Param
…upport-List-Struct-Param Fixed conflicts.
|
Test for Screen.Recording.2022-07-06.at.5.34.04.PM.mov |
|
Test for Screen.Recording.2022-07-06.at.5.40.06.PM.mov |
| render(<NewRunParametersV2 {...props}></NewRunParametersV2>); | ||
|
|
||
| const listParam = screen.getByDisplayValue('[1,2,3]'); | ||
| fireEvent.change(listParam, { target: { value: '[4,5,6]' } }); |
There was a problem hiding this comment.
Can we check the screen now has [4,5,6]?
screen.getByDisplayValue('[4,5,6]');
There was a problem hiding this comment.
Same for all cases below.
Added and modified unit tests.
|
Test for double type input. The printOp process wait for (30 + inputNum) seconds Screen.Recording.2022-07-07.at.4.51.03.PM.mov |
| @@ -71,6 +71,11 @@ function convertInput(paramStr: string, paramType: ParameterType_ParameterTypeEn | |||
| return Number(paramStr); | |||
| } | |||
| return null; | |||
| case ParameterType_ParameterTypeEnum.NUMBER_DOUBLE: | |||
| if (!Number.isNaN(Number(paramStr))) { | |||
There was a problem hiding this comment.
NIT: Is it necessary to use Number()? Can we simplify it by using !Number.isNaN(paramStr)?
There was a problem hiding this comment.
Yes, I think so. The input of isNaN() should be number.
There was a problem hiding this comment.
How about !isNaN(paramStr)? If we can use the standard library to do conversion for us, it is preferred: https://www.w3schools.com/jsref/jsref_isnan.asp
|
/assign @zijianjoy |
| expect(handleParameterChangeSpy).toHaveBeenCalledTimes(1); | ||
| expect(handleParameterChangeSpy).toHaveBeenLastCalledWith({ | ||
| doubleParam: 4.56, | ||
| }); |
There was a problem hiding this comment.
For new tests: Please also check for the existence of display value 4.56. Just like we mentioned in https://github.com/kubeflow/pipelines/pull/7991/files#r916159217
|
/lgtm Great work implementing LIST and STRUCT type parameter inputs! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: zijianjoy The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test kubeflow-pipeline-frontend-test |
convertInputfunction is able to support LIST and STRUCT type user input parameter. Also, the default values from IR can be displayed correctly.