Skip to content
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

Implement Imagine Grayscale filter #638

Merged
merged 6 commits into from
Jul 12, 2016
Merged

Implement Imagine Grayscale filter #638

merged 6 commits into from
Jul 12, 2016

Conversation

gregumo
Copy link
Contributor

@gregumo gregumo commented Oct 7, 2015

No description provided.

/**
* {@inheritdoc}
*/
public function load(ImageInterface $image, array $options = [])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR looks good, though you use [] which is not supported on php5.3, but we still supports this version, could you changes this to be BC?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests failed on travis.

@gregumo
Copy link
Contributor Author

gregumo commented Oct 7, 2015

The fix is done but Travis is still failing. I think it fail for another reason.

@lsmith77 lsmith77 added the State: Reviewing This item is being reviewed to determine if it should be accepted. label Oct 7, 2015
@lsmith77
Copy link
Contributor

can you also add something to the docs?

@lsmith77
Copy link
Contributor

lsmith77 commented Nov 1, 2015

please rebase on master to get the test fixed

@makasim
Copy link
Collaborator

makasim commented Jan 4, 2016

@gregumo Could you please do a rebase?

@gregumo
Copy link
Contributor Author

gregumo commented Jan 5, 2016

@lsmith77 The doc for Grayscale Filter has been added

@makasim I just made the rebase but Imagine Grayscale Filter results are not the same between imagine/imagine 0.6.1 and 0.6.3. And the class Imagine\Image\Palette\RGB that I used doesn't exist before 0.6.1. So I propose to upgrade imagine dependency to 0.6.3 minimum. I made the change in the composer.json file but I let you add a new release tag for Liip if you think it's necessary.

Sorry for my late reply :/

@makasim
Copy link
Collaborator

makasim commented Jan 5, 2016

The bundle support imagine 0.5 now. Should we drop it? Is it widely used?

@gregumo
Copy link
Contributor Author

gregumo commented Jan 6, 2016

If you ask me if Grayscale filter is widely used, I really don't know but I think this is a really interesting filter to add to LiipImagineBundle.

For Imagine 0.5.0, my test can't work because the class 'Imagine\Image\Palette\RGB' that I load has been created in Imagine 0.6.0. So the continuous integration test with the composer tag --prefer-lowest failed if I don't change imagine dependency tags in composer.json.

Then I have a color issue between 0.6.1 and 0.6.3:

  • for Imagine 0.6.0 and 0.6.1, the color returned after my Grayscale filter test is #145af0.
  • for Imagine 0.6.3, the color returned is #565656.

This is why I propose to upgrade Imagine dependency to ^0.6.3

So I think we should drop Imagine 0.5 for 0.6.3 and upper.

@makasim
Copy link
Collaborator

makasim commented Jan 6, 2016

@lsmith77 @havvg wdyt? Can we drop support of 0.5.x imagine?

@havvg
Copy link
Contributor

havvg commented Jan 6, 2016

I think so, just for semantic versioning: This is a BC and would increase the major version :)

@lenybernard
Copy link

Hello,
👍
When do you plan to merge this ?

@gregumo
Copy link
Contributor Author

gregumo commented Jun 13, 2016

Hi ! Do you plan to merge this PR ?

@lsmith77
Copy link
Contributor

I think we can bump the imagine dependency to 0.6.3 without a major new version

@lsmith77 lsmith77 merged commit bd98616 into liip:master Jul 12, 2016
@lsmith77 lsmith77 removed the State: Reviewing This item is being reviewed to determine if it should be accepted. label Jul 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants