We have moved to GitHub Issues
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.
@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?
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.
@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 :)
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?
@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.
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
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
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?
@warren.buckley and @claus I don't get this, this was already supported by using the
furtherOptions with something like
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.
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.
Backwards Compatible: False
Affected versions: 7.5.3
Due in version:
Sprint: Sprint 51