-
Notifications
You must be signed in to change notification settings - Fork 571
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
Make file/file_list field preview images work with named sizes #848
Conversation
Methods for getting all available image sizes and returning a named image size from an array of values without passing an actual image are needed to output the correct images from the JS media uploader.
Filter doesn’t get called if hooked from CMB2_JS or elsewhere, I assume the filter is added too late. Probably needs a better home; calling from cmb2_bootstrap() for now.
…' fields In CMB2_Type_File[_List].php – try to get the closest named image size from the ‘preview_size’ array and get the correct width and height from the named size. Pass the named size string as well as the dimensions to the hidden field to be used by the JS media uploader. In cmb2.js we can then retrieve the correct image (url and dimensions) form the returned JS attachment object. Added logic to fit the image to the preview size rather than stretching the image. Updated the default preview sizes from the hardcoded 350x350px and 50x50px to ‘large’ and ‘thumbnail’. Fixes CMB2#844
Consistent with the behaviour of wp_get_attachment_image() (otherwise we may get different preview sizes before/after reload).
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.
Overall, this is a pretty great PR. I have not tested it, but after a code review, it looks very promising. I have left several inline comments, but here are some additional thoughts:
The changes in CMB2_Type_File
and CMB2_Type_File_List
share similar functionality to get the image size. It would be best to move that inline and repeated logic to a utility method in their base class, CMB2_Type_File_Base
.
The same goes for your JS updates.. Much of that should probably moved to methods to not have repeated logic/blocks of code.
Your last update, "Changed the default preview_size
from 350x350px (for file
fields) and 50x50px (for file_list
fields) to the named sizes large
and thumbnail
respectively." will be a problem for back-compatibility, so we should remove that change.
includes/CMB2_Field.php
Outdated
@@ -1093,7 +1093,7 @@ public function _set_field_defaults( $args ) { | |||
'date_format' => 'm\/d\/Y', | |||
'time_format' => 'h:i A', | |||
'description' => isset( $args['desc'] ) ? $args['desc'] : '', | |||
'preview_size' => 'file' == $args['type'] ? array( 350, 350 ) : array( 50, 50 ), | |||
'preview_size' => 'file' == $args['type'] ? 'large' : 'thumbnail', |
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.
This is problematic for back-compatibility. This can't really change.
includes/CMB2_Utils.php
Outdated
* @return false|string $data Named image size e.g. 'thumbnail'. | ||
*/ | ||
public static function get_named_size( $size ) { | ||
$image_sizes = self::get_available_image_sizes(); |
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.
i'd move this inside the if ( is_array( $size ) ) {
condition since it's not needed otherwise.
includes/types/CMB2_Type_File.php
Outdated
@@ -28,7 +60,8 @@ public function render() { | |||
'class' => 'cmb2-upload-file regular-text', | |||
'size' => 45, | |||
'desc' => '', | |||
'data-previewsize' => is_array( $img_size ) ? '[' . implode( ',', $img_size ) . ']' : 350, | |||
'data-previewsize' => sprintf( '[%s,%s]', $size_width, $size_height ), |
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.
Couldn't this be '[%d,%d]'
?
@@ -55,7 +87,7 @@ public function render() { | |||
if ( $this->is_valid_img_ext( $fullurl ) ) { | |||
|
|||
$output .= $this->img_status_output( array( | |||
'image' => wp_get_attachment_image( $id, $img_size ), | |||
'image' => wp_get_attachment_image( $id, $img_size, null, array( 'class' => 'cmb-file_list-field-image', 'style' => 'height: auto;' ) ), |
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.
We should probably abandon the inlines styles here.
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.
The 'auto' height is needed to stop the images being squashed if they're over the max-height that's set in the css, adding it here is probably just lazy though. I'll look at just adding it to the existing css.
includes/CMB2_JS.php
Outdated
* @param array $meta Array of attachment meta data ( from wp_get_attachment_metadata() ). | ||
* @return string filtered $response array | ||
*/ | ||
public static function prepare_image_sizes_for_js( $response, $attachment, $meta ) { |
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.
It's tough to know the best place for these things, but I feel like this static method would be better suited for CMB2_Type_File_Base
.
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.
And to hook it in, I'd probably do something similar to the other field-specific stuff, like oembed ajax hooks:
if ( in_array( $field['type'], array( 'file', 'file_list' ) ) ) {
// Initiate attachment JS hooks
add_filter( 'wp_prepare_attachment_for_js', 'CMB2_Type_File_Base::prepare_image_sizes_for_js', 10, 3 );
}
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.
Honestly, it was a struggle trying to decide where to hook this in, this sounds better!
includes/types/CMB2_Type_File.php
Outdated
@@ -74,10 +107,9 @@ public function render() { | |||
if ( $this->is_valid_img_ext( $meta_value ) ) { | |||
|
|||
if ( $_id_value ) { | |||
$image = wp_get_attachment_image( $_id_value, $img_size, null, array( 'class' => 'cmb-file-field-image' ) ); | |||
$image = wp_get_attachment_image( $_id_value, $img_size, null, array( 'class' => 'cmb-file-field-image', 'style' => 'height: auto;' ) ); |
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.
We should probably abandon the inlines styles here.
includes/types/CMB2_Type_File.php
Outdated
} else { | ||
$size = is_array( $img_size ) ? $img_size[0] : 350; | ||
$image = '<img style="max-width: ' . absint( $size ) . 'px; width: 100%; height: auto;" src="' . $meta_value . '" alt="" />'; | ||
$image = '<img style="width: auto; height: auto;" src="' . $meta_value . '" class="cmb-file-field-image" alt="" />'; |
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.
We should probably abandon the inlines styles here.
js/cmb2.js
Outdated
|
||
// Preview size dimensions | ||
var previewWidth = media.previewSize[0] ? media.previewSize[0] : 150; | ||
var previewHeight = media.previewSize[1] ? media.previewSize[1] : 150; |
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.
Looks like you're changing this default as well (from 50 to 15), which again, is problematic for back-compatibility.
js/cmb2.js
Outdated
|
||
// Get the correct dimensions and url if a named size is set and exists | ||
// fallback to the 'large' size | ||
if ( typeof( this.sizes[ media.sizeName ] ) !== 'undefined' ) { |
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.
use 'yoda' conditions (here and in the rest of your code)
moved get_available_image_sizes() in to the scope where it is needed.
Abandoned inline styles to and moved to external stylesheet. JS media attachment filter moved to a more appropriate place.
Thanks for the review @jtsternberg Completely agree on all points; I've now moved all the repeating logic to utility methods. Your suggestion on hooking the filter makes much more sense too. Also reverted the changes to the defaults. Think I've addressed all your initial points, it's late though! I moved the inline styles to the CSS file but my compiled CSS is totally different, uses spaces instead of tabs etc (the diff basically shows every line as replaced) so I just committed the sass file for now. |
Awesome, thanks for the quick turnaround. I won't get a chance to test this till next week, but feel free to keep bumping this thread if I don't get around to it. I'd like to get this in the next release. No worries about the compiled assets. I can take care of those. |
@jtsternberg did you get a chance to test this at all? I've been a bit snowed under myself but I'm going to try and do a bit more thorough testing on it in the next few days (I want to get this in a project I'm working on now). |
Reviewing/testing this now. Please don't push any more changes as I'm starting to clean up, etc. If you have other changes to propose, let me know, and i'll push up the new branch to collaborate on. |
Thanks a ton for your work @Cai333. This has been merged to trunk, and will be in the next release which I'll try to get out soon. |
No problem at all @jtsternberg |
Fixes #844.
Add some utility methods for working with named images.
Filter the attachment meta data passed to the JS media uploader and adds all custom
(i.e. registered with
add_image_size()
) image sizes.File and File List fields try to get the closest named image size if passed an array for the
preview_size
parameter. If passed a string, the string is checked against existing image sizes. The named size is then passed as a data attribute to the hidden field to be used by the JS media uploader.The JavaScript that handles the media uploader now checks the named size passed from the hidden field and retrieves the correct image from the attachment object returned by the media uploader. The dimensions are then adjusted to fit within the preview size (rather than being stretched as they were before).
Changed the default
preview_size
from 350x350px (forfile
fields) and 50x50px (forfile_list
fields) to the named sizeslarge
andthumbnail
respectively.