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

Added additional settings. #316

Merged
merged 7 commits into from
Oct 14, 2024
Merged

Conversation

GuidoZam
Copy link
Contributor

@GuidoZam GuidoZam commented Oct 5, 2024

🎯 Aim

Added a new additional step setting when scaffolding a new project to specify if the node version manager configuration file should be created or not.

πŸ“· Result

image

image

βœ… What was done

  • Added a new project additional step to create or not the node version manager configuration file.
  • Added new VSCode setting createNodeVersionFileDefaultValue to define the default value of the new project additional step.
  • Added new VSCode setting nodeVersionManagerFile to specify which configuration file to be created.

πŸ”— Related issue

Closes: #285

@Adam-it Adam-it self-assigned this Oct 5, 2024
@Adam-it
Copy link
Member

Adam-it commented Oct 5, 2024

Awesome 🀩😍. I will review this ASAP

github-actions bot and others added 2 commits October 6, 2024 01:47
Automated check and update of SPFx extensions and webparts samples data
and aces sample data.

Co-authored-by: Adam-it <[email protected]>
@Adam-it Adam-it force-pushed the new-node-version-configurations branch from e898466 to a766d5e Compare October 7, 2024 21:54
Copy link
Member

@Adam-it Adam-it left a comment

Choose a reason for hiding this comment

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

@GuidoZam Awesome work so far πŸ‘πŸ‘πŸ‘πŸ‘πŸ‘. You Rock 🀩
I left a few comments. Please do give them a recheck.
When testing I also found some 2 strange behaviors we should fixup before we merge:

I don't know why but when I select creating nvm file together with other dependencies
image
it is set in the project.pnp file
image
but as a result in terminal I only get the nvm file installed no other npm install is running
image
I don't know why but this seem to be only like this when I have this new dependency set otherwise it works normally and installed everything so it seems like a bug

also
when you reset the create nvm file to default it will be set to true not false by default
image

most probably this is due to the fact that in package.json we set this a string "false" not false

package.json Outdated Show resolved Hide resolved
src/services/actions/Scaffolder.ts Outdated Show resolved Hide resolved
src/services/actions/Scaffolder.ts Outdated Show resolved Hide resolved
src/services/actions/Scaffolder.ts Outdated Show resolved Hide resolved
src/services/actions/Scaffolder.ts Outdated Show resolved Hide resolved
src/constants/WebviewCommand.ts Outdated Show resolved Hide resolved
src/constants/WebviewCommand.ts Outdated Show resolved Hide resolved
@Adam-it Adam-it marked this pull request as draft October 7, 2024 22:53
@Adam-it
Copy link
Member

Adam-it commented Oct 8, 2024

@GuidoZam I also forgot to mention that we should update the readme part with additional step in scaffolding form to now include this new option as well together with screenshots

@GuidoZam
Copy link
Contributor Author

GuidoZam commented Oct 9, 2024

Sure thing @Adam-it, I will check the possible bug and also update the documentation and get back to you when I have news.

data/sp-dev-fx-samples.json Outdated Show resolved Hide resolved
@GuidoZam
Copy link
Contributor Author

Hi @Adam-it, I verified and fixed all your reviews.
The two bugs are resolved:

  • The first one I think was a different behavior produced by the custom name of the terminal. I've removed that customization and everything's seems to work fine.
  • About the second bug you were right, it was just a type mistake, updating the package.json type resolved the issue.

I've also updated the README file, have a look and tell me if this way is ok for you. There are some new images and text for the Node version manager support (section 12 of the README).

About the data/sp-dev-fx-samples.json file I didn't update that on purpose, I guess I've missed something while mergin' the branches.

Let me know if there's something else to update (hopefully not 🀞).

@GuidoZam GuidoZam marked this pull request as ready for review October 13, 2024 16:53
@GuidoZam GuidoZam requested a review from Adam-it October 13, 2024 16:53
Copy link
Member

@Adam-it Adam-it left a comment

Choose a reason for hiding this comment

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

LGTM πŸ‘ and works perfectly πŸ‘
I did a couple of small fixups

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
data/sp-dev-fx-samples.json Outdated Show resolved Hide resolved
@Adam-it Adam-it merged commit e842630 into pnp:dev Oct 14, 2024
Adam-it pushed a commit that referenced this pull request Oct 19, 2024
## 🎯 Aim

Added a new additional step setting when scaffolding a new project to
specify if the node version manager configuration file should be created
or not.

## πŸ“· Result

![image](https://github.com/user-attachments/assets/e99d1ee8-2c0a-4070-8252-40086a50b222)

![image](https://github.com/user-attachments/assets/bca41a6a-7247-49e0-a51c-adb77d86982b)

## βœ… What was done

- [X] Added a new project additional step to create or not the node
version manager configuration file.
- [X] Added new VSCode setting `createNodeVersionFileDefaultValue` to
define the default value of the new project additional step.
- [X] Added new VSCode setting `nodeVersionManagerFile` to specify which
configuration file to be created.

## πŸ”— Related issue

Closes: #285
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.

πŸ’‘ [Feature]: Add setting to enable/disable the .nvmrc file creation
2 participants