Updated: Comment #19

Problem/Motivation

The breakpoint module uses simple configuration to create multiple config entities - this means that breakpoints are not deployable and difficult to work with from a CMI perspective.

Proposed resolution

Remove simple config and replace with default config entities.

Remaining tasks

Review code

User interface changes

API changes

  • Remove many functions from breakpoint module related to automatic config entity creation.
  • Add getBreakpoints() and getBreakpointById() methods to BreakpointGroupInterface

#1937914: Special handling for setting the BreakpointGroup::breakpoints property during ConfigStorageController::importChange()
#2120875: Remove breakpoint and picture module from core

Original report by @xjm

From #1850158: Bugged assumption in ModuleTestBase::assertModuleConfig().

  • We renamed breakpoint.multipliers to breakpoint.settings.multipliers, but I couldn't find a single place it was actually used. The multipliers aren't used for the moment. Once there's a UI for breakpoint, the user will be able to alter the defaults.
  • Picture module supports multipliers, but it reads them from the breakpoints, so for the moment picture module only support the 1x multiplier.
  • We could remove the config for now, but the idea is to add a UI for it, see #1813126: Awesome breakpoints & gridbuilder UI and #1775302: Do a UX review of Breakpoint module .
Files: 
CommentFileSizeAuthor
#33 23-33-interdiff.txt664 bytesalexpott
#33 1851018.33.patch38.91 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 59,026 pass(es).
[ View ]
#9 i1851018-9.patch36.83 KBattiks
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch i1851018-9.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#4 i1851018-4.patch32.67 KBattiks
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#3 1851018.3.patch39.19 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 59,259 pass(es).
[ View ]

Comments

attiks’s picture

Status:Active» Closed (works as designed)

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?

attiks’s picture

Status:Closed (works as designed)» Active

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

attiks’s picture

Issue summary:View changes

Updated issue summary.

alexpott’s picture

Status:Active» Needs review
StatusFileSize
new39.19 KB
PASSED: [[SimpleTest]]: [MySQL] 59,259 pass(es).
[ View ]

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().

attiks’s picture

StatusFileSize
new32.67 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
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!

Status:Needs review» Needs work

The last submitted patch, i1851018-4.patch, failed testing.

attiks’s picture

Status:Needs work» Needs review

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

Status:Needs review» Needs work

The last submitted patch, i1851018-4.patch, failed testing.

attiks’s picture

grr, wrong patch, I'll reroll

attiks’s picture

Status:Needs work» Needs review
StatusFileSize
new36.83 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch i1851018-9.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Status:Needs review» Needs work

The last submitted patch, i1851018-9.patch, failed testing.

attiks’s picture

Status:Needs work» Needs review
StatusFileSize
new36.8 KB
PASSED: [[SimpleTest]]: [MySQL] 59,241 pass(es).
[ View ]

wrong order

heyrocker’s picture

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.

alexpott’s picture

@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.

attiks’s picture

#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

mdrummond’s picture

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.

beejeebus’s picture

i like the way this is going. the only nitpick i have is:

<?php
+  public $breakpoint_ids = array();
?>

i guess that should be $breakpointIds, as it's a class variable?

tim.plunkett’s picture

Also please make it protected.

alexpott’s picture

StatusFileSize
new37.94 KB
PASSED: [[SimpleTest]]: [MySQL] 59,860 pass(es).
[ View ]
new3.44 KB

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).

alexpott’s picture

Tagging

attiks’s picture

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

attiks’s picture

grr, cross post

attiks’s picture

Issue summary:View changes

Updated issue summary.

alexpott’s picture

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.

alexpott’s picture

StatusFileSize
new38.95 KB
PASSED: [[SimpleTest]]: [MySQL] 59,034 pass(es).
[ View ]
new750 bytes

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

attiks’s picture

#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?

attiks’s picture

Issue summary:View changes

Update issue summary

alexpott’s picture

#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.

mdrummond’s picture

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?

alexpott’s picture

23: 1851018.22.patch queued for re-testing.

xjm’s picture

Issue tags:+Default configuration
xjm’s picture

Issue tags:+beta blocker
alexpott’s picture

23: 1851018.22.patch queued for re-testing.

alexpott’s picture

StatusFileSize
new38.91 KB
PASSED: [[SimpleTest]]: [MySQL] 59,026 pass(es).
[ View ]
new664 bytes

Removed unnecessary commented out code :) thanks @beejeebus!

beejeebus’s picture

Status:Needs review» Reviewed & tested by the community

i think this is RTBC. the bot will set it back to needs work if the tests fail.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

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!

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.