merge presets with user-provided defaults

[sdumetz]

Currently PictureTag::Parsers::Preset is merging all presets with DEFAULT_PRESET, it could merge the Gem's defaults with Site.data.picture.presets.default. Allowing users to have smaller picture.yml files by providing custom site-wide defaults.

The change doesn't break any tests but would of course be a breaking change that require a major version bump. However this behavior feels quite natural to me : many tools have a way to supply user defaults that are inherited by other presets.

It might be possible to make it non-breaking by creating a new section in picture.yml, thus accessing the site-wide-defaults with PictureTag.site.data.dig("picture","defaults"). I'm open to take whatever path the maintainer or other users feel preferable.

Additionally, I created some basic tests which are probably crap, sorry about this, I wasn't able to do any better.

[rbuchberger]

I like this idea! Thanks for the PR! Don't mind the CI fail - It's been broken for a little while and I haven't gotten around to fixing it.

I think we can do this without bumping the major version, by adding a setting and disabling it by default. Then whenever we do release a new major version, we can enable it by default.

I had another idea to allow preset composition by specifying an inherit option: something like:

preset_a: 
  key: value
  (...)

preset_b:
  inherit: preset_a
  (...)

Could even inherit multiple presets by giving an array, with later ones overriding earlier ones. I had originally planned this for version 2, but ended up deciding it added more complexity than it saved. Looking at your project's config, I'm thinking that might not have been the right decision!

[sdumetz]

I did a prototype with an include key that supports arrays. The hash merge uses Jekyll::Utils.deep_merge_hashes but few changes are necessary to make it use PictureTag::Parsers::Configuration if you prefer.

It merges hashes but not arrays so widths are not "added together", which is probably good, but should be documented.

Let me know if you think this implementation is OK before I try to write the doc for it.

[rbuchberger]

Hi!

Firstly, I'm really sorry for taking so long to get to this. My life's been crazy, and I haven't had time to spend on things that aren't my job or family.

Secondly, I don't think this approach is right, for one reason that didn't come to me until recently: It's already a feature of YML. You can accomplish this with anchors and aliases. Heres' the concept applied to my config file, which appears to work perfectly:

media_queries:
  full_width: 'min-width: 901px'
  tablet: 'min-width: 601px'
  mobile: 'max-width: 600px'

presets:
  base: &base
    formats: [webp, original]
    sizes:
      full_width: 862px
    size: calc(100vw - 18px)

  default:
    <<: *base
    widths: [500, 600, 700, 800, 900, 1000, 1200, 1600]
    link_source: true
    data_attributes: true
    attributes:
      a: 'data-no-swup="true"'

  project_showcase:
    <<: *base
    widths: [700, 864, 900, 1296, 1600, 1728]
    format_quality:
      webp: 100
    media_widths:
      mobile: [400, 500, 600, 700, 800]
    attributes:
      picture: 'class="showcase-image"'

  circle_portrait:
    <<: *base
    base_width: 300
    pixel_ratios: [1, 1.5, 2]
    quality: 90
    attributes:
      parent: 'class="circle-portrait"'

  blog:
    <<: *base
    widths: [500, 600, 700, 800, 900, 1000, 1200, 1600]
    nomarkdown: false
    link_source: true

Would that approach meet your needs? It's definitely not intuitive; we'd want to document it clearly.

[sdumetz]

Oh, I never heard of this feature.

I tested it and it works well. While it's a little more verbose I agree with you it's a way better solution with 100% opt-in and explicit inheritance.

If I find time to add some doc about this someday I'll open another request.

[rbuchberger]

Thanks a ton! if you don't find the time i can take care of it.