pnp / vscode-viva

With the SharePoint Framework Toolkit extension, you can create and manage your SharePoint Framework solutions on your tenant. All actions you need to perform during the development flow are at your fingertips.
https://marketplace.visualstudio.com/items?itemName=m365pnp.viva-connections-toolkit
MIT License
38 stars 17 forks source link

šŸ’” [Feature]: Add setting to enable/disable the .nvmrc file creation #285

Closed GuidoZam closed 1 week ago

GuidoZam commented 3 months ago

šŸŽÆ Aim of the feature

Add a new additional step setting when scaffolding a new project, the additional YES/NO switch will have a label 'Create node version manager configuration file (.nvmrc)'. This setting will be added after the already existing ones:

image

The setting, if set to YES, will create the .nvmrc file when scaffolding the new project.

Other than adding the new additional step two new extension settings will be added:

šŸ“· Images (if possible) with expected result

No additional remarks

šŸ¤” Additional remarks or comments

No additional remarks

Adam-it commented 3 months ago

@GuidoZam thanks for this suggestion šŸ‘. I think it is a really awesome idea, let's give it though and discuss different options and then prototype(draft/spec) how it should be done before we open it up. I will try to get back with more comments and questions regarding this issue later during the day šŸ‘

Adam-it commented 3 months ago

@GuidoZam some feedback from my side on this idea:

  1. This new setting seems strictly related to project scaffolding. VS Code extension setting are usually something most of us (developers) do not change šŸ˜‰ so if we put this there it will get little visibility. Besides that we already have some additional 'settings' that one may be interested when scaffolding new project and this may be found in the 3rd step of the scaffolding form. IMO I would rather add this as a new option in the 'Additional Steps' section in the scaffolding form. Just another YES/NO switch with label like 'create node version manager configuration file (.nvmrc)' or something similar. image
  2. Then if the above suggestion would be fine with you in the VS code extension settings I would rather add a setting if this should be 'turned on' (so set as YES) by default. Otherwise by default it will be NO which should be this default... default value šŸ˜…
  3. SPFx Toolkit supports either NVM or NVS. NVM only supports .nvmrc (at least this is what I think šŸ˜…), but NVS supports both .nvmrc and .node-version. Maybe we should also include additional setting, enabled only when someone selected NVS as preferable node version manager, to select if .nvmrc should be created (default) or .node-version. image

What do you think of the above šŸ‘†? IMO applying those suggestions to your idea will make this more flexible and visible.

As for the additional remark. For windows one may use nvm-windows which is also the same as nvm https://github.com/coreybutler/nvm-windows?tab=readme-ov-file in this case nvm use respect the .nvmrc file. At least I think it does šŸ˜…. It does seem to work šŸ‘

GuidoZam commented 3 months ago

@Adam-it my reply to your points:

  1. You're right, adding a new toggle in the 'Additional Steps' section definitely seems a better way and place to manage this.
  2. Seems a great idea, and I think it's even more useful and personalizable this way.
  3. Also this point seems legit. The .nvmrc file will be the default for both nvm and nvs while when the user select nvs as default Node version manager it will also make available an additional setting where it can select between .nvmrc or .node-version.
Adam-it commented 3 months ago

Awesome šŸ¤©šŸ˜. @GuidoZam may I kindly ask you to update the initial post of this issue with what we clarified šŸ™. You may of course copy paste most of my points and this will be our spec (prototype) of this feature. After that let's open this up. Would you like to get assigned?

GuidoZam commented 3 months ago

@Adam-it I've updated the issue, I think it's ok this way but if you need more clarification just tell me šŸ˜„ For the assignment: yes, assign this to me, I will do my best to improve this awesome extension!

Adam-it commented 3 months ago

@Adam-it I've updated the issue, I think it's ok this way but if you need more clarification just tell me šŸ˜„ For the assignment: yes, assign this to me, I will do my best to improve this awesome extension!

AWESOME!. All yours šŸ‘šŸ˜šŸ¤©

Adam-it commented 1 month ago

Hi @GuidoZam Hows the work coming up with this issue? Do you need any additional help from my side? This year the SPFx Toolkit repo will participate in hacktoberfest and this issue will count so if you are also planning to join this event please do remember to rise a PR during October (no sooner no later šŸ˜‰)

GuidoZam commented 1 month ago

Hey @Adam-it! The work is almost done, I'm having some difficulties with the dynamic update of the setting that specify which Node version file to use. Do you have any advice on that?

Good to know about the hacktoberfest, I will surely commit all the work in time to participate!

Adam-it commented 1 month ago

I'm having some difficulties with the dynamic update of the setting that specify which Node version file to use. Do you have any advice on that?

What do you mean by dynamic update?

GuidoZam commented 1 month ago

I mean to hide or show the nodeVersionConfigurationFile, as mentioned in the issue description:

nodeVersionConfigurationFile: this setting will define what Node version configuration file to use. If the user has selected NVM as Node versioning software this setting won't display, instead if the NVS versioning software is used the user can choose the configuration file to use.

Once done that I will commit the changes!

Adam-it commented 1 month ago

maybe it doesn't need to be dynamically show/hidden only when NVS. IMO this setting may always be present but in its description we may just include information that it applies only when NVS manager is selected and for NVM it won't be considered

GuidoZam commented 1 month ago

That was my plan B! šŸ˜„ I'll try to reserve some time to finalize this next week, I'll keep you updated!

GuidoZam commented 3 weeks ago

Hey @Adam-it I finally commited the changes! Everything is working fine.

JFYI I had to cancel the previous PR and recreate a new one because there was some issue with some previous commit.

Adam-it commented 1 week ago

Awesome work šŸ‘šŸ‘šŸ‘. PR merged and I will try to make a new pre-release with this ASAP šŸ‘