Skip to content
This repository has been archived by the owner on Oct 13, 2020. It is now read-only.

Background fill not working. #87

Merged
merged 3 commits into from
Dec 9, 2013
Merged

Background fill not working. #87

merged 3 commits into from
Dec 9, 2013

Conversation

joehoyle
Copy link
Member

Check the attached example, I uploaded a red box and it filled the background with black.

fill

Example code to see this in evidence:
echo '<img src="' . wpthumb( 'http:https://s8.postimg.org/l0pouom41/image.jpg', 'width=500&height=500&crop=1&background_fill=solid' ) .'"" />

@ghost ghost assigned mattheu Oct 24, 2013
@mecha
Copy link

mecha commented Oct 30, 2013

Is this issue being taken into account please ? Thanks

@joehoyle
Copy link
Member

Yup, @mattheu reproduced, working on fixing the unit tests to track it down.

@jgalea
Copy link
Author

jgalea commented Nov 6, 2013

Any ETC for this one?

@ghost ghost assigned joehoyle Nov 10, 2013
@joehoyle
Copy link
Member

Hi @jgalea I have pushed the code to fix this to this branch - could you test it and let me know if it's all good? Quick note: the argument name is now background_fill=auto

@jgalea
Copy link
Author

jgalea commented Nov 11, 2013

It's still not working Joe, see the examples attached.

Example 1 - Source
source1
Example 1 - Result
result1

Example 2 - Source
source2
Example 2 - Result
result2

In the first example you'll see that an incorrect background colour was applied, while in the second example the image is being enlarged and looking bad.

@joehoyle
Copy link
Member

@jgalea I'll look into example 1, however example 2 is expected. The background fill will not work here, so you will get a small image. I presume you are then forcing the width / height of the image which is upscaling it in the browser?

@jgalea
Copy link
Author

jgalea commented Nov 13, 2013

@joehoyle That's right, but I think it would be nicer to just give it a black background so at least you get the image size you requested. You can find some examples of the black background fill on http:https://www.wpmayor.com/wordpress-news/

Since we want a particular image size so all images are uniform, having a small image will break that design, and on the other hand browser upscaling makes them look bad, so the black background fill would be the best solution.

@joehoyle
Copy link
Member

@jgalea I think that's too much of an assumption across the board - for example, what if you have a white site and you want it to fall back to white if it can't auto pad? Perhaps we can have another option for what you are suggesting like:

array( 'background_fill' => 'auto,0000000000' )

or something like that.

@jgalea
Copy link
Author

jgalea commented Nov 13, 2013

@joehoyle Yes that crossed my mind while I was replying, I totally agree with having that fallback option.

@joehoyle
Copy link
Member

@jgalea I just pushed a fix, can you give it another go? Thanks!

@joehoyle
Copy link
Member

@jgalea I pushed a fix for this, can you give it a bash now? Thanks!

@jgalea
Copy link
Author

jgalea commented Nov 16, 2013

Definitely better now Joe, some of them don't work perfectly though, I noticed it mostly on images with white backgrounds, they get a grey bg, here are some examples:

example1
example2
example3

@joehoyle
Copy link
Member

Hi @jgalea

Can you attach the original version of the white image you are trying to background fill?

The resize issue doesn't seem to be related to this pull-request. As this isn't a support thread - could you open a new issue for that. I read through your notes here, but I am not 100% clear on what you want the last image to look like. Though it looks like maybe your first example (resize + crop) is only doing a crop? Again, without having the actual image + your code example, I am shooting in the dark so you be good to get that on the new ticket too.

@jgalea
Copy link
Author

jgalea commented Nov 18, 2013

Here;s the original image Joe, I'll open another ticket for the other issue.

thumbnail17

@willmot
Copy link
Member

willmot commented Nov 18, 2013

That image has a grey inner-shadow/border. That's the reason it get's a grey background-fill.

screen shot 2013-11-18 at 15 40 31

@jgalea
Copy link
Author

jgalea commented Nov 18, 2013

Right, I guess the others are the same then. I thought the class considered the total area rather than just the edges, but it's probably best to extend based on the edges. To eliminate such instances you'd have to make a condition that checks for such thin borders and excludes them when taking the decision of the background fill colour.

@willmot
Copy link
Member

willmot commented Nov 18, 2013

thought the class considered the total area rather than just the edges

It currently measures the colour of each corner pixel and if each corner is the same colour it fills with that.

@jgalea
Copy link
Author

jgalea commented Nov 18, 2013

Understood, I imagine you'd like to keep it nice and simple then, rather than complicate it further as I suggested.In that case this issue is probably resolved.

I think Joe's idea earlier would also be nice to have implemented:
#87 (comment)

@jgalea
Copy link
Author

jgalea commented Nov 28, 2013

Any plans to implement the background fill fallback?

joehoyle added a commit that referenced this pull request Dec 9, 2013
@joehoyle joehoyle merged commit a0db936 into master Dec 9, 2013
@joehoyle joehoyle deleted the background-fill-auto branch December 9, 2013 03:39
@joehoyle
Copy link
Member

joehoyle commented Dec 9, 2013

Merged, created #96 for the fallback color.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants