react-command-convert-to-pdf - Updated #1435
Conversation
NicolasKheirallah
changed the title
There was a problem hiding this comment.
@NicolasKheirallah Awesome work so far 👏👏👏👏
I think we are on the right track but it seems w have a few tiny steps w need to sort out before we proceed. I left a few comments, please do give the a double check 🙏
Also we should update the assets/sample.json file. We should update the updateDateTime and SPFX-VERSION metadata
Also did you generate the upgrade guidance for this project and go over the required things?
I noticed some were still not done like including eslint or modifying the yo-rc.json
There was a problem hiding this comment.
let's not include such a file in the sample. This seems to me more like a coding preference kinda approach rather than a file that refers to the sample itself.
Lets remove it
There was a problem hiding this comment.
we should also update in the Compatibility section the tags for SPFx to 1.20.0 and Node to v18.
There was a problem hiding this comment.
I think we are still missing this update. When I look at the readme file we still did not udpate the SPFx version and node version tags
| build.addSuppression(/error semicolon: Unnecessary semicolon$/); | ||
| build.addSuppression(/error semicolon: Missing semicolon$/); |
There was a problem hiding this comment.
we should not suppress errors in build toolchain. It would be best if we resolve those issues along the way.
If some warning is a pain we may just eslint file
| build.tslintCmd.enabled = false; |
There was a problem hiding this comment.
this is not a huge problem or a bug but seems to me a bit out of scope for the related issue. Our aim is to upgrade the project and fix it up if anything requires updates to make it work with SPFx 1.20.
Here we are adding an additional file for auto update of version number. I say lets avoid including additional side changes and make our update and PR simple and clean 👍
There was a problem hiding this comment.
Thanks for comments, feedback and guidance as well for take the time to go through it!
I've never used the SPFX upgrade CLI, Didn't know it existed!! That's gonna help quite a bit!
I'll fix it right away and make a new pull request for all the projects! :)
There was a problem hiding this comment.
no problem. in the related issue I mentioned two tools that might support you in the upgrade.
You may use either CLI for Microsoft 365 using the m365 spfx project upgrade command
or You may install the SPFx Toolkit VS Code extension and there is an upgrade action as well
|
@NicolasKheirallah are you still working on this? If the PR is ready for review please click on the 'Ready for review' button to let us know |
I must been blind, didn't even see that button 🤣 |
There was a problem hiding this comment.
@NicolasKheirallah I think we are almost there. I still noticed some things we missed or which is strange. Could you have a closer look?
There was a problem hiding this comment.
We should update the SPFx version and Node version tags
There was a problem hiding this comment.
Yes! :) Thanks so much for the help!!
There was a problem hiding this comment.
why change this file to tsx? I think it is not rendering anything right?
There was a problem hiding this comment.
Had some rendering inside it when I refactored it and trying some stuff and I forgot to change it back 😅
What's in this Pull Request?