Opened 7 years ago
Closed 5 years ago
#42463 closed defect (bug) (fixed)
Poor Description of add_image_size Params
Reported by: | miqrogroove | Owned by: | nikolastoqnow |
---|---|---|---|
Milestone: | 5.3 | Priority: | normal |
Severity: | normal | Version: | 3.9 |
Component: | Media | Keywords: | has-patch |
Focuses: | docs | Cc: |
Description
From https://core.trac.wordpress.org/browser/tags/4.8/src/wp-includes/media.php#L274
274 * @param bool|array $crop Optional. Whether to crop images to specified width and height or resize. 275 * An array can specify positioning of the crop area. Default false.
So it literally says to do one thing or another. But which is what? "Default false" has a meaning, but it is totally lost in this documentation.
Attachments (8)
Change History (36)
#2
@
7 years ago
- Keywords needs-refresh added
- Milestone changed from Awaiting Review to 5.0
- Owner set to nikolastoqnow
- Status changed from new to assigned
- Version changed from trunk to 3.9
Introduced in [27472].
@nikolastoqnow Thanks for your patch! Looks like an empty line slipped into the patch. The use of "we" should also be avoided.
#3
@
7 years ago
- Keywords good-first-bug needs-refresh removed
Hi @ocean90 ,
I have made the changes as you requested in @nikolastoqnow patch.
Please let me know if any changes on that.
Thanks,
Chetan
#4
@
7 years ago
How about adding something to the effect of:
- Resize to fit one dimension, then crop image to fit the other dimension.
- Keep source aspect and resize to fit both dimensions, allowing one dimension to become smaller than specified.
This is roughly how I would describe the two different outcomes. And assign them to "true" and "false".
This ticket was mentioned in Slack in #core-media by mike. View the logs.
7 years ago
#6
@
6 years ago
Taking a look at this, and noticed that there is an existing extended description further up in the docs that was added in [27472].
Is this sufficient? Should we move some of that documentation down, or add to it?
https://core.trac.wordpress.org/browser/trunk/src/wp-includes/media.php#L253
#8
@
6 years ago
The docblock is totally inadequate. My reading of it is literally "We will either scale or crop your photo, and the default is false." So wth? Am I to understand that if I set "true", then a 1000 x 1000 image will be "cropped" to, say, the middle 100x100 pixels and never resized? Based on my experience, that is not even accurate.
#10
@
6 years ago
- Milestone changed from 5.0 to 5.1
Moving to the 5.1 milestone due to the WordPress 5.0 focus on the new
editor (Gutenberg).
This ticket was mentioned in Slack in #core-media by killua99. View the logs.
6 years ago
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
6 years ago
#15
@
6 years ago
- Keywords needs-refresh removed
I don't checked the wording itself but it is now refreshed against current trunk.
#16
@
6 years ago
- Keywords needs-patch added; has-patch removed
Thanks for the refresh @audrasjb. The latest change to the wording improves the understanding of the boolean value, but omits the behavior of the $crop
parameter if it's set as an array. Ideally, the description for this parameter would summarize the behavior outlined in the description for this function:
If false (default), images will be scaled, not cropped.
If an array in the form of array( x_crop_position, y_crop_position ):
x_crop_position accepts ‘left’ ‘center’, or ‘right’.
y_crop_position accepts ‘top’, ‘center’, or ‘bottom’. Images will be cropped to the specified dimensions within the defined crop area.
If true, images will be cropped to the specified dimensions using center positions.
@
6 years ago
According with the discussion on slack channel the wording wasn't accurate and the need of mention about the extended description on the summary could help to describe the $crop
argument.
This ticket was mentioned in Slack in #core-media by killua99. View the logs.
6 years ago
#18
@
6 years ago
- Keywords has-patch added; needs-patch removed
- Milestone changed from 5.2 to 5.3
Thanks for the update, @killua99. I think we can still improve this slightly before committing. However, with 5.3 RC1 so close, I'm going to go ahead and move this forward to the 5.3 milestone and we can continue to iterate.
#19
@
6 years ago
I feel the same @joemcgill with the 5.2 RC 1 so close better to move it to 5.3. Let me know what do you have on your mind about grammar improvement.
This ticket was mentioned in Slack in #core by david.baumwald. View the logs.
5 years ago
#21
@
5 years ago
In the patch 42463.5.patch I've moved the description of the cropping behavior to the $crop
param doc.
This should resolve all ambiguity with the current explanation of the $crop
param, as it feels silly to re-explain the description of it provided above.
This ticket was mentioned in Slack in #core-media by pierlo. View the logs.
5 years ago
This ticket was mentioned in Slack in #core-media by pierlo. View the logs.
5 years ago
#24
@
5 years ago
- Keywords dev-feedback added
@joemcgill Do you want to do anything specific here, or should I make a couple final adjustments and commit?
Moving the docs to the param should allow IDEs to show the details as expected, correct?
This ticket was mentioned in Slack in #core-media by mike. View the logs.
5 years ago
#26
@
5 years ago
- Keywords dev-feedback removed
I think the approach @pierlo is taking in 42463.5.patch is really close. I've cleaned it up a bit in 42463.2.diff and think it's ready to commit.
#27
@
5 years ago
42463.5.diff just cleans up some whitespacing but is otherwise identical to 42463.2.diff. Aside: auto-numbering works a lot better when we use diff
as the extension since that's what grunt upload_patch
uses.
Add better description about crop parameter in add_image_size function