Add one step samples for Word and Excel in Yeoman #794
Conversation
There was a problem hiding this comment.
This seems to diverge quite a bit from the design and intent of this plugin. I samples case seems to do a lot of special casing to get unique behavior and experience and as a result we get mix of yeoman functionality and a custom cmd experience. You've introdued the use of git, cloning instead of copying the source code, and extra user prompts . . . all of with are similar to but the the same as existing behavior.
You also have a lot of code that looks like it is mostly the same, but is different in only a few simple ways. This should be made generic and parameterized instead of trying to maintain multiple copies of the same thing.
The basic idea of generating samples vs. blank projects needs to be thought of more thoroughly to come up with a long term design.
package.json
Outdated
| @@ -69,6 +77,7 @@ | |||
| "typescript": "^4.8.2", | |||
| "yeoman-assert": "^3.1.1", | |||
| "yeoman-environment": "^3.13.0", | |||
| "yeoman-test": "^6.3.0" | |||
| "yeoman-test": "^6.3.0", | |||
| "yo": "^4.3.1" | |||
There was a problem hiding this comment.
Why are we importing 'yo' . . . it's not used by the generator.
| @@ -158,5 +184,16 @@ | |||
| "word": { | |||
| "displayname": "Word" | |||
| } | |||
| }, | |||
| "sampleOptions": { | |||
src/app/config/projectsJsonData.ts
Outdated
| return this.m_projectJsonData.projectTypes[_.toLower(projectType)].displayname; | ||
| let displayName = this.m_projectJsonData.projectTypes[_.toLower(projectType)].displayname; | ||
|
|
||
| if (projectType === 'excel_sample' || projectType === 'word_sample' || projectType === 'excel_hello_world' || projectType === 'word_hello_world') { |
There was a problem hiding this comment.
We shouldn't be highlighting one project over another.
src/app/config/projectsJsonData.ts
Outdated
| return displayName; | ||
| } | ||
|
|
||
| getProjectDisplayNames() { |
src/app/index.ts
Outdated
| @@ -96,6 +111,16 @@ module.exports = class extends yo { | |||
| type: Boolean, | |||
| description: 'Get more details on Yo Office arguments.' | |||
| }); | |||
|
|
|||
| this.option('excel_sample', { | |||
There was a problem hiding this comment.
Why do we need these new options?
| const crt_path = '.office-addin-dev-certs\\localhost.crt'; | ||
| const key_path = '.office-addin-dev-certs\\localhost.key'; | ||
| const full_path_crt = homeDirectory + '\\' + crt_path; | ||
| const full_path_key = homeDirectory + '\\' + key_path; |
There was a problem hiding this comment.
What are these being used for?
| const full_path_key = homeDirectory + '\\' + key_path; | ||
|
|
||
| shell.cd('./Samples/hello-world/excel-hello-world'); | ||
| shell.exec('npm install -g office-addin-debugging', {async:true}, (code, stdout, stderr) => { |
There was a problem hiding this comment.
This should be part of the project and not installed seperately.
We also should not be installing things globally . . . we're targeted at a project and not modifying the general user environment.
| spinner.setSpinnerString('|/-\\'); | ||
| spinner.start(); | ||
|
|
||
| shell.exec('git clone https://github.com/OfficeDev/Excel-Scenario-based-Add-in-Samples.git', {async:true}, (code, stdout, stderr) => { |
There was a problem hiding this comment.
By cloning the project you are keeping a user connection with the original template/sample repo . . . which is not what we're trying to do here. We're creating a new project that is not connected back to a template repo. We also already have code in this plugin that accomplishes that . . . this is taking a completely divergent path.
| @@ -362,6 +451,13 @@ module.exports = class extends yo { | |||
| }); | |||
| } | |||
|
|
|||
| _postOpenSampleHints(): void { | |||
There was a problem hiding this comment.
We don't need to special case general information.
There was a problem hiding this comment.
We don't need to special case general information.
Can we just put the special case here temporarily for sample add-ins?
There was a problem hiding this comment.
This is shipping code. Even "temporary" things need to be ship quality.
tsconfig.json
Outdated
| @@ -1,5 +1,6 @@ | |||
| { | |||
| "compilerOptions": { | |||
| "allowJs": true, | |||
|
If you want to experiment with a yeoman generator in ways that are very different from the established patterns here than maybe you should consider a separate yeoman plug-in (possibly forked from this code). On top of that . . . if it is something that doesn't use the generator option, then why are you using yeoman in the first place? The same thing could be accomplished with through other means (like doing more in the bath file). |
Thank you for your pull request. Please provide the following information.
Do these changes impact User Experience? (e.g., how the user interacts with Yo Office and/or the files and folders the user sees in the project that Yo Office creates)
If Yes, briefly describe what is impacted.
There will be 2 more options for users' choices:
Users could 1-step to get the sample add-in code, edit it and automatically launch.
Do these changes impact documentation? (e.g., a tutorial on https://learn.microsoft.com/en-us/office/dev/add-ins/overview/office-add-ins)
If Yes, briefly describe what is impacted.
There will be an one-line command added for users to use the scripts in yeoman.
Validation/testing performed:
We have tested the parameters, options and error handlings
Platforms tested: