Improve breakpoint configuration implementation.

You're right it isn't used for now, it was in the initial patch, but since the UI is still not implemented is isn't used. Once there's a UI people will be able to add/remove multipliers and they can be used by the picture module.

PS: The picture module already supports multipliers but the only way to add mulltipliers is by using code.

"per webchick, there's other code in this module that could be converted to CMI." Are there issues for this?

Leaving this open to keep track of this.

From IRC (thanks xjm):

alright, so, these specifics (that the default multipliers in the yaml file aren't used, and that the corresponding hook_help() text referring to the defaults is incorrect) should be documented in some issue, somewhere.

and it sounds like what's already there does need more test coverage if something broke and doesn't work again now, see #1851098: Breakpoint groups aren't enabled using the standard profile

Updated issue summary.

The attached patch removes all the magical config entity creating and just provides default config entities. It also fixes #1937914: Special handling for setting the BreakpointGroup::breakpoints property during ConfigStorageController::importChange().

diff --git a/core/lib/Drupal/Core/Config/Context/LanguageConfigContext.php b/core/lib/Drupal/Core/Config/Context/LanguageConfigContext.php

Is this file part of this patch?

diff --git a/core/modules/system/config/schema/system.data_types.schema.yml b/core/modules/system/config/schema/system.data_types.schema.yml

+++ b/core/modules/system/config/schema/system.data_types.schema.ymlundefined
@@ -53,6 +53,12 @@ text:
diff --git a/core/modules/system/config/schema/system.schema.yml b/core/modules/system/config/schema/system.schema.yml

more changes to files not related to breakpoint

New patch removing those attached, patch is looking good, thanks!

#4: i1851018-4.patch queued for re-testing.

grr, wrong patch, I'll reroll

wrong order

Someone else will have to comment on the breakpoint-specific aspects of the code, but I'm loving the direction of this patch. This is absolutely the way the default config should be handled.

@attiks thanks for sorting out the patch - working on two things at once sorry!

So I had not looked at the breakpoint module in depth and I'm wondering the utility of being able to define a breakpoint outside of a group of breakpoints? Ie... when would the same breakpoint be shared across two groups and why would you do that? And also what would happen if you edited a breakpoint because one of the important things would be how it relates to other breakpoints - and if it's going to be in more than one group that's going to get very complex.

We could simplify things a lot if the groups contained all the breakpoint information.

#13 Thanks for writing it in the first place ;-)

Regarding the re-use of breakpoints, that is a valid use case. You mostly start out with the breakpoints defined in your theme because those define where you're layout will be altered. But for your content you'll have multiple groups of breakpoints depending on the location of the content (main content, sidebar, header, ...).

For example an image in the sidebar will be scaled down in the sidebar, but the moment that your sidebar is rendered below the main content (mobile phone for example) the width might increase to 100%, will the width of another image inside the main area will still be scaled down.

If we don't share breakpoints between groups it will force site builders to write the same breakpoint over and over again, which isn't good for their moral.

If you change a breakpoint from one group, you mostly want this change to happen in other groups as well, for example if your sidebar width changes from 10em to 12em, you will want to adjust all breakpoints that react on that.

I hope it's clear, if not ... shoot

The primary use case for multiple breakpoint groups is for responsive images.

A particular site is likely to have multiple kinds of images. For example a hero image that generally takes up the full width of the main content area. Or an image that typically floats to the side of the main content, but might full the full column width at smaller screens. Or images that typically appear in sidebars.

For each of those situations, it's likely you'd want to start with the layout breakpoints, where the widths of various containers make large shifts. Those are most likely going to be the points where you are going to want to shift the size of the image source files for a particular type of image.

However, the best practice is for breakpoints to be content-based, not device-based. So that means that you might need additional breakpoints because your content doesn't work well in between the major layout breakpoints. For example, maybe your layout shifts at 25em (about 400px) and 43.75em (about 700px). However, if you have a banner image that covers the whole width of the layout, that gets to be a pretty big shift in the image size. The image size required to fill the full width at 700px is 300% bigger than what is required at 400px (1.75 x 1.75 = 3.0625). That's a lot of extra file to download. So maybe you decide to put in a couple smaller breakpoints at 31em and 37em. That means the image size you need is never going to be more than 50% larger than necessary, a much more reasonable ratio.

So to cover that use case, you'd want to clone your theme's primary layout breakpoints into a new breakpoint group, and you'd want to be able to add in those two additional minor breakpoints for that image type (and any other minor breakpoints you need between other major layout breakpoints). And yes, if the breakpoints shifted in your theme's layout breakpoints, you'd probably want that to cascade down to your banner breakpoint group (although you'd want to go in to make sure that all the picture mappings still make sense after the change).

Hope that helps to clarify why multiple breakpoint groups are useful.

Also please make it protected.

Patch attached makes it protected but leaves the name as breakpoint_ids as this looks better in the YAML file - I'm not sure we have standards for Config Entity property / YAML file keys yet - we have both camelCase and under_score in core :)

There are other public properties on the config entity but I've not changed them as that is beyond on the scope of the current patch (will update issue summary).

Tagging

#16, #17 Those ids are read directly from the config file, do we use uppercase in yml?

grr, cross post

Updated issue summary.

Also as the original issue summary says we're not using breakpoint.settings anywhere in core. I would prefer to remove this as we do not know of the requirements yet. At least with BreakpointGroup and Breakpoint we have partial uses in core. We're are severely lacking configurability but picture and toolbar do use the system.

Patch that remoaves breakpoint.settings.yml as it is totally unused.

#22 The settings are needed to support retina devices, is it a possibility to keep the schema so contrib can provide a UI for it?

Update issue summary

#24 if the settings are not used anywhere in core then this does not make sense. If the UI module in contrib needs these settings then it can provide them. We should not be creating completely useless config because this will make people think there is something that can be configured when it can not.

Is there any possibility we could still get UI for breakpoints in before release? It's going to be difficult for people to make much use of Picture without the ability to configure breakpoints.

Is the thought that there will be default breakpoints for the base themes, and then contrib themes will define their own breakpoints so there's a starting point without the UI?

Adding related issue #2132551: Picture module uses config keys with a dot

23: 1851018.22.patch queued for re-testing.

23: 1851018.22.patch queued for re-testing.

Removed unnecessary commented out code :) thanks @beejeebus!

Wow. This patch is AWESOME clean-up!! Thanks so much for all of this work, alexpott!!

I inspected the diff and this is removing a ton of one-off code from the dark ages before config entities and moving it instead to standard patterns that have developed since then. It also fixes some bugs along the way. Hooray! :)

Committed and pushed to 8.x. Thanks!