Conversation
mikachan
left a comment
There was a problem hiding this comment.
This is fab!
I've left a couple of comments. I also wanted to check if we should be validating the image files server-side as well as the HTML form validation. Things such as the file size, and if there's a way to check if the file is actually an image, maybe by checking the mimetype as well as the type.
| // Add screenshot.png. | ||
| // Add screenshot. | ||
| $screenshot_path = __DIR__ . '/../screenshot.png'; | ||
| if ( is_uploaded_file( $screenshot['tmp_name'] ) && $screenshot['type'] === 'image/png' ) { |
There was a problem hiding this comment.
We're checking the type of the file here is an image/png, but on the upload form, we say it can be a .png, .jpg, or .jpeg. I'm guessing we should probably test if the image is png or jpg/jpeg?
| <?php _e('Screenshot:', 'create-block-theme'); ?><br /> | ||
| <small><?php _e('Upload a new theme screenshot (2mb max | .png,.jpg,.jpeg allowed | 1200x900 recommended)', 'create-block-theme'); ?></small><br /> | ||
| <input type="hidden" name="MAX_FILE_SIZE" value="20000000" /> | ||
| <input type="file" accept=".png" name="screenshot" id="screenshot" class="upload"/> |
There was a problem hiding this comment.
We're only accepting .png here too.
There was a problem hiding this comment.
I modified this to only accept png, since that seems to be the standard for screenshots. What do you think?
There was a problem hiding this comment.
Yeah I think that makes sense, .png definitely seems to be the standard for screenshots.
|
I refactored this and I think it's ready for another review. One thing is that it fails silently if the screenshot is not valid or too big, maybe we should add in some error handling as a follow up? |
mikachan
left a comment
There was a problem hiding this comment.
Nice, I really like the is_valid_screenshot function! This is testing well for me.
One thing is that it fails silently if the screenshot is not valid or too big, maybe we should add in some error handling as a follow up?
Yep, this sounds like a good thing to follow up on separately.
Fixes #74
To test, try out adding a screenshot and generating a theme with the Create child, Clone, and Blank theme options.