-
-
Notifications
You must be signed in to change notification settings - Fork 26.8k
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
Handle .webp images #442
Comments
Can you send a PR for this please? |
@gafemoyano Would you like assistance on a PR? |
I've never done a PR before, if you could point me in the right direction on how to do it properly I'd be happy to help. |
Of course! I'll touch base with you tomorrow, however in the meantime, if you haven't already:
The people here are great, there is a fantastic rule once you take on the issue, it is yours, no one will attempt to "beat you to the race." Helpful links: |
Hi, thanks for the useful links! I was wondering what would qualify as a test plan as stated on CONTRIBUTING.md for a change like this. |
Is it necessary to modify this line on createJestConfig? I haven't used jest before so I'm not exactly sure of what that line does. |
@gafemoyano a "test plan" is nothing more than: "Hey! I made these changes, please test the following to make sure it looks good on your end." So you are adding webp images, perhaps put a small note in the PR on how to test that this works, maybe provide a sample image for someone reviewing to test on their local machine, etc... |
So is it alright if I commit the changes I made to the project template to test that the changes were working? |
Maybe not the other pull requests I checked out only commit the files that actually matter on the PR. |
Did you test locally? Give sh tasks/e2e.sh a go, I like to run Travis off On Aug 16, 2016 9:25 PM, "Felipe Moyano" notifications@github.com wrote:
|
Yes, thanks. Also our Flow instructions in
Ideally not, just include a link to |
Thanks guys, I've gone ahead and created the pull request. @ryanyogan As a side note I couldn't really get the e2e.sh script to work properly. I also tried running it on the master branch to make sure my changes weren't the issue but it kept giving me this error message:
Which is weird cause the build/favicon.ico file that gets generated includes a hash i.e. but on
|
@gafemoyano Everything looks fine here: https://travis-ci.org/facebookincubator/create-react-app/jobs/152982529 here, I know recently someone added hashing to the assets however the tests (someone correct me if I am wrong) are good. I think you should be good as long as you followed @gaearon's suggestions in the PR add a link to a sample image so someone reviewing can download a .webp image easily for testing. Make sure you updated the flow config updates (/templates/README.md) he mentioned as well. You should be golden after that. Good work, I should note I am not on the FB team, just someone that believes in this project so I can only comment as far as my knowledge on the project goes. Edit: It looks like you did that, congrats on your first OSS PR, you picked an awesome project to contribute to! |
Thank you so much @ryanyogan for guiding me through this!! I couldn't have done it without your help and I'm glad that there's people like you on this community to guide newcomers like me. I'm very excited to finally contribute to a project that it's growing on me and I've been finding increasingly useful every day. So thanks again to both Ryan and Dan for being awesome and encouraging me to do this (admittedly very small) first PR. |
Fixed by #458. |
Released in 0.2.3, thanks! |
Hi
I think create-react-app can't handle .webp files right now since it throws the error message:
I wouldn't like to eject just to modify webpack.config so I was wondering if the .webm extension could be added to the file-loader config.
Of course I could just convert the images to jpeg but maybe it's not such an uncommon file format and handling it could improve developer experience
The text was updated successfully, but these errors were encountered: