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

added support for many filter transformations in one filter set (style), fixes GH-1 #11

Merged
merged 1 commit into from Oct 19, 2011
Merged

Conversation

ghost
Copy link

@ghost ghost commented Oct 13, 2011

Now the following is possible:

liip_imagine:
    styles:
        my_thumb:
            quality: 75
            filters:
                thumbnail: { size: [120, 90], mode: outbound }
                watermark: { watermark: '/path/to/image.png' }
                other_filter: {}

fixes GH-1

@LouTerrailloune
Copy link
Contributor

As you renamed the filters key to styles, I think we should by consistent and also rename the twig filter to apply_style (will fix #9).

@ghost
Copy link
Author

ghost commented Oct 13, 2011

As you renamed the filters key to styles, I think we should by consistent and also rename the twig filter to apply_style (will fix #9).

I am not sure. Having similar code base will allow others to easily migrate to LiipImagineBundle. If this should be done, it could also be merged via separate PR to keep this PR manageable.

@lsmith77 what do you think?

@LouTerrailloune
Copy link
Contributor

Switching from avalanche to liip will involve many changes anyway (config
format, interface namespaces)

@lsmith77
Copy link
Contributor

yeah .. i don't think we should break compat for the hell of it .. but things seem to be diverging pretty quick ..

@lsmith77
Copy link
Contributor

btw .. at the latest i will dive into all your discussions and proposals over the weekend .. but was at a conference the past two days and now i have to get a bit of paid work done :)

@lsmith77 lsmith77 mentioned this pull request Oct 14, 2011
@lsmith77
Copy link
Contributor

i havent tested this feature yet. i am not quite sold on the term "styles".

@lsmith77
Copy link
Contributor

also i think we should prefix the twig filters with "imagine" as noted by @stof in #9

@LouTerrailloune
Copy link
Contributor

I don't think 'styles' is the better word for this. We should stick to 'filters' or maybe 'filter_sets' to give a hint that many imageine filters will be applied. Of cource we rename the twig filter to 'apply_filter_set'.

@ghost
Copy link
Author

ghost commented Oct 19, 2011

Can we agree on something regarding the filter name? Just liip_imagine ? apply_liip_imagine_style ? Other?

Should this be processed in this PR? I think this could be a separate change.

Any other changes are required to merge this?

@lsmith77
Copy link
Contributor

like i said .. i am not yet convinced that it makes sense to use the term style over filter. furthermore .. your PR needs to be synced to all the changes in master before it can be merged.

finally, i haven't tested it out. and ideally i would actually like to start asking for unit tests for all new features. then again i don't want to drop that on you as the first one. if you feel you have the time and skill to create tests, it would be very good.

@ghost
Copy link
Author

ghost commented Oct 19, 2011

@lsmith77 PR is updated to match recent changes and style was changed to filter_sets. Let me know if now it can be merged.

Regarding the tests. I can not contribute to tests which currently don't pass. See #22 and #23 first.

@lsmith77
Copy link
Contributor

ooops .. sorry for those .. thx for the PR and i fixed the other issue

```

You've now defined a filter called `my_thumb` that performs a thumbnail transformation.
You've now defined a style called `my_thumb` that performs a thumbnail transformation.
Copy link
Contributor

Choose a reason for hiding this comment

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

this change needs to be reverted to the old terminology

@lsmith77
Copy link
Contributor

ok reviewed the PR .. only a few minor issues left.

as for tests ... it would be awesome if you could provide a unit test for the filter manager, but i will merge it even without. but like i said .. its important that we expand the unit tests before we have too many features and end up breaking stuff left and right.

@ghost
Copy link
Author

ghost commented Oct 19, 2011

Fixed & squashed into one commit.

lsmith77 added a commit that referenced this pull request Oct 19, 2011
added support for many filter transformations in one filter set (style), fixes GH-1
@lsmith77 lsmith77 merged commit c8d28cd into liip:master Oct 19, 2011
@lenar
Copy link
Contributor

lenar commented Dec 1, 2011

Does this watermark filter the example shows actually exist somewhere? ... or is it just an example of future possibilities?

@lsmith77
Copy link
Contributor

lsmith77 commented Dec 1, 2011

future possibilities. if the filter is in imagine .. it should be trivial to add it though

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.

Add support for several filters in one filter set
3 participants