Create default responsive image styles

+++ b/core/modules/breakpoint/breakpoint.breakpoints.yml
@@ -0,0 +1,7 @@
+breakpoint.all.viewport_sizing:
+  label: Viewport Sizing
+  mediaQuery: ''
+  weight: 0
+  multipliers:
+    - 1x
+  group: All Viewports

this doesn't look right, see http://cgit.drupalcode.org/drupal/tree/core/modules/toolbar/toolbar.brea...

I moved the new 'Viewport Sizing' breakpoint from the breakpoint module to the responsive_image module because the breakpoint wouldn't make much sense without responsive images. I renamed the group to responsive_image.all because of it. Then I fixed the 'group' property for the breakpoints. Tests should be green.

(BTW: your patch in #10 was empty so I had to reconstruct it using the patch in #8 and the interdiff in #10 ;-) )

#13 Valid point regarding the name, we need to figure out a way to fix this

+++ b/core/modules/responsive_image/config/install/responsive_image.styles.narrow.yml
@@ -0,0 +1,24 @@
+uuid: 8eb79e19-da57-4bd3-8304-4e0b5a147276

+++ b/core/modules/responsive_image/config/install/responsive_image.styles.wide.yml
@@ -0,0 +1,26 @@
+uuid: 5cffd3ef-5656-4446-b200-b771d8076568

I recall that we don't include UUIDs in default config

Patch looks good to me and makes it easier for people to use sizes

+++ b/core/modules/responsive_image/config/install/responsive_image.styles.narrow.yml
@@ -0,0 +1,24 @@
+    - image.style.large
+    - image.style.medium

+++ b/core/modules/responsive_image/config/install/responsive_image.styles.wide.yml
@@ -0,0 +1,26 @@
+    - image.style.large
+    - image.style.medium

These image styles might not exist. Someone could delete them before installing responsive image. Not sure what to do.

We can always provide our own image styles to avoid it?

+++ b/core/modules/responsive_image/config/install/image.style.max_1120x1120.yml
@@ -0,0 +1,11 @@
+      upscale: true

+++ b/core/modules/responsive_image/config/install/image.style.max_1720x1720.yml
@@ -0,0 +1,11 @@
+      upscale: true

+++ b/core/modules/responsive_image/config/install/image.style.max_215x215.yml
@@ -0,0 +1,11 @@
+      upscale: true

+++ b/core/modules/responsive_image/config/install/image.style.max_2160x2160.yml
@@ -0,0 +1,11 @@
+      upscale: true

Why the need to upscale? I think it is safer to never upscale.

##2 True, but upscaling means PHP will upscale and lower the image quality, while creating a bigger file, that will get downloaded by the browser.

If we don't upscale a themer can still decide to add width: 100% risking the upscaling effect.

Assuming bot will be happy, back to RTBC

Had a call today with @mdrummond and @Bojhan where this patch came up.

When demoing the patch, Marc showed the list of image styles approximately quadrupling in size, with numerous very similarly-named options (Max 325x325, Max 400x400, Max 600x600, etc. times about 12 or 16).

The problem is:

a) All of those additional image styles selections are going to show up everywhere that image styles are used in configuration: display modes, views, etc., not just encapsulated within responsive image's configuration.
b) There is no clear signal to end users why all of those sizes are predefined. In talking to Marc it's basically:

Narrow: small, medium, large, plus retina for each.
Wide: small, medium, large, plus retina for each.

However, this is very hard to tell from just raw pixel sizes.

c) And even if we were to rename these with more "human-readable" names, the fact is that each individual site needs to decide for itself what form factors/viewport sizes it wants to support. Core currently ships with three image styles: Thumbnail, Medium, and Large with some opinions on what those sizes should be. Many sites override these, and for ~three image styles that's basically fine. But if you have to go in and edit an additional ~12 image styles, each with almost identical names, that will get both very confusing and very irritating very fast.

So Marc and I discussed a compromise which is still providing additional image styles, still naming them "Max XXX" so we don't need to come up with nonsensical English words that happen to flow alphabetically, but limit it to just 4 sizes: 325x325, 600x600, 1300x1300, 2600x2600. This allows for a Narrow/Wide regular/retina, and individual sites can extrapolate from there.

#32 Looks good and agree on the naming

So I'm wondering if we can put this all in the standard profile - so if you install in minimal it stays minimal. One thing we need to do if to add an enforced dependency on responsive image module. See the node type provided by the book module for an example. It also needs to go in a new folder in standard config/optional... This should also result in this patch not changing tests. Well, I think that StandardTest should assert the new optional config does not exist until the responsive image module is installed.

I'll have a try later today.

If I understand correctly the attached patch is correct, but when I enable the responsive_image module, the optional config isn't loaded, I tried with and without adding the enforced.

PS: If I add responsive_image as a dependency for the standard profile, everything works.

According to #2453919: all installed modules' config/optional directories are searched for any configuration entities of the new type

So this doesn't work for profiles?

It wasn't working for optional config in install profiles, fixed it

Fix added for failing tests

Tests added to StandardTest to make sure the image styles are not installed by default.

Tests extended to make sure the optional styles are installed once responsive_image is enabled.

Back tot Alex to make sure this is good

1. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
   @@ -147,6 +147,8 @@ public function installDefaultConfig($type, $name) {
   +      // Search the install profile's optional configuration too.
   +      $this->installOptionalConfig(NULL, [$type => $name]);
   

   I need to review exactly what this is going to do. And it needs further testing... but the idea of having optional config in install profiles is sound. And perhaps deserves it's own issue.

2. +++ b/core/profiles/standard/config/optional/responsive_image.styles.narrow.yml
   @@ -0,0 +1,28 @@
   +  module:
   +    - responsive_image
   +  enforced:
   +    module:
   +      - responsive_image
   
   +++ b/core/profiles/standard/config/optional/responsive_image.styles.wide.yml
   @@ -0,0 +1,30 @@
   +  module:
   +    - responsive_image
   +  enforced:
   +    module:
   +      - responsive_image
   

   This is not necessary the dependency is implied from the name of the configuration object.

1. There's a test added to make sure the optional config gets read, not sure what else I can do. Unless you mean you want to have the tests somewhere else as well, so they don't depend on the install profile and/or responsive_image module.

2. Removed the lines

The extra call to installDefaultConfig() doesn't make sense since this means that we have to loop through a lot of stuff twice. We can fix this in a different way. What I mean by tests is that this ability to install default configuration from profiles should be part of the configuration tests not hidden in the StandardTest where it's testing responsive_image.

Ah not so simple - fortunately we have tests... patch adds a test around the new ability of profiles to provide completely new optional configuration.

+++ b/core/modules/responsive_image/src/Tests/ResponsiveImageAdminUITest.php
@@ -44,7 +44,7 @@ public function testResponsiveImageAdmin() {
     // Add a new responsive image style, our breakpoint set should be selected.

This comment is no longer true. I think we should be a little more specific than "our"

Looking good, thanks all

Once committed I'll updated the CR at https://www.drupal.org/node/2453919 or do we need to create a new one for the profile optional config?

I don't think that the config change needs a CR - it's an addition.

#62 Anything else we need to do? Can you commit this, or is it better to ask @webchick?

@attiks I can not commit this - I've worked on it.

So I went to commit this, but it's failing on DrupalCI so I queued a re-test an hour and a bit ago and it's still re-queueing. Will check again tonight.

Last test is green, https://www.drupal.org/pift-ci-job/16985

Oh, crap, I am so sorry! I thought I committed this weeks ago.

Committed and pushed to 8.0.x. Thanks!