Adds a toggle between one-off and recurring runs to NewRun page #1274
Conversation
frontend/src/pages/NewRun.test.tsx
Outdated
| @@ -33,6 +33,10 @@ class TestNewRun extends NewRun { | |||
| public async _pipelineSelectorClosed(confirmed: boolean): Promise<void> { | |||
| return await super._pipelineSelectorClosed(confirmed); | |||
| } | |||
|
|
|||
| public _updateRecurringRunState(isRecurringRun: boolean): void { | |||
There was a problem hiding this comment.
Wondering if you can just do something like "public x = super.x;" rather than a function wrapper.
| {pipeline && Array.isArray(pipeline.parameters) && !!pipeline.parameters.length && ( | ||
| <div> | ||
| {pipeline.parameters.map((param, i) => | ||
| <TextField id={`newRunPipelineParam${i}`} key={i} variant='outlined' | ||
| label={param.name} value={param.value || ''} | ||
| onChange={(ev) => this._handleParamChange(i, ev.target.value || '')} | ||
| style={{ height: 40, maxWidth: 600 }} className={commonCss.textField} />)} | ||
| style={{ maxWidth: 600 }} className={commonCss.textField}/>)} |
There was a problem hiding this comment.
Why remove height? What happens when you have long parameter names?
There was a problem hiding this comment.
Height here is a bit of a misnomer here. I've actually go a follow-up that uses InputProps to change what you'd expect height to change. This change is essentially a noop in this case. Extremely long parameter names go off the end of the text box, but I don't think this is something we'll have to worry about
frontend/src/pages/NewRun.tsx
Outdated
| @@ -416,6 +442,14 @@ class NewRun extends Page<{}, NewRunState> { | |||
| }, () => this._validate()); | |||
| } | |||
|
|
|||
| protected _updateRecurringRunState(isRecurringRun: boolean): void { | |||
| this.props.updateToolbar({ | |||
| actions: this.props.toolbarProps.actions, | |||
There was a problem hiding this comment.
IIRC this line is optional?
There was a problem hiding this comment.
Yes, this isn't needed
| @@ -535,10 +569,9 @@ class NewRun extends Page<{}, NewRunState> { | |||
| } | |||
|
|
|||
| private _start(): void { | |||
| const { pipelineFromRun, pipeline, usePipelineFromRun } = this.state; | |||
There was a problem hiding this comment.
Consistency mostly. We were pulling a few variables out of state like this, and not doing so for others within this same function, and it didn't seem like we were getting much for doing so
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yebrahim 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 |
1 similar comment
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yebrahim 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 |
…flow#1274) * Allows toggling between one-off and recurring runs in the new run page * Clean up and adds tests * Fix integration test - account for extra field in form * Cleanup and PR comments
Changes are mainly around lines 272 and 445 of
NewRun.tsx.Other changes, are primarily cleanup and tests.
Fixes #1217
Examples of the toggle:
One-off:
Recurring:
This change is