-
Notifications
You must be signed in to change notification settings - Fork 744
refactor(toolkit): add support for async showWhen and skipping prompter backward and forward direction #6166
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
refactor(toolkit): add support for async showWhen and skipping prompter backward and forward direction #6166
Conversation
53cda39 to
71ad18d
Compare
justinmk3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- implement a new NestedWizard abstract class that contain logic to instantiate and restore child Wizard used as prompter
- update wizard state controller to support
ControlSingal.Skipand add concept of wizard direction for tracking
Nice approach, and explanation! And clear test code.
| /** | ||
| * @param val Value immediately returned by the prompter. | ||
| */ | ||
| constructor(public readonly val: T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous implementation supports resolving the value val it was assigned in the beginning. but this is removed in the new solution. Is this intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Since we have SKIP signal. Resolving value was a workaround.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would there be a case we actually want it to resolve some default value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A straight forward case would be in bindPrompter, we have multiple if/else based on previous states, and maybe there are two case leads to skip prompter, but we want different value for this field even though it is skipped.
With previous solution we will be able to do as the follows
form.stuff.bindPrompter(
(f) => {
if (f===1) {
return skipPrompter(1)
}
if (f===2) {
return skipPrompter(2)
}
else{
return createQuickPick
}
},
{ setDefault: "skipped"} // set default won't be ideal in this case
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous workaround for skipping prompter was to automatically resolve the value and skip displaying UI to customer. However, that did not work for backward direction since the auto resolve always revert the backward flow to forward flow.
So we are splitting the skipping case from setting default value case. For how to use the SkipPrompter, please follow this example: aws-toolkit-vscode/packages/core/src/dev/activation.ts at master · vicheey/aws-toolkit-vscode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can consider resolving the value when forward and skip when backwards right?
Co-authored-by: Justin M. Keyes <[email protected]>
roger-zhangg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this super helpful feature, few questions
| /** | ||
| * Depending on current wizard direction, skip signal get converted to forward or backward control signal | ||
| */ | ||
| result.controlSignal = this.direction === DIRECTION.Forward ? undefined : ControlSignal.Back |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would this work on Wizard prompter? Will it able to identify the direction of it's parent wizard?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The WizardPrompter is still considered a prompter. It is not responsible for identifying parent wizard direction.
This state management functionality belongs to the stateController object within the Wizard class. Each Wizard is responsible for prompting, receiving, and processing answers from prompters (including WizardPrompter). Each answer contains data and a ControlSignal (undefined indicates no direction change). The Wizard class's stateController tracks the current direction and uses the ControlSignal from the prompter to manage direction changes.
How do we restore the state of WizardPrompter when a back button is clicked?
Well, we do not restore the state of the WizardPrompter object, we always instantiate a new instance of this class. We, however, restore the state of Wizard class used for instantiate the WizardPrompter. Again, the (parent) wizard class is responsible for this (reference)
| * An abstract class that extends the base Wizard class plus the ability to | ||
| * use other wizard classes as prompters | ||
| */ | ||
| export abstract class NestedWizard<T> extends Wizard<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This name seems confusing, unless I'm mistaken. This class is a Wizard that contains nested wizards, it's not itself nested. Right? So maybe a better name is Wizards (plural). That also improves discoverability because it sorts alphabetically next to Wizard.
|
Merged to unblock. Nicely done, thanks! Some followup requests:
|
…aws#6166 ## Problem Wizard framework does not support: - async `showWhen()` provider. - restoring previous state of WizardPrompter in nested wizard when user clicks BACK button. - skipping prompter in backward direction. ## Solution - introduce `NestedWizard` class that uses child wizards as prompters with support for instantiating and restoring child wizards in backward direction. - update wizard state controller to support `ControlSingal.Skip` and add concept of wizard direction. - support async `showWhen()` provider. fix aws#6094
Problem
Attempt to fix #6094 as the current implementation of wizard does not support:
showWhenSolution
NestedWizardclass that create wizard classes as prompters with support for instantiating and restoring child wizards in backward direction.ControlSingal.Skipand add concept of wizard directionshowWhenclauseLicense: I confirm that my contribution is made under the terms of the Apache 2.0 license.