-
Notifications
You must be signed in to change notification settings - Fork 46
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
Metadata screenshot #621
Metadata screenshot #621
Conversation
admin/create-theme/theme-utils.php
Outdated
public static function process_screenshot( $file_path ) { | ||
$new_screeenshot_id = attachment_url_to_postid( $file_path ); | ||
$new_screenshot_metadata = wp_get_attachment_metadata( $new_screeenshot_id ); | ||
$upload_dir = wp_get_upload_dir(); | ||
|
||
$new_screenshot_location = path_join( $upload_dir['basedir'], $new_screenshot_metadata['file'] ); | ||
|
||
// TODO: We're not actually processing anything. The image might be of the wrong dimensions, too large, etc. | ||
// We're just getting the file location so that we can copy it. | ||
// Should we process it? Or just enforce that the screenshot is the correct size? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if we should actually do any processing to the media selected (resize, compress, etc) or just use it as we get it and enforce sizes etc?
I removed the |
@pbking Thank you for your quick implementation! I would like to test the backend a little more, but first I made some adjustments to the UI. What do you think? Are the buttons a little too noticeable?
|
I think they are fine either way. I like the option you just pushed slightly better though. 👍 |
432eaea
to
19f0ac6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
- PHP error logs will not occur when adding a screenshot to a theme that does not originally have it.
- The screenshot is resized appropriately
- Also works on Windows host OS
Hrm 🤨 Regarding the wide image... The size restrictions are noted as a MAXIMUM of 1200 x 900. Which the image fits in. The original is less than 900 pixels tall. I believe that the cropping/resizing is acting as I would expect. I tried another image that was very wide, but over 900 px tall (2500 x 1200) and it cropped and resized to the expected 1200 x 900 size. There might be a better way to handle this edge case (upscaling the image maybe? then cropping it?) but I don't think it's worth the effort (when a user should really be providing an image of the appropriate dimensions anyway...) If you believe that we should consider addressing that further would you mind opening an issue to explore additional solutions @madhusudhand ? |
Add ability for user to manage the theme's screenshot from Editor on Metadata Modal.
Fixes: #560
To Test:
The image should be replaced in the theme folder and sized to 1200 x 900 px.
If a screenshot existed prior it should be removed. (Especially if the previous screenshot was another file type)