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

Add support for default JPEG quality with a default_filter_settings config #909

Open
wants to merge 1 commit into
base: 1.0
from

Conversation

Projects
None yet
4 participants
@peterkeung

peterkeung commented Apr 15, 2017

Q A
Branch? 1.0
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? no
Fixed tickets n/a
License MIT
Doc PR not yet

As a CMS implementer, I don't want to have to define the image quality for every image alias / filter set. It's an exception that I want a different image quality, and my team sometimes forgets to set the image quality. Thus, it greatly improves dev UX to have an editable default image quality setting.

I understand this is more "proof of concept" than "ready to go" as it is missing docs and tests, and should probably be expanded to include more settings.

@robfrawley

This comment has been minimized.

Show comment
Hide comment
@robfrawley

robfrawley Apr 16, 2017

Collaborator

This looks like a good idea with positive DX improvements, but I don't want to add this only to deprecate it for the 2.x branch, where I've been working on a way to implement a liip_imagine.defaults config array that mirrors the normal configuration hierarchy, and allows one to provide defaults in a more global manner.

Let me see if I can finalize the structure of that component for 2.x and then we can add a one-off at the same index. To clarify, all we'll need to change from this PR is the position of the default quality values in the configuration hierarchy so it provides a seamless experience during an upgrade to 2.x.

Obviously, tests and docs are required too, but you're already aware of that. ;-) I'll give you a heads up in the coming days on where the config item should be placed. Adding some defaults for PNG compression and a few others may be nice too for the 1.x branch.

Collaborator

robfrawley commented Apr 16, 2017

This looks like a good idea with positive DX improvements, but I don't want to add this only to deprecate it for the 2.x branch, where I've been working on a way to implement a liip_imagine.defaults config array that mirrors the normal configuration hierarchy, and allows one to provide defaults in a more global manner.

Let me see if I can finalize the structure of that component for 2.x and then we can add a one-off at the same index. To clarify, all we'll need to change from this PR is the position of the default quality values in the configuration hierarchy so it provides a seamless experience during an upgrade to 2.x.

Obviously, tests and docs are required too, but you're already aware of that. ;-) I'll give you a heads up in the coming days on where the config item should be placed. Adding some defaults for PNG compression and a few others may be nice too for the 1.x branch.

@peterkeung

This comment has been minimized.

Show comment
Hide comment
@peterkeung

peterkeung Apr 16, 2017

Thanks! Will stay tuned.

peterkeung commented Apr 16, 2017

Thanks! Will stay tuned.

@antoligy

This comment has been minimized.

Show comment
Hide comment
@antoligy

antoligy Apr 16, 2017

Collaborator

Hey @peterkeung, nice to see more contributions from the eZ Community!

I've gotta admit, I've had the same frustration with defining image variations for a long time, and going through a lot of the image aliases across different projects I contribute to, they're nearly all duplicate configuration parameters! Not good for maintainability!

I would like to suggest that for 2.0, instead of leaving this with only defining defaults, making configuration entirely composable and including a "default" variation which can set configuration for every implementing variation.
For completely "different" configurations, they wouldn't necessarily need to be composed from default and could define a different base if needed. I guess this is taking the existing reference property, only instead of relying on a pregenerated image variation, we allow for anything to be overridden.

e.g. a common use-case for me:

image_variations:

    default:
        quality_factor:
            jpeg: 80
        post_processors:
            mozjpeg: {}
            pngquant: {}

   large:
       parent: default
       quality_factor:
           jpeg: 90

    thumbnail:
        parent: default
        filters: [{name: thumbnail, params: {size: [340, 250], mode: outbound}}]

What do you think?

Collaborator

antoligy commented Apr 16, 2017

Hey @peterkeung, nice to see more contributions from the eZ Community!

I've gotta admit, I've had the same frustration with defining image variations for a long time, and going through a lot of the image aliases across different projects I contribute to, they're nearly all duplicate configuration parameters! Not good for maintainability!

I would like to suggest that for 2.0, instead of leaving this with only defining defaults, making configuration entirely composable and including a "default" variation which can set configuration for every implementing variation.
For completely "different" configurations, they wouldn't necessarily need to be composed from default and could define a different base if needed. I guess this is taking the existing reference property, only instead of relying on a pregenerated image variation, we allow for anything to be overridden.

e.g. a common use-case for me:

image_variations:

    default:
        quality_factor:
            jpeg: 80
        post_processors:
            mozjpeg: {}
            pngquant: {}

   large:
       parent: default
       quality_factor:
           jpeg: 90

    thumbnail:
        parent: default
        filters: [{name: thumbnail, params: {size: [340, 250], mode: outbound}}]

What do you think?

@robfrawley

This comment has been minimized.

Show comment
Hide comment
@robfrawley

robfrawley Apr 16, 2017

Collaborator

@antoligy That is exactly what I'm trying to implement, actually! I stole the idea from the new Symfony service defaults key.

Collaborator

robfrawley commented Apr 16, 2017

@antoligy That is exactly what I'm trying to implement, actually! I stole the idea from the new Symfony service defaults key.

@andrerom

This comment has been minimized.

Show comment
Hide comment
@andrerom

andrerom Nov 14, 2017

@robfrawley Any news on this?

andrerom commented Nov 14, 2017

@robfrawley Any news on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment