Spin-off from #1734642: Move breakpoint module into core. That issue is working on a UI for configuring breakpoints. This issue is just the ability for themes to identify their own breakpoints, at this time, presumed to be static (i.e., corresponding to the media queries within the their CSS files).

The benefit of this is to allow #1775530: Move picture into core to proceed independently of #1734642: Move breakpoint module into core.

Files: 
CommentFileSizeAuthor
#16 i1775774-breakpoints-16.patch1 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 41,227 pass(es).
[ View ]
#13 interdiff.txt984 bytesattiks
#13 i1775774-breakpoints-13.patch3.84 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 41,219 pass(es).
[ View ]
#11 interdiff.txt642 bytesattiks
#11 i1775774-breakpoints-11.patch3.92 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 41,218 pass(es).
[ View ]
#7 interdiff.txt2.79 KBattiks
#7 i1775774-breakpoints-7.patch3.89 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 41,183 pass(es).
[ View ]
#5 interdiff.txt2.25 KBattiks
#5 i1775774-breakpoints-5.patch3.84 KBattiks
FAILED: [[SimpleTest]]: [MySQL] 41,185 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#4 interdiff.txt2.25 KBattiks
#4 i1775774-breakpoints-4.patch3.84 KBattiks
FAILED: [[SimpleTest]]: [MySQL] 41,184 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#1 breakpoints.patch3.49 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 41,112 pass(es).
[ View ]

Comments

effulgentsia’s picture

Status:Active» Needs review
Issue tags:+mobile, +Responsive Design
StatusFileSize
new3.49 KB
PASSED: [[SimpleTest]]: [MySQL] 41,112 pass(es).
[ View ]

This patch uses the theme's .info file for specifying breakpoints, since that's where stylesheets are specified. It adds a system_breakpoint_list() function that mimics system_region_list().

In #1734642-8: Move breakpoint module into core, there's discussion about this belonging in config rather than .info, and I think that's a good point for when a UI is added, but in this issue, let's keep breakpoint definition in the same place as stylesheet definition.

Also, it's possible that the Layouts initiative will decouple page-level layout building from themes, and if/when that happens, we'll want to tie breakpoint definitions to layouts rather than to themes, but since as of now, themes handle their own layout, they must be the ones to define their breakpoints. We can adjust later when that changes.

attiks’s picture

Status:Needs review» Reviewed & tested by the community

Patch applies cleanly and all breakpoints correspondent to what's defined inside the css.

I adapted breakpoints module to support this new syntax, mainly to see if it works (and it does ;-)), but there's one thing left: the picture module uses 'breakpoint groups' to do his work, so we at least need a basic API to handle that.

I created a new issue to split the module into two parts: #1776236: Split breakpoints into API and UI module

webchick’s picture

Assigned:Unassigned» Dries

Hm. Dries will probably want to have a look at this. He has never been a fan over overloading .info files.

attiks’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new3.84 KB
FAILED: [[SimpleTest]]: [MySQL] 41,184 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new2.25 KB

Alternative patch as proof-of-concept using yml files

attiks’s picture

StatusFileSize
new3.84 KB
FAILED: [[SimpleTest]]: [MySQL] 41,185 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new2.25 KB

using $theme_key.breakpoints.yml

Status:Needs review» Needs work

The last submitted patch, i1775774-breakpoints-5.patch, failed testing.

attiks’s picture

Status:Needs work» Needs review
StatusFileSize
new3.89 KB
PASSED: [[SimpleTest]]: [MySQL] 41,183 pass(es).
[ View ]
new2.79 KB

I had to add config_install_default_config('theme', 'seven'); to the test otherwise the config wasn't loaded.

effulgentsia’s picture

#7 could work once we figure out how to remove the config_install_default_config('theme', 'seven'); from the test. Will it work without that if we test the active theme of the test (Bartik? Stark?)?

But I'd like to hear back from Dries on whether config is preferable to .info. My reasoning in #1 still stands: at this time, I think this info belongs in the same place as where stylesheets are added, and moving it to config should happen as part of making it UI configurable. But I'm okay being overruled on that if Dries or others feel strongly against adding new stuff to .info.

aspilicious’s picture

Status:Needs review» Needs work
+++ b/core/themes/bartik/config/bartik.breakpoints.ymlundefined
@@ -0,0 +1,3 @@
\ No newline at end of file

Add newline at the end of your config files

attiks’s picture

Status:Needs work» Needs review

Alex, I uploaded the patches because I heard the same as #3, I'll try the tests with other themes to see what's going on.

Personally the more I work with config, the more I like it.

attiks’s picture

