U4-8963 - Updates for ImageProcessor parameters to support background color

Created by Claus Jensen 12 Sep 2016, 07:37:15 Updated by Claus Jensen 19 Apr 2017, 06:34:33

Relates to: U4-9781

Subtask of: UAASSCRUM-765

Update the GetCropUrl methods to support the background color parameter.

Comments

Claus Jensen 12 Sep 2016, 07:39:03

PR: https://github.com/umbraco/Umbraco-CMS/pull/1474

@sebastiaan Could you confirm that adding an optional parameter to GetCropUrl (and not reordering the existing parameters) isn't a breaking change as it should still work as it used?


Claus Jensen 12 Sep 2016, 10:38:36

Ah - guess it will have to be put in as a breaking change since adding the overload would create ambigious signatures due to optional parameters...

Splitting this in two issues so we can get the other change in as a non-breaking change.. then this can be merged whenever we can.


Warren Buckley 15 Sep 2016, 12:57:09

@claus reviewed this & happy with the changes made in the PR but as this is pegged for 7.6.0 and I believe the next version is 7.5.4 I have not merged this back into dev-v7 yet.

Let me know what you would like me to do please mate :)


Claus Jensen 15 Sep 2016, 13:41:45

Yep - this should not be merged into dev-v7 now. I'm not sure how we will handle this .. do we have some kind of waiting state for the issue so we don't forget about it and can merge it into 7.6 when we get there?


Warren Buckley 15 Sep 2016, 14:12:29

@Sebastiaan passing this over to you, but reviewed the changes in the PR & happy just need to know where/when to merge this in and to ensure this does not get forgotten about.


Warren Buckley 26 Jan 2017, 20:42:42

OK updated the PR to target dev-7.6 Run the unit test manually as its been a while since this PR was submitted so just to be 100% sure and its all good Performed a test using the @Url.GetCropUrl() extension method on UrlHelper and can confirm that it works correctly in producing a string that can be used in an HTML image element which includes the correct bgcolor querystring.

All tested and OK marking as fixed & merging in


Shannon Deminick 19 Apr 2017, 05:23:55

Yes this is a breaking change and it causes breaks: http://issues.umbraco.org/issue/U4-9781

... but there doesn't seem to be a reason to have to break it we don't need to add optional parameters, we can create new overloads, i'll fix it


Shannon Deminick 19 Apr 2017, 05:29:15

Why couldn't we have just fixed this the same way that was done for http://issues.umbraco.org/issue/U4-8751 and use the furtherOptions?


Shannon Deminick 19 Apr 2017, 05:32:06

@warren.buckley and @claus I don't get this, this was already supported by using the furtherOptions with something like

&bgcolor=#CC0000


Shannon Deminick 19 Apr 2017, 05:38:18

This can be acheived by doing this for example:

var urlString = mediaPath.GetCropUrl(400, 400, cropperJson, imageCropMode: ImageCropMode.Pad, furtherOptions: "&bgcolor=fff");

and we don't introduce any breaking changes.


Claus Jensen 19 Apr 2017, 06:34:33

For the same reason that every other one of the parameters could just have been specified as a furtherOptions magic string ... it's not really user friendly to do it this way and requires you to go read docs to find out what the actual parameter name is, and then figure out that you can specify those in our furtherOptions to achieve the effect (I had no idea until I spent some time reading through our code and the ImageProcessor docs - and then suggested this PR).

Background color is a pretty common parameter to be setting when using crops/padding, so we should be including it as a parameter to have intellisense kick in and help people using this without having to know magic strings.


Priority: Normal

Type: Bug

State: Closed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: False

Fix Submitted:

Affected versions: 7.5.3

Due in version:

Sprint: Sprint 51

Story Points:

Cycle: