Skip to content

Conversation

@roger-zhangg
Copy link
Member

@roger-zhangg roger-zhangg commented Oct 19, 2024

Problem

Currently prompter doesn't support multipick

Solution

Add a new function createMultiPick that supports multipick and can be used in Wizard.
The returned result will be a list encoded by JSON.stringify , use JSON.parse to recover the list

Proposed UX

image

// define DataQuickPickItem, note you can use `picked: true` to pre-select items in the multipick
const syncFlagItems: DataQuickPickItem<string>[] = [
    {
        label: 'Build in source',
        data: '--build-in-source',
        description: 'Opts in to build project in the source folder. Only for node apps',
    },
    {
        label: 'Code',
        data: '--code',
        description: 'Sync only code resources (Lambda Functions, API Gateway, Step Functions)',
        picked: true,
    }
]

export interface SyncParams {
    readonly syncFlags: string
}

// define the wizard
export class SyncWizard extends Wizard<SyncParams> {
    public constructor() {
        super()
        this.form.syncFlags.bindPrompter(() => {
            return createMultiPick(syncFlagItems, {
                title: 'Specify parameters for sync',
                placeholder: 'Press enter to proceed with highlighted option',
                buttons: createCommonButtons(samSyncUrl),
            })
        })
    }
}

// run the wizard
result = await new SyncWizard().run()

// Use `JSON.parse` to decode to list -> ['--build-in-source','--code']
console.log(JSON.parse(result.syncFlags))

TODO

Add Tests


License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@roger-zhangg roger-zhangg requested a review from a team as a code owner October 19, 2024 01:46
@github-actions
Copy link

This pull request modifies code in src/ but no tests were added/updated. Confirm whether tests should be added or ensure the PR description explains why tests are not required.

@github-actions
Copy link

This pull request implements a feature or fix, so it must include a changelog entry. See CONTRIBUTING.md#changelog for instructions.

@jpinkney-aws
Copy link
Contributor

The code itself looks fine to me but I think we should really have tests for something like this before we merge it

@roger-zhangg
Copy link
Member Author

Thanks @jpinkney-aws I'm working on that, will ping you once added

@roger-zhangg
Copy link
Member Author

Hi @jpinkney-aws Please take another look when available. Thanks!

@justinmk3
Copy link
Contributor

/runintegrationtests

Copy link
Contributor

@justinmk3 justinmk3 left a comment

Choose a reason for hiding this comment

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

Tests are passing and the change is isolated, so we can iterate on this if needed. Meanwhile, it's great to have this in mainline. Thank you!

@justinmk3
Copy link
Contributor

The failing integ tests are perf tests, unrelated to this PR.

@justinmk3 justinmk3 merged commit d2bc2d6 into aws:master Oct 22, 2024
27 of 30 checks passed
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.

3 participants