Add user-friendly option values for spo site commsite enable. #5366
Conversation
|
thanks for another awesome PR. We will review this ASAP |
There was a problem hiding this comment.
Hi @Saurabh7019, great work! 💪 let's get some things fixed before we merge this.
Saurabh7019
force-pushed
the
issue-5199
branch
from
76e2476
to
deb5cac
Compare
|
Hi Martin, I have removed the DesignPackage Enum to make the code easier; could you please review it and let me know your thoughts? |
There was a problem hiding this comment.
Hi @Saurabh7019, looks good to me. I've got some minor comments that I'll fix myself during the merge. 👏
| @@ -64,10 +79,14 @@ class SpoSiteCommSiteEnableCommand extends SpoCommand { | |||
| } | |||
|
|
|||
| public async commandAction(logger: Logger, args: CommandArgs): Promise<void> { | |||
| const designPackageId: string = args.options.designPackageId || '96c933ac-3698-44c7-9f4a-5fd17d71af9e'; | |||
| const designPackageId = args.options.designPackageId || { | |||
There was a problem hiding this comment.
I find this code slightly hard to read. Shall we move this to a this.getDesignPackageId(args.options) function? That function can return the options.designPackageId immediately and otherwise run a switch statement on the options.designPackage.
|
|
||
| if (this.verbose) { | ||
| logger.logToStderr(`Enabling communication site at ${args.options.url}...`); | ||
| logger.logToStderr(`Enabling communication site with design package '${args.options.designPackage || 'Topic'}' at '${args.options.url}'...`); |
There was a problem hiding this comment.
This would not log options.designPackageId in the current state. Let's just log the local designPackageId variable.
|
|
||
| throw 'Invalid request'; | ||
| }); | ||
| await command.action(logger, { options: { designPackage: 'Topic', url: 'https://contoso.sharepoint.com' } } as any); |
There was a problem hiding this comment.
We're missing an assert statement here.
|
Merged manually, thank you for the awesome work! 👏🎉 |
Closes #5199