Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds Launch Plan selector #3

Merged
merged 5 commits into from
Sep 3, 2019
Merged

Adds Launch Plan selector #3

merged 5 commits into from
Sep 3, 2019

Conversation

schottra
Copy link
Contributor

@schottra schottra commented Sep 3, 2019

  • Made WorkflowSelector a generic component, and made the fetchResults function optional. The default implementation will just search the values of the provided options without issuing any network requests
  • Refactored the form state to make launch plans behave similarly to the workflow, instead of the weird mixture of selecting a launch plan or using a default one.
    • There is now an effect that fires once the launch plan loads which will attempt to select the default launch plan (the one with the same name as the workflow) if it exists.
  • Used the new generic selector to allow selection of the launch plan
  • Updated the logic for showing the inputs to wait until a launch plan is selected
    LaunchPlanSelector

@schottra schottra requested a review from mrmcduff September 3, 2019 21:32
@@ -107,26 +108,42 @@ export const LaunchWorkflowForm: React.FC<LaunchWorkflowFormProps> = props => {
{...state.workflowOptionsLoadingState}
>
<div className={styles.formControl}>
<WorkflowSelector
<SearchableSelector
label="Workflow Version"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A const somewhere? ¯\_(ツ)_/¯

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's only used in this file, in this single place. So I'm inclined not to make it a constant. If/when we get around to i18n support, my position on that will definitely change :-)

launchPlan && workflow.hasLoaded
? getInputs(workflow.value, launchPlan)
selectedLaunchPlan && workflow.hasLoaded
? getInputs(workflow.value, selectedLaunchPlan.data)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can selectedLaunchPlan.data also change, and should you trigger the useEffect on that too? Just checking.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. So technically, according to the Rules of Hooks, using that value means I should include it in the dependency array.

Practically, the data in the selector options is a const value that does not change unless you regenerate all of the items. In that case, the outer object would also change.

But I plan on enabling the extra lint rule in the future to ensure all captured values used in the effect are listed in the dependencies array. So I'll update it here.

Good catch!

@schottra
Copy link
Contributor Author

schottra commented Sep 3, 2019

I'm going to merge this and make the above-mentioned change in my follow-up branch.

@schottra schottra merged commit 03c6059 into launch Sep 3, 2019
@schottra schottra deleted the launch-dev branch September 4, 2019 18:21
@schottra schottra restored the launch-dev branch September 4, 2019 20:55
@schottra schottra deleted the launch-dev branch December 12, 2019 17:16
anrusina added a commit that referenced this pull request Apr 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants