ISW - Add Start Input Step #21163
ISW - Add Start Input Step #21163
Conversation
|
|
||
| useEffect(() => { | ||
| setStepsConfig(initialStepsConfig); | ||
| setWizardData({ ...wizardData, input }); // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
Do we have to omit the dependencies here? Because it's usually not recommended, it breaks React mental model, it's hard to maintain and might cause bugs, I think we should at least explicitly document why they were omitted.
There was a problem hiding this comment.
Yes, as this should really just happen on initial load. Otherwise, it will end up in an endless loop as it is updating a context used by a parent. I can add a comment though 👍
| const withInitialStepsData = updateStepConfigOrData(stepsData, currentStepName, defaultStepData); | ||
|
|
||
| setStepsData(withInitialStepsData); | ||
| } | ||
| } // eslint-disable-next-line react-hooks/exhaustive-deps |
| @@ -67,28 +67,59 @@ const ConntectedPipelinesList = styled.ul` | |||
| padding-left: 20px; | |||
There was a problem hiding this comment.
We can use theme spacing here.
There was a problem hiding this comment.
Generally I agree but in this case I prefer a fixed spacing as it is influencing the space around the bullet point of a <li> (it is also kind of reverting the removal of this spacing)
| stopInput(); | ||
|
|
||
| if (routingStepData.shouldCreateNewPipeline) { | ||
| if (createdPipelineId) { |
There was a problem hiding this comment.
here if we only have one nested condition we can just use 1 if (routingStepData.shouldCreateNewPipeline && createdPipelineId)
| const hasPreviousStep = checkHasPreviousStep(orderedSteps, activeStep); | ||
| const hasNextStep = checkHasNextStep(orderedSteps, activeStep); | ||
| const isNextStepDisabled = checkIsNextStepDisabled(orderedSteps, activeStep, stepsConfig); | ||
| const [startInputStatus, setStartInputStatus] = useState<'NOT_STARTED' | 'RUNNING' | 'SUCCESS' | 'FAILED' | 'ROLLED_BACK' | 'ROLLING_BACK'>('NOT_STARTED'); |
There was a problem hiding this comment.
Do we need to have a reusable specific type for InputStatus?
There was a problem hiding this comment.
For now I don't think it is needed, as the type itself is only used in this line.
As this state is being typed we don't really need an as const for the values.
Description
Closes #20567
Requires #21164 for the rollback
/nocl
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: