Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Related Issues
#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
tobreakpoint.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 .
Comment | File | Size | Author |
---|---|---|---|
#33 | 23-33-interdiff.txt | 664 bytes | alexpott |
#33 | 1851018.33.patch | 38.91 KB | alexpott |
#9 | i1851018-9.patch | 36.83 KB | attiks |
#4 | i1851018-4.patch | 32.67 KB | attiks |
#3 | 1851018.3.patch | 39.19 KB | alexpott |
Comments
Comment #1
attiks CreditAttribution: attiks commentedYou'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?
Comment #2
attiks CreditAttribution: attiks commentedLeaving 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
Comment #2.0
attiks CreditAttribution: attiks commentedUpdated issue summary.
Comment #3
alexpottThe 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().
Comment #4
attiks CreditAttribution: attiks commentedIs this file part of this patch?
more changes to files not related to breakpoint
New patch removing those attached, patch is looking good, thanks!
Comment #6
attiks CreditAttribution: attiks commented#4: i1851018-4.patch queued for re-testing.
Comment #8
attiks CreditAttribution: attiks commentedgrr, wrong patch, I'll reroll
Comment #9
attiks CreditAttribution: attiks commentedComment #11
attiks CreditAttribution: attiks commentedwrong order
Comment #12
gddSomeone 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.
Comment #13
alexpott@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.
Comment #14
attiks CreditAttribution: attiks commented#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
Comment #15
RainbowArrayThe 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.
Comment #16
Anonymous (not verified) CreditAttribution: Anonymous commentedi like the way this is going. the only nitpick i have is:
i guess that should be $breakpointIds, as it's a class variable?
Comment #17
tim.plunkettAlso please make it protected.
Comment #18
alexpottPatch 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).
Comment #19
alexpottTagging
Comment #20
attiks CreditAttribution: attiks commented#16, #17 Those ids are read directly from the config file, do we use uppercase in yml?
Comment #21
attiks CreditAttribution: attiks commentedgrr, cross post
Comment #21.0
attiks CreditAttribution: attiks commentedUpdated issue summary.
Comment #22
alexpottAlso 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.
Comment #23
alexpottPatch that remoaves breakpoint.settings.yml as it is totally unused.
Comment #24
attiks CreditAttribution: attiks commented#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?
Comment #24.0
attiks CreditAttribution: attiks commentedUpdate issue summary
Comment #25
alexpott#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.
Comment #26
RainbowArrayIs 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?
Comment #27
alexpottAdding related issue #2132551: Picture module uses config keys with a dot
Comment #28
alexpottComment #29
alexpott23: 1851018.22.patch queued for re-testing.
Comment #30
xjmComment #31
xjmComment #32
alexpott23: 1851018.22.patch queued for re-testing.
Comment #33
alexpottRemoved unnecessary commented out code :) thanks @beejeebus!
Comment #34
Anonymous (not verified) CreditAttribution: Anonymous commentedi think this is RTBC. the bot will set it back to needs work if the tests fail.
Comment #35
webchickWow. 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!