Add Resize and RelativeResize filters #37
Conversation
Ported these over from JmikolaImagineBundle, with some modifications to accomodate LiipImagineBundle's LoaderInterface. These can potentially replace the existing Thumbnail filter.
| return $filter->apply($image); | ||
| } | ||
|
|
||
| throw new InvalidArgumentException('Expected method/parameter pair, none given'); |
There was a problem hiding this comment.
this seems like a really weird way to configure the filter. What if the user sets several entries in the option array ?
another issue is that it introduces a limitation in how it can be used from Twig: hash keys cannot be set dynamically with variables so it is impossible to set the method according to a parameter (which could be useful when making it configurable). Why not using array('method' => $method, 'parameter' => $parameter) ?
There was a problem hiding this comment.
I didn't consider how filter parameters are defined for LiipImaigneBundle. For the original bundle, I have the following in my yaml configuration:
featured_logo:
type: relative_resize
options: { heighten: 78 }
dashboard_avatar:
type: relative_resize
options: { widen: 170 }Separate method and parameter keys there would have been superfluous. Only one method can specified at a time, and they each have a single parameter. On the subject of combining multiple operations, I think @lsmith told me that would be possible with a filter chain he had in this bundle.
There was a problem hiding this comment.
Ok, after reading through this bundle's documentation, I still don't see any examples of configuring these filters in Twig. I just see them setup in YAML and then referred to in Twig by name. Can you point me towards an example?
There was a problem hiding this comment.
ah, options come from the config, not from the Twig function. Missed this point.
There was a problem hiding this comment.
So we're all good here? :)
The foreach syntax was just some shorthand to avoid calling key() and current). I'm definitely not using it for iteration.
There was a problem hiding this comment.
yeah, but this is quite confusing
There was a problem hiding this comment.
@lsmith77: What's your opinion here. Do you want me to use key() and current() or perhaps make the configuration more verbose so that method and parameter must be specified by their own keys?
There was a problem hiding this comment.
hmm yeah .. i guess key() and current() would indeed be better. could you send another PR with that change .. but more importantly could you fix the README? i already added some docs there, but they are incomplete.
There was a problem hiding this comment.
Will do. I completely dropped the ball on porting over my README docs, sorry.
Add Resize and RelativeResize filters
ghost
mentioned this pull request
Ported these over from JmikolaImagineBundle, with some modifications to accomodate LiipImagineBundle's LoaderInterface. These can potentially replace the existing Thumbnail filter.
@lsmith: Please let me know if anything is missing. I think the DI configuration was all that this needed apart from the classes themselves.