StatusFileSize
new3.92 KB
PASSED: [[SimpleTest]]: [MySQL] 41,218 pass(es).
[ View ]
new642 bytes

It looks like the config ins't loaded by the test framework, I added theme_enable(array('seven')) to force it.

aspilicious’s picture

Status:Needs review» Needs work
+++ b/core/themes/bartik/config/bartik.breakpoints.ymlundefined
@@ -0,0 +1,3 @@
+wide: 'all and (min-width: 851px)'
\ No newline at end of file

Still no newlines at the end of the config files

attiks’s picture

Status:Needs work» Needs review
StatusFileSize
new3.84 KB
PASSED: [[SimpleTest]]: [MySQL] 41,219 pass(es).
[ View ]
new984 bytes
moshe weitzman’s picture

Status:Needs review» Reviewed & tested by the community

Personally, I could go with either .info or .yml. I think extensions should probably all be yml and we should ditch .info but thats for another issue. Anyway, lets let committers decide between effulgentsia's patch and attik's latest.

For me, the Test isn't worth the time it takes to run. It sets up a whole Drupal just to show that we can do a $config->get() [already tested elsewhere] and return an item out of its array. If this were a pure unit test of system_breakpoint_list(), then it would be very fast (but still pointless!). No idea how feasible a unit test is for this in today's simpletest.

sun’s picture

Status:Reviewed & tested by the community» Needs review

Overall, the config file approach makes much more sense to me for this use-case.

In #1712250: Convert theme settings to configuration system and some related issues, we're considering to move some of the theme .info stuff into a [theme].settings configuration object. A [theme].breakpoints would nicely complement that.

So +1 for using config. :)

On the patch:

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/BreakpointsTest.php
@@ -0,0 +1,36 @@
+ * Definition of Drupal\system\Tests\Common\BreakpointsTest.

Can we move this test into Drupal\system\Tests\Theme ?

+++ b/core/modules/system/system.module
@@ -3126,6 +3126,27 @@ function system_region_list($theme_key, $show = REGIONS_ALL) {
+function system_breakpoint_list($theme_key) {

I don't really understand why this function is part of system.module -- the configuration clearly seems to belong to a particular theme (not system module), so why don't we put it into theme.inc as

theme_get_breakpoints()

?

What do you think?

effulgentsia’s picture

StatusFileSize
new1 KB
PASSED: [[SimpleTest]]: [MySQL] 41,227 pass(es).
[ View ]

I don't really understand why this function is part of system.module

If we're using config, then I don't think we need a wrapper function at all, or, per #14, a test. So here's just the config files and that's it.

webchick’s picture

Assigned:Dries» Unassigned

Nice. :) Unassigning from Dries as this makes this a lot less controversial.

moshe weitzman’s picture

Status:Needs review» Reviewed & tested by the community

Nice one.

attiks’s picture

Quick question: we expect module builders to know that there might be a config called $theme_key.breakpoints? Is a wrapper function not easier for developers?

aspilicious’s picture

They will be used to the config system. So I don't see a reason why it should be bad for dx.

attiks’s picture

With the code in #13 the breakpoints module could just call system_breakpoint_list($theme_key); to get a possible list of defined breakpoints by the theme, without a wrapper function I'll need to do something like

<?php
$theme_breakpoints
= array();
$config = config($theme_key . '.breakpoints');
if (
$config) {
  
$theme_breakpoints = $config->get();
}
?>

Meaning I will provide my own wrapper function, as well as probably any other module wanting to use this, hence the question.

aspilicious’s picture

This is the case for a lot of other config conversions. We can't write a wrapper for all of them. But yeah I can see contrib code dealing with breakpoints write a wrapper for it.

attiks’s picture

@aspilicious true, so RTBC for me as well

effulgentsia’s picture

Re #21: I'm pretty sure that config($theme_key . '.breakpoints')->get() returns an empty array if the config object doesn't exist, so the only reasons I can think of to create a wrapper function for this one line statement are:
- If we want to default $theme_key to the current theme.
- If we don't want calling code to be coupled to the name of the config file, or want to allow for future logic that does something in addition to just getting the config data.

Perhaps once we start using this, we'll see value in the above, and can add a wrapping function at that time.

catch’s picture

Does theme config actually get copied to the config directory when themes are enabled? My assumption would be that it doesn't yet. That notwithstanding I'm also much happier with this in config than .info.

attiks’s picture

I just checked, it doesn't get copied but it is accessible.

catch’s picture

Status:Reviewed & tested by the community» Needs review

If it doesn't get copied, then it won't be available to config() no? If it's not I think we need to add that here, although not sure if that would introduce a dependency on #1067408: Themes do not have an installation status then?

attiks’s picture

Status:Needs review» Reviewed & tested by the community

There's something strange going on, I now have a config directory called "Array" and it contains bartik.breakpoints.yml

I did a full clean install with the patch from #16 and now the config contains bartik.breakpoints.yml, but not seven.breakpoints.yml.

I changed the admin theme to bartik, disabled seven, enabled seven and now seven.breakpoints.yml is also there.

So only problem is that the installer doesn't copy seven.breakpoints.yml if that needs to be fixed in this issue change to status, for now back to RTBC.

catch’s picture

Status:Reviewed & tested by the community» Needs work

OK so standard_install() doesn't enable seven:

http://api.drupal.org/api/drupal/core%21profiles%21standard%21standard.i...

We should at least change that here I think.

attiks’s picture

Is there're a reason why it is enabled like this:

<?php
 
// Enable the admin theme.
 
db_update('system')
    ->
fields(array('status' => 1))
    ->
condition('type', 'theme')
    ->
condition('name', 'seven')
    ->
execute();
?>

If not we can just do

<?php
theme_enable
(array('seven'));
?>
effulgentsia’s picture

attiks’s picture

Status:Needs work» Reviewed & tested by the community
Gábor Hojtsy’s picture

An interesting related problem is declaring layouts from themes: #1787846: Themes should declare their layouts. Do you agree that CMI .yml files would be appropriate for that use case as well or something else should be used. This approach inspired me to propose that there. It would such if things use different technologies. Eg. regions are in the .info file, breakpoints are in .yml and then layouts would be yet another discovery system and/or file format. Feedback welcome there!

RobW’s picture

I think most anything that you're able to alter through a UI fits well with config. In my view, .Info files generally contain hard and final information that can't be changed without breaking the theme itself (like regions, required css and js files, etc.).

Gábor Hojtsy’s picture

@RobW: as per the intent of this issue, these breakpoints are just a static copy-paste from the breakpoints already defind in CSS to inform the rest of the system.

attiks’s picture

Can this get committed or are there still some problems left?

webchick’s picture

We're over thresholds atm so I unfortunately can't commit any feature patches. :( Help with smashing critical and major bugs would be greatly appreciated.

hass’s picture

~6 clicks. Apply patch > Commit > Push to remote > Password > Ok. Takes me half a minute. That's why nothing get's forward with core patches? Please grant me commit permission, I can help.

RobW’s picture

Hass, I think the threshold Webchick is talking about is the number of critical bugs in Drupal 8. If it's over a certain amount, no new features will be committed until the bugs are fixed. So in order to get this committed more quickly you would have to help fix the current Drupal 8 critical bugs.

See the "issue thresholds" section on drupal.org/node/1201874.

hass’s picture

The wrong way is "wait until D8 is fixed" to get a properly working D7 core. Than D8 will be released later - who cares. We cannot use unready banana D7 and this is more important than future releases. We are in the situation that we are waiting e.g. a shiny SSL patch #961508: Dual http/https session cookies interfere with drupal_valid_token() or #1067120: Missing hook_taxonomy_term_view() or hook_entity_view() when viewing a term for D8 and this hold back to fix a totally broken D7 core. Great! And then we start with D9 and miss D7 and D8 again.

I have a very good understanding about regressions, but what we are doing here is plain wrong. We make D7 users waiting for over-designed patches for a future core version (that may also never get ready) and block them from implementing current (un-)stable D7 versions. Something with the focus is really wrong.

attiks’s picture

@hass I understand your problem, but polluting this issue isn't going to solve much, I think it's better to create a separate issue to discuss this.

webchick’s picture

Actually, even better is to just fix bugs, then this becomes a non-issue. :)

hass’s picture

There are D7 real life bugfixes in queue, 2+ years RTBC, still no commit.

catch’s picture

@hass, which of the 7 issues currently RTBC for 7.x has been there for 2 years?

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Yeah, that sounds like a pretty egregious exaggeration.

In any case, we are now below thresholds, and this patch looks like a great first step to supporting responsive layouts and images in Drupal 8! I did find it confusing that these were added without an API or UI or something to use them. Alex informed me that these are going to be added in subsequent issues (e.g. picture element) and this is an unblocker. It looks like catch's concerns were addressed in #1712352: Configuration system does not support themes.

Therefore, committed and pushed to 8.x. Thanks!!

I'm pretty sure at this point we don't need a change notice, although at whatever spot we end up adding an API/UI to use these, we might.

webchick’s picture

Issue tags:+Spark

Post-emptively tagging "Spark."

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