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

Upgrade InjectCSS sample from SPFx v1.8.0 to v1.20.0 #1416

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

tom-daly
Copy link
Contributor

@tom-daly tom-daly commented Oct 1, 2024

By submitting this pull request, you agree to the contribution guidelines

If you aren't familiar with how to contribute to open-source repositories using GitHub, or if you find the instructions on this page confusing, sign up for one of our Sharing is Caring events. It's completely free, and we'll guide you through the process.

Q A
Bug fix? no
New feature? no
New sample? no
Related issues? #1414

What's in this Pull Request?

These changes upgrade the solution from SPFx v1.8.0 to v1.20.0.

It also fixes the warning from the linter by adding the proper typing

It also includes modified instructions on how to deploy using PnP PowerShell since the new instructions Sept 9, 2024.

@tom-daly tom-daly changed the title Upgrade solution to spfx v1.20.0 Upgrade InjectCSS sample from SPFx v1.8.0 to v1.20.0 Oct 1, 2024
@Adam-it Adam-it self-assigned this Oct 1, 2024
@Adam-it
Copy link
Contributor

Adam-it commented Oct 1, 2024

Thank you for opening a PR to our samples repo and helping us upgrade our project 👍
We will try to review this PR ASAP

Copy link
Contributor

@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.

@tom-daly Awesome work 👏👏👏
I had a quick check and managed to build and deploy the solution on my tenant. You Rock 🤩

I noticed a few old dependencies which should be aligned with SPFx version. I know the extension does not use them but since they are in the packge.json file we may fix them up as well. Please do give them a double check before we proceed 🙏

this kind of info you may always catch using the validate action using SPFx Toolkit or m365 spfx project doctor command using CLI for Microsoft 365.

samples/react-application-injectcss/package.json Outdated Show resolved Hide resolved
samples/react-application-injectcss/package.json Outdated Show resolved Hide resolved
samples/react-application-injectcss/package.json Outdated Show resolved Hide resolved
samples/react-application-injectcss/package.json Outdated Show resolved Hide resolved
@Adam-it Adam-it marked this pull request as draft October 6, 2024 21:52
@tom-daly
Copy link
Contributor Author

tom-daly commented Oct 6, 2024

This was the only 1 of my commits where I used the m365 cli to check the upgrade and it didn't work well. I also ran the dr and it did not pick up your suggestions but just remove gulp and add gulp-cli. I've taken the package dependencies from a clean v1.20.0 project and will be checking that in momentarily.

@tom-daly tom-daly marked this pull request as ready for review October 6, 2024 23:23
@Adam-it
Copy link
Contributor

Adam-it commented Oct 7, 2024

This was the only 1 of my commits where I used the m365 cli to check the upgrade and it didn't work well. I also ran the dr and it did not pick up your suggestions but just remove gulp and add gulp-cli. I've taken the package dependencies from a clean v1.20.0 project and will be checking that in momentarily.

I used SPFx Toolkit to support me in this review and the validate action picked up this as well.
if you had any problems with CLI for M365 please (if you have time) open up an issue in the CLI repo so that we may pick it up.
Anyway thanks for a quick turn around. I will do a recheck ASAP

Copy link
Contributor

@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.

@tom-daly thanks for the quick turn around.
I think we are almost there. Let's do a bit of clean up and we should be ready to merge 🚀

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should not update the package-lock.json which is outside of the sample folder.
Lets revert this

samples/react-application-injectcss/build.cmd Outdated Show resolved Hide resolved
@Adam-it Adam-it marked this pull request as draft October 7, 2024 00:35
@Adam-it
Copy link
Contributor

Adam-it commented Oct 22, 2024

@tom-daly are you still working on this PR? If it is ready to be merged please do let us know by hitting the Ready for review button 👍

@tom-daly tom-daly marked this pull request as ready for review October 23, 2024 01:07
@tom-daly tom-daly requested a review from Adam-it October 23, 2024 01:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants