Fix Swap wasp.png to wasp.svg #1538
Conversation
|
@lalitkumawat1m did you test out the templates changes locally with |
|
It works fine |
|
If you tested it out locally, why does the CI fail on the e2e tests? 🙃 |
There was a problem hiding this comment.
@lalitkumawat1m Thanks for the effort!
I did a thorough review now and this PR is not in a great shape.
You mostly did global waspLogo.png -> waspLogo.svg string replace -> a lot of those replacements are incorrect or unneeded.
Also, you didn't follow those up with replacing actual waspLogo.png files with waspLogo.svg files except in one place.
In this state, I doubt that any but one example is correctly showing its logo, and all new projects that wasp would generate with wasp new I believe are also broken.
So I am not sure what you tested, when you said you tested it?
I left comments that will hopefully help with getting this PR to a better shape, but testing and understanding the changes is also a part of doing PR, so if you want to continue with this PR please put effort into that.
| @@ -903,7 +903,7 @@ This was the file tree of a newly generated project in the previous version of W | |||
| │ ├── Main.css | |||
| │ ├── MainPage.js | |||
| │ ├── .waspignore | |||
| │ └── waspLogo.png | |||
| │ └── waspLogo.svg | |||
There was a problem hiding this comment.
This shouldn't be changed, this and the line below, since this is ChangeLog -> so this is how it was at that point in the time and should be kept that way.
| @@ -46,7 +46,7 @@ This will generate a project skeleton in the folder `myApp`. The project structu | |||
| │ │ ├── MainPage.jsx | |||
| │ │ ├── react-app-env.d.ts | |||
| │ │ ├── tsconfig.json | |||
| │ │ └── waspLogo.png | |||
| │ │ └── waspLogo.svg | |||
There was a problem hiding this comment.
Old blog post, so let's keep it as it was.
| @@ -1104,7 +1104,7 @@ Ok, but what good is a function that fetches the data if we’ve got nowhere to | |||
| Let’s go now to our `src/client/MainPage.tsx` file (make sure it’s got the `.tsx` extension and not `.jsx`) and replace the contents with these below: | |||
|
|
|||
| ```tsx | |||
| import waspLogo from './waspLogo.png' | |||
| import waspLogo from './waspLogo.svg' | |||
There was a problem hiding this comment.
Also old blog post, let's keep it as it was.
| @@ -52,7 +52,7 @@ waspBuild/.wasp/build/web-app/src/ext-src/Main.css | |||
| waspBuild/.wasp/build/web-app/src/ext-src/MainPage.jsx | |||
| waspBuild/.wasp/build/web-app/src/ext-src/vite-env.d.ts | |||
| waspBuild/.wasp/build/web-app/src/ext-src/vite.config.ts | |||
| waspBuild/.wasp/build/web-app/src/ext-src/waspLogo.png | |||
| waspBuild/.wasp/build/web-app/src/ext-src/waspLogo.svg | |||
There was a problem hiding this comment.
You did search and replace here, but that is not how our e2e tests work. To update them correctly, you actually need to build and run the waspc Haskell project on your machine and update the e2e tests. Which is involved, you need to have Haskell Toolchain running on your machine (GHC, Cabal). We could run these for you possibly though, if you get everything else right! But you should certainly revert these manual changes on e2e tests that you did (everything inside of e2e-test dir).
| @@ -1,4 +1,4 @@ | |||
| import waspLogo from './waspLogo.png' | |||
| import waspLogo from './waspLogo.svg' | |||
There was a problem hiding this comment.
This is really one of the most important changes, this change here.
This tells Wasp to, when it generates new project, use .svg in it.
But that waspLogo.svg, you need to provide it also somewhere in the data/Cli/templates/basic/src/client/ dir, I don't think you did though, I don't think you replaced that waspLogo.png with waspLogo.svg? You should do that.
| @@ -1,4 +1,4 @@ | |||
| import waspLogo from './waspLogo.png' | |||
| import waspLogo from './waspLogo.svg' | |||
There was a problem hiding this comment.
All these changes you did in examples -> they are not really needed. It doesn't really matter if they use waspLogo.png, it is just a file generated at start. So I would revert these changes.
There was a problem hiding this comment.
Also, most of the example you corrected only half way -> you changed the import, but didn't swap .png file with .svg file. You did that in only one example. So I believe only one example works correctly.
|
Thank you for the contribution but this change affects our starter templates which are crucial for our Wasp CLI. I will now close this PR as I'm not confident that we tested out the changes in a good way. If you manage to test it locally, feel free to reopen. |
Description
Fixes #1534
Select what type of change this PR introduces:
Update Waspc ChangeLog and version if needed
If you did a bug fix, new feature, or breaking change, that affects waspc, make sure you satisfy the following: