Problem/Motivation

Responsive themes (including Drupal 8 core themes) will require using a variety of image sizes at different resolutions, so

  • Images are optimized for screen and bandwidth.
  • Anonymous page caching will work out-of-the-box.
  • Other modules can easily alter the output for.

Proposed Resolutions

We need a unified way to handle breakpoints (media queries) defined by themes and modules, this patch adds the following:

Breakpoint class

Breakpoints can be defined by a theme/module using a yml file: myThemeOrModule.breakpoints.yml.

BreakpointGroup class

There's automatically a breakpoint group created for each theme/module that defines breakpoints.
Themes/modules can also define new breakpoint groups using a yml files: myThemeOrModule.breakpoint_groups.yml.

Documentation: http://drupal.org/node/1803874
Background info: #23, #101

Remaining tasks in follow-up issues

Original report by [Jeff Burnz]

see #1170478: Responsive Images for the background discussion

Related discussions

CommentFileSizeAuthor
#156 breakpoint-156.patch61.73 KBattiks
#156 interdiff_156_148.txt28.8 KBattiks
#148 breakpoint-148.patch62.38 KBattiks
#148 interdiff_148_146.txt3.47 KBattiks
#146 breakpoint-146.patch61.44 KBattiks
#146 interdiff_146_142.txt1.73 KBattiks
#142 breakpoint-142.patch61.44 KBattiks
#142 interdiff_142_135.txt1.39 KBattiks
#138 breakpoint-138.patch76.83 KBattiks
#135 breakpoint-135.patch60.38 KBattiks
#135 interdiff_135_126.txt43.82 KBattiks
#126 breakpoint-126.patch69.74 KBattiks
#126 interdiff_126_122.txt1.03 KBattiks
#122 breakpoint-122.patch69.84 KBattiks
#122 interdiff_122_115.txt13.49 KBattiks
#115 breakpoint-115.patch64.33 KBattiks
#115 interdiff_115_113.txt3.8 KBattiks
#113 breakpoint-113.patch64.29 KBattiks
#113 interdiff_113_111.txt7.91 KBattiks
#111 breakpoint-111.patch64.17 KBattiks
#111 interdiff_111_110.txt2.54 KBattiks
#111 interdiff_111_97.txt50.8 KBattiks
#110 breakpoint-110.patch64.27 KBattiks
#110 interdiff_110_108.txt826 bytesattiks
#110 interdiff_110_97.txt49.8 KBattiks
#108 breakpoint-108.patch63.94 KBattiks
#108 interdiff_108_97.txt49.52 KBattiks
#108 interdiff_108_106.txt17.13 KBattiks
#106 breakpoint-106.patch52.79 KBattiks
#106 interdiff_106_104.txt18.75 KBattiks
#106 interdiff_106_97.txt35.13 KBattiks
#105 interdiff_104_103.txt17.07 KBattiks
#105 interdiff_104_97.txt24.01 KBattiks
#104 breakpoint-104.patch58.38 KBattiks
#104 interdiff_104_103.txt17.07 KBattiks
#104 interdiff_104_97.txt24.01 KBattiks
#103 breakpoint-103.patch57.47 KBattiks
#103 interdiff.txt10.91 KBattiks
#97 breakpoint-97.patch58.02 KBattiks
#97 interdiff.txt4.02 KBattiks
#95 breakpoint.patch56.78 KBattiks
#94 breakpoint.patch56.34 KBattiks
#92 breakpoint.patch55.69 KBattiks
#90 breakpoint.patch55.47 KBattiks
#90 interdiff.txt9.84 KBattiks
#84 breakpoint.patch56.54 KBattiks
#84 interdiff.txt439 bytesattiks
#83 breakpoint.patch56.46 KBattiks
#77 breakpoint.patch55.81 KBattiks
#72 breakpoint.patch55.83 KBattiks
#67 breakpoint.patch56.11 KBattiks
#65 breakpoint.patch55.77 KBattiks
#63 breakpoint.patch44.3 KBattiks
#56 breakpoint.patch44.34 KBattiks
#54 breakpoint.patch43.72 KBattiks
#47 breakpoint.patch43.88 KBattiks
#41 breakpoints.patch44.3 KBattiks
#39 i1734642-39.patch47.72 KBattiks
i1734642-38.patch43.35 KBattiks
#30 export.jpg.jpg27.71 KBjessebeach
#30 export.jpg-1.jpg16.45 KBjessebeach
#7 breakpoints.zip11.39 KBattiks
#7 breakpoints.pdf582.79 KBattiks
#2 i1734642-bartik-theme.patch518 bytesattiks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

attiks’s picture

attiks’s picture

FileSize
518 bytes

Patch to add breakpoints definition to Bartik theme. Breakpoints are only read at the time a theme or the breakpoint module is enabled.

Bojhan’s picture

It might make sense if this issue follows the issue template, I have a hard time following the action points.

jessebeach’s picture

@attiks, would we wrap the breakpoints provided in the .info file in a default wrapper? I'm wondering how useful breakpoints will be if they're defined without any context. I've mentioned before the idea of a progression which is really just a wrapper for a set of breakpoints.

attiks’s picture

@jessebeach breakpoints defined in the theme.info are automatically wrapped in a group breakpoints.group.theme_key

If you want the theme to be able to define multiple groups, that can be taken care of, the following example will create groups breakpoints.group.theme_key, breakpoints.group.theme_key__layout, breakpoints.group.theme_key__node_layout

breakpoints[mobile] = (min-width: 0px)
breakpoints[narrow] = all and (min-width: 740px)
breakpoints[normal] = all and (min-width: 980px)
breakpoints[wide] = all and (min-width: 1220px)

breakpoints_group[layout] = mobile
breakpoints_group[layout] = narrow
breakpoints_group[layout] = normal

breakpoints_group[node_layout] = mobile
breakpoints_group[node_layout] = normal
breakpoints_group[node_layout] = wide
attiks’s picture

@Bojhan you're completely right, will try to write something by Monday.

attiks’s picture

Issue summary: View changes

Updated issue summary.

attiks’s picture

Issue summary: View changes

Updated issue summary.

attiks’s picture

FileSize
582.79 KB
11.39 KB

Mockups (balsamiq) of the breakpoints module so

  • people don't have to install to see how it works
  • we can start discussing UX/UI

Breakpoints are essentially media queries that can be defined by

  • a theme, using the theme.info file (Done by the themer)
  • a user, using the interface (Done by a site administrator or themer)

User roles involved:

  • Themer, knows the ins & outs of media queries, responsive web design, ...
  • Site administrator, knows how to use the admin side of Drupal

Workflow

The themer defines the breakpoints defined by the theme (inside theme.info) to handle the layout changes, and optionally defines additional breakpoints and groups for content layout changes (using the UI, but see also #5). Once the breakpoints and groups are defined, he can use the picture module to decide when which image_styles has to be used (see follow-up comment).

The site administrator is still able to override the settings created by the themer, so he/she has full control over his/her own site.

If you want more info/details feel free to ask.

aspilicious’s picture

In drupal 8 shouldn't this be a config file? $theme.breakpoints.yml

(yes themes can have a config file)

attiks’s picture

@aspilicious thanks, I learned something new, is there an issue describing what parts are going to be moved to config files and what remains inside theme.info?

using $theme.breakpoints.yml will mean breakpoints need to handle this in a special way, since the settings will end inside breakpoints.theme.theme_key and if/when we support groups defined in breakpoints it will even get more complicated.

aspilicious’s picture

Everything that is configuration should live in a config file. I didn't look at the code but I guess you're using database tables. Those aren't needed anymore if they only store info about the breakpoint.

For example (D8):
Fields will be stored in config (with all his settings)

attiks’s picture

No database used, everything is stored in config

Wim Leers’s picture

#10: I don't think we're moving "settings" from module/theme info files into config files?

attiks’s picture

#12 If I understand you correctly you're saying breakpoints should use database tables to store the config?

aspilicious’s picture

We talked a bit in irc and I'm totally confused about #12. In d8 all the settings *should* move to config files/the config system. As breakpoints is purely config I don't see any reason not to do it.

We are moving every module setting to config.

attiks’s picture

from IRC:

[18:49]	aspiliciousAFK	attiks, he says keep using .info files but I don't think thats a good idea...
[18:49]	aspiliciousAFK	as this seems very config to me
[18:50]	aspiliciousAFK	and .info files means .info file parsing
[18:50]	attiks	aspiliciousAFK: theme.info has to stay anyway i think, so the system can find the module/theme
[18:50]	aspiliciousAFK	attiks, yeah sure for discovery
[18:51]	attiks	aspiliciousAFK: so i guess name, package stays inside the .info file, but AFAIK the rest can move to config
[18:51]	aspiliciousAFK	attiks, but not for any other purpose
[18:51]	aspiliciousAFK	yeah
[18:51]	attiks	aspiliciousAFK: i *think*, *hope* so
[18:51]	aspiliciousAFK	attiks, and dependencies and stuff
[18:51]	aspiliciousAFK	needs to stay too
[18:51]	attiks	aspiliciousAFK: dependencies inside .info i guess, i don't see a reason to change it on a per site basis
[18:52]	attiks	aspiliciousAFK: everything that can be changed should be inside config files, all static info inside .info file
[18:52]	aspiliciousAFK	attiks, yeah correct
RobW’s picture

If themes can provide their own config files (awesome!), unifying the source of breakpoints no matter what they are set by sounds like the best choice.

It seems like this (themes with config) would also open the way for themes to provide their own image styles, which is fantastic news for responsive theming.

Wim Leers’s picture

I'm not familiar with CMI, so ignore me. I was indeed saying "just use .info files", because I didn't know they were only going to be used for discovery in D8. Apologies for derailing the conversation.

Wim Leers’s picture

Issue summary: View changes

Updated issue summary.

attiks’s picture

Issue summary: View changes

Updated issue summary.

attiks’s picture

Issue summary: View changes

Updated issue summary.

JohnAlbin’s picture

During the mobile initiative meeting yesterday, we talked about splitting up the UI decisions from the API decisions. So while evaluating these modules, we should be careful to keep our evaluation of the API separate from the UI.

Also, the UI for breakpoints may be shared with the responsive layout builder that the Blocks Everywhere initiative is working on. We discussed that during Drupalcon Munich with Kris Vanderwater.

jessebeach’s picture

@attiks, would you mind if I updated the title of this issue and remove the reference to picture. We can track the picture issue in #1671844: Add support for picture tag and just deal with breakpoints here.

attiks’s picture

Title: Move breakpoints and picture into core » Move breakpoints into core
attiks’s picture

Issue summary: View changes

Updated issue summary.

attiks’s picture

#18 The UI is going to be reviewed in #1775302: Do a UX review of Breakpoint module

RobW’s picture

Is there a comprehensive overview of the purpose/ use cases for breakpoints module? There should be a list of user actions and expectations to test the UI and API against in the above issues.

If there isn't, I'd be happy to write a draft. Pointing me towards the key issues would be helpful.

RobW’s picture

Issue summary: View changes

Updated issue summary.

attiks’s picture

@RobW, there are not many docs yet, but a short intro, if I find some time I'll try to rewrite this later.

Breakpoints

  • Breakpoints is build to manage breakpoints (being full media queries) for responsive designs/layouts.
  • Breakpoints can be defined by a theme, a module or the site administrator
  • Breakpoints can be grouped together, the idea is that other modules can use these groups to work with, without having to know the exact breakpoints inside a particular group.
  • For each theme containing breakpoints, there will be a group created automatically, containing all breakpoints defined by the theme
  • Each breakpoint consist of a name, a media query, an optional set of multipliers and some meta data (source, status)
  • Breakpoints can be enabled/disabled
  • Breakpoints defined by a theme can be overriden (and reverted)
  • Breakpoint groups can be duplicated
  • The weight of a breakpoint inside a breakpoint group can be altered by drag & drop

Multipliers

  • Multipliers can be defined by the site administrator
  • Multipliers are needed to support retina displays (picture tag)

Use cases

1/ Responsive images / picture

  • This module maps each breakpoint/multiplier combination to an image style, so the picture module can use this.
  • The module supports multiple breakpoints groups, because designer/themers might want to use different media queries for different parts of the site.

2/ Layouts

The Layout module is now using 'breakpoints' defined in px, but it would be nice if there's support for full media queries so themers can have full control. AFAIK it isn't decided if they will use 1 breakpoint group or multiple.

3/ Blocks initiative

I assume they need something similar to breakpoints, but I didn't have the time to look at the code to see if and how it can be integrated.

attiks’s picture

@RobW (and others) feel free to ping me in IRC or send me an email to clarify certain parts.

attiks’s picture

Another potential use case #1621594: Media Query Detection

effulgentsia’s picture

During the mobile initiative meeting yesterday, we talked about splitting up the UI decisions from the API decisions.

I created a spin-off issue, #1775774: Allow themes to identify their breakpoints to Drupal, which I hope allows us to decouple #1775530: Move picture into core from the rest of the work in this issue.

Gábor Hojtsy’s picture

@tkoleary made some interesting design proposals that he posted at https://projects.invisionapp.com/share/J66DW0KQ (press shift to see where to click). We tried to use the current user interface and at least I was pretty confused, needed direct handholding from attiks. The UI proposal takes some different approaches, eg. editing the breakpoint widths direct instead of media queries for embedded stuff. This is a level of detail the layout editor we are working on for Drupal 8 needs. A good question is whether this looses us significant features. The theme level breakpoints at #1775774: Allow themes to identify their breakpoints to Drupal use media queries as well.

Gábor Hojtsy’s picture

(Note the grid editor is a plugin component and would not be part of the breakpoints feature per say).

attiks’s picture

#27 "A good question is whether this looses us significant features"

We need the full MQ or themers aren't going to be happy, and AFAIK the grid builder only needs for 2 things:
1/ people not familiar width media queries can easily change the width of each step
2/ the preview is easy to render

Solutions:
1/ We add a field "approximate width" to the breakpoints module
2/ Grid builder stores this in its own data

How are we going to change the MQ inside a BP when people start changing the width of a step?
1/ allow people to change the MQ
2/ grid/BP module changes the MQ using javascript
3/ grid/BP module changes the MQ on save

jessebeach’s picture

FileSize
16.45 KB
27.71 KB

RE: #29; Solutions, 1/: I really want us find a solution that doesn't require the user to provide an approximate size for a step (the area between two breakpoints, I really want this term to catch on because a breakpoint defines a point, not a line). If the user must provide a media query and the approximate size of the area produced by that media query, we're going to allow them quickly diverge code-defined layout definition from their mentally-estimated layout definition. And then if one changes a media query that defines a step and doesn't change the approximate size of the step, all kinds of mayhem will ensue.

I realize that the approximate size of the step is assumed to be necessary because of the way the layout builder currently works. But the current behavior of the layout builder is an artifact of old requirements. All is maleable.

I think we can define the size of a step by finding the width, min-width, min-device-width, and/or their max equivalents in media query associated with a breakpoint. We'll have to do a little grepping to extract that info and some client-side validation, but I'd rather we attempt to determine the size of a step rather than put the onus on a user to calculate or guess. I'm happy to be the one to code this.

What the proposed design does right now is it hides the complexity of media queries from users as a default. They define the min-width of a breakpoint with the slider. It makes it impossible to define two breakpoints, one at 320px and the other at 768px, and then reorder them so that the 768px breakpoint is listed before the 320px in the the group list. It also prevents a user from creating gaps in their group e.g. a breakpoint with max-width:400px and then the next with min-width: 450px.

export.jpg.jpg

We could provide a means of editing the media query after the breakpoint is established with the slider and hopefully alert the user if the edited input fundamentally or illegally changes the basic definition of the breakpoint, creates overlaps or creates gaps. I hope this will address your question @attiks, "How are we going to change the MQ inside a BP when people start changing the width of a step?" Maybe we put the media query configuration under the step edit link?

export.jpg-1.jpg

Or maybe we allow the media query to be edited right from the slider? And give the user a preview of the if they create a gap or overlap? I really think we can determine through validation if this is the case. Maybe I'm just being optimistic :)

attiks’s picture

#30 I see 2 separate use cases

1/ user uses a layout defined by the layout module, hence it uses only simple MQ's (min/max width)
In this case the layout plugin (screenshot) can easily save back the data to the breakpoint, actually overwriting the MQ.

2/ user uses his own breakpoints set containing breakpoints with some advanced MQ (ex from omega: all and (min-width: 980px) and (min-device-width: 980px), all and (max-device-width: 1024px) and (min-width: 1024px) and (orientation:landscape)) If you parse something like this, you'll probably extract 980px, which is fine for the preview, but you're not going to write the value back. Which isn't probably necessary anyway, because people using the layout module like this will (hopefully) know what they are doing.

Trying to avoid that people are

  • putting the MQ's in the wrong order will be hard, breakpoints module doesn't check this.
  • going to create gaps can happen in use case 2, but I'm not sure we should care.

"I'm happy to be the one to code this." If you need help/reviews let me know.

"Maybe we put the media query configuration under the step edit link?" That's possible, but it depends if the gridbuilder is going to be plugin or not, see #1775302-5: Do a UX review of Breakpoint module

webchick’s picture

Wait. Can we not have the UX discussion in two places?

Should this be postponed on the other, or..?

Bojhan’s picture

Status: Active » Postponed

Yes, it should be. Postponed on #1775302: Do a UX review of Breakpoint module

jessebeach’s picture

Maybe we need a UML diagram of the data model so we're all on the same page? I've never actually made one of those but it strikes me it might be helpful here.

attiks’s picture

Status: Postponed » Active

The module is split in to parts, since #1776236: Split breakpoints into API and UI module is done for Drupal 8. This issue is about everything except the UI, so marking as active again, see also #26

JohnAlbin’s picture

> Maybe we need a UML diagram of the data model so we're all on the same page?

I agree a diagram of the breakpoint API would be mighty useful.

attiks’s picture

I'm rewriting most of the code to catch up with core, will try to make something by next week.

attiks’s picture

FileSize
47.72 KB

New patch

attiks’s picture

attiks’s picture

FileSize
44.3 KB
Gábor Hojtsy’s picture

diff --git a/core/modules/breakpoints/README.txt b/core/modules/breakpoints/README.txt
index 0000000..83cd585

index 0000000..83cd585
--- /dev/null

--- /dev/null
+++ b/core/modules/breakpoints/README.txtundefined

+++ b/core/modules/breakpoints/README.txtundefined
@@ -0,0 +1,4 @@

@@ -0,0 +1,4 @@
+-- SUMMARY --
+
+The Breakpoints module allows you to manage all your breakpoints and organize
+them into Breakpointsets.

Core module's don't have a readme, I guess people are not expected to go and read them anyway....

+++ b/core/modules/breakpoints/breakpoints.infoundefined
@@ -0,0 +1,9 @@
+name = Breakpoints
+description = Manage breakpoints and breakpoint sets.

I'd include something about why breakpoints are useful. Eg "Manage breakpoints and breakpoint sets used for adaptive site display." Or something along those lines...

+++ b/core/modules/breakpoints/breakpoints.infoundefined
@@ -0,0 +1,9 @@
+files[] = breakpoints.module
+files[] = breakpoints.install

I don't think these need to be listed for any reason now?

+++ b/core/modules/breakpoints/breakpoints.installundefined
@@ -0,0 +1,5 @@
+<?php
+/**
+ * @file
+ * Install, update and uninstall functions for the breakpoints module.

Empty file for install not needed AFAIS.

+++ b/core/modules/breakpoints/breakpoints.moduleundefined
@@ -0,0 +1,255 @@
+/**
+ * @file
+ * Breakpoints

Now this should be more descriptive. See other core modules for examples. Ironically the breakpoint test module & theme have a much better description. :)

+++ b/core/modules/breakpoints/breakpoints.moduleundefined
@@ -0,0 +1,255 @@
+use \Drupal\breakpoints\Breakpoint;
+use \Drupal\breakpoints\BreakpointSet;

Generally Drupal core avoids using plurals for base objects. Node, User, Block, etc. Not Nodes, Users, Blocks, etc. This starts with the module name. So here "breakpoint.module", breakpoint namespace and Breakpoint class would apply.

+++ b/core/modules/breakpoints/breakpoints.moduleundefined
@@ -0,0 +1,255 @@
+define('BREAKPOINTS_SOURCE_TYPE_THEME', 'theme');
+define('BREAKPOINTS_SOURCE_TYPE_MODULE', 'module');
+define('BREAKPOINTS_SOURCE_TYPE_CUSTOM', 'custom');

These should use "const" in Drupal 8 (not define) and have individual phpdoc on what they are.

Also you have these under Breakpoint:: as well, why here too?!?

+++ b/core/modules/breakpoints/breakpoints.moduleundefined
@@ -0,0 +1,255 @@
+ * Implements hook_enable().
+ * Import breakpoints from all enabled themes.
...
+ * Implements hook_themes_enabled().
+ * Import breakpoints from all new enabled themes.
...
+ * Implements hook_themes_disabled().
+ * Remove breakpoints from all disabled themes.

Should have an empty line between function description and initial phpdoc line.

Not just these, also seen elsewhere.

+++ b/core/modules/breakpoints/breakpoints.moduleundefined
@@ -0,0 +1,255 @@
+        $message = t('The breakpoints from theme %theme are imported and !setlink.', array(
+          '%theme' => check_plain($themes[$theme_key]->info['name']),
+          '!setlink' => module_exists('breakpoints_ui') && $uri ? l(t('a new breakpoint set is created'), $uri, $uri_options) : t('a new breakpoint set is created'),

!setlink as separate text is not nice for translatability. I think this should be its own sentence and then you can optionally wrap it in a link. t('A new breakpoint set is created.') would be the second sentence then.

+++ b/core/modules/breakpoints/breakpoints.moduleundefined
@@ -0,0 +1,255 @@
+  $breakpointsets = entity_load_multiple('breakpoints_breakpointset', $theme_list);
+  foreach ($breakpointsets as $breakpointset) {

$breakpointset written altogether looks pretty strage. I do see how breakpoint_set would look odd (and sound like you are setting a breakpoint if as part of a function name or setting), however three words written altogether is still strange.... Any better ideas?

+++ b/core/modules/breakpoints/breakpoints.moduleundefined
@@ -0,0 +1,255 @@
+/**
+ * Load general settings.
+ */
+function breakpoints_settings() {
+  $config = config('breakpoints');
+  if ($config->isNew()) {
+    return FALSE;
+  }
+  return (object)$config->get();
+}
+
+/**
+ * Save general settings.
+ *
+ * @param array $multipliers
+ *   array containing multipliers.
+ */
+function breakpoints_settings_save($multipliers) {
+  $config = config('breakpoints');
+  if ($config->isNew()) {
+    return FALSE;
+  }
+  $config->set('multipliers', $multipliers);
+  $config->save();
+}
+

The only general settings are multipliers? Then the getter should reflect that or the setter should be more generic if not.

+++ b/core/modules/breakpoints/breakpoints.moduleundefined
@@ -0,0 +1,255 @@
+ * @param string $theme_key
+ *
+ * @return BreakpointSet

Lacks explanation for the param and return.

+++ b/core/modules/breakpoints/breakpoints.moduleundefined
@@ -0,0 +1,255 @@
+  $types['breakpoints_breakpoint'] = array(
...
+  $types['breakpoints_breakpointset'] = array(

In light of the rename of the module to breakpoint, would these be instead 'breakpoint' and 'breakpoint_set'? 'breakpoint_breakpoint', etc. would look odd. Just like the current ones look extensive (even though they do set up a clear namespace which might be preferred after all, more reviewers needed :).

+++ b/core/modules/breakpoints/breakpoints.moduleundefined
@@ -0,0 +1,255 @@
\ No newline at end of file

Mising newline.

+++ b/core/modules/breakpoints/lib/Drupal/breakpoints/Breakpoint.phpundefined
@@ -0,0 +1,281 @@
+  /**
+   * The breakpoint source.
+   *
+   * @var string
+   */

Would be probably worth it to document possible sources here.

+++ b/core/modules/breakpoints/lib/Drupal/breakpoints/Breakpoint.phpundefined
@@ -0,0 +1,281 @@
+  /**
+   * The breakpoint source type.
+   *
+   * @var string
+   */

And source types here (I see it below, but cross-linking or listing them here in phpdoc would be useful).

+++ b/core/modules/breakpoints/lib/Drupal/breakpoints/Breakpoint.phpundefined
@@ -0,0 +1,281 @@
+  const BREAKPOINTS_SOURCE_TYPE_THEME = 'theme';
+  const BREAKPOINTS_SOURCE_TYPE_MODULE = 'module';
+  const BREAKPOINTS_SOURCE_TYPE_CUSTOM = 'custom';

Not sure about current coding standards, would class constants need this much namespacing? It is already under the breakpoint class.

+++ b/core/modules/breakpoints/lib/Drupal/breakpoints/Breakpoint.phpundefined
@@ -0,0 +1,281 @@
+   * Shortcut function to enable a breakpoint and save it.
+   * @see breakpoints_breakpoint_action_confirm_submit()
...
+   * Shortcut function to disable a breakpoint and save it.
+   * @see breakpoints_breakpoint_action_confirm_submit()
...
+   * Check if the mediaQuery is valid.
+   * @see isValidMediaQuery()
...
+   * Check if a mediaQuery is valid.
+   * @see http://www.w3.org/TR/css3-mediaqueries/
+   * @see http://www.w3.org/Style/CSS/Test/MediaQueries/20120229/reports/implement-report.html

Should have an empty line before @see in each case.

+++ b/core/modules/breakpoints/lib/Drupal/breakpoints/Breakpoint.phpundefined
@@ -0,0 +1,281 @@
+      // @todo: strip comments, new lines, ....

Sounds like this would in fact be easier to do compared to the rest of the validator :)

+++ b/core/modules/breakpoints/lib/Drupal/breakpoints/BreakpointSet.phpundefined
@@ -0,0 +1,115 @@
+  /**
+   * The BreakpointSet breakpoints.
+   *
+   * @var array
+   */

Document that this contains Breakpoint type instances (right?).

+++ b/core/modules/breakpoints/lib/Drupal/breakpoints/BreakpointSet.phpundefined
@@ -0,0 +1,115 @@
+  /**
+   * The BreakpointSet source type.
+   *
+   * @var string
+   */

Document possible values.

+++ b/core/modules/breakpoints/lib/Drupal/breakpoints/Tests/BreakpointSetCrudTest.phpundefined
@@ -0,0 +1,75 @@
+      'name' => 'Breakpoint Set CRUD operations',

+++ b/core/modules/breakpoints/lib/Drupal/breakpoints/Tests/BreakpointSetTestBase.phpundefined
@@ -0,0 +1,60 @@
+ * Base class for Breakpoint Set tests.

I think you want to call this "Breakpoint set" on the UI generally. Drupal does not use US title casing for naming things (or for titles for that matter).

Applies elsewhere too.

+++ b/core/modules/breakpoints/tests/breakpoints_theme_test.infoundefined
@@ -0,0 +1,5 @@
+name = Breakpoints Theme Test

Breakpoints theme test(not title case).

Gábor Hojtsy’s picture

BTW the test coverage is great and very extensive, congrats on that!

attiks’s picture

"Generally Drupal core avoids using plurals for base objects" there's already a breakpoint module

I've send himerus an email

Gábor Hojtsy’s picture

Well, the breakpoint module project page says "Omega BOF idea about to be a reality." and it never had a commit, so I'd say it is absolutely fine to reuse that name for a Drupal 8 core module. If @himerus wants to release something for Drupal 8 he will need to change the name. But given no prior history (not even a commit) in the project), its just a name reserved for the past 6 months. It is not like a rules module or not even an image module where this would need careful consideration.

attiks’s picture

About the renaming and naming of entities, 3 possibilities

1/ breakpoint.module
breakpoint_breakpoint
breakpoint_breakpointset

2/ breakpoint.module
breakpoint
breakpointset

3/ two modules
breakpoint.module
breakpoint
breakpointset.module
breakpointset

1/ is how it is now
2/ disadvantage is that you have a breakpointset_load inside breakpoint.module, AFAIK not really according to the guidelines
3/ disadvantage is that you have 2 modules you need to enable, and both are needed at the same time, there's no use in enabling only breakpointset.

Ideas?

attiks’s picture

FileSize
43.88 KB

New patch using breakpoint as name, so no interdiff

most of #42 is fixed

tstoeckler’s picture

How about:
breakpoint.module
breakpoint
breakpoint_set

That would be consistent with node.module/node, comment.module/comment, etc.

attiks’s picture

#48 can you clarify, this looks like option 2 but would mean that you get breakpointset_load inside the breakpoint.module, unless I'm missing something.

tstoeckler’s picture

Right, it's like 2) except IMO breakpoint_set is more in-line with what we do elsewhere than breakpointset. I agree that breakpoint_set_load() looks weird but I don't think it's that bad. Look at the PhpDoc and it tells you that it loads a breakpoint set and you know what's going on...

attiks’s picture

#50 good point, I like it. I'll wait another day so other people can have their say as well.

Gábor Hojtsy’s picture

There are a few examples of modules in core, where multiple objects are defined. Node module as nodes themselves and node types / content types. Node uses node_type as the prefix for node types (such as node_type_form, node_type_load, etc). Then taxonomy module defines terms and vocabularies, which is a different case because neither of them have taxonomy in their name per say :) So it has taxonomy_term_load vs. taxonomy_vocabulary_load, etc. Whatever naming we use we can just build on that and follow an existing practice. To avoid breakpoint_set_load() and such, which can sound pretty confusing, we can also try to avoid 'set' since its a common code verb, and use group? breakpoint_group_load(), etc. Not sure if this is our own terminology question or an industry thing :)

Furthermore, I took a look at the listed "things to be done" in the summary (with my comments after --):

  • Write full documentation for breakpoints. -- This would still be nice to be done.
  • Document API changes. -- What do you mean by this? This is an entirely new module, no APIs changed per say, right?
  • Move theme settings from theme.info to yml. -- Do not think this is in scope for this issue, the breakpoints are defined in a yml file already for themes.
  • Work on breakpoints UI. See also #1775302: Do a UX review of Breakpoint module -- That sounds like a followup (should be listed elsewhere, to not confuse as to what should happen in this issue).

I think the remaining clear point is that this module does not really do anything useful on its own (especially not without a UI), and the introduction of picture module would do something useful with it, right? So it would be committable I think with the expectation that picture module is figured out and lands before 8.x. Otherwise is this targeted as a generic API for contrib that has no use in core (such as node access records and grants for example)?

attiks’s picture

So let's go for
breakpoint_load() and breakpoint_group_load()
Classes will be Breakpoint and BreakpointGroup

attiks’s picture

FileSize
43.72 KB

patch with the renames

attiks’s picture

"Document API changes." isn't needed
"Move theme settings from theme.info to yml" is done
"breakpoints UI" is follow-up indeed, picture can work without a breakpoint UI, so yes comparable to node access

So I'll try to add some more documentation

attiks’s picture

FileSize
44.34 KB

added some more comments

Crell’s picture

breakpoint_load() and breakpoint_group_load()

For new code, please please please do not bother with wrapper functions. Go straight up injected OO from day one. This code looks mostly that, but I wouldn't even bother with the wrapper functions. Factory objects in the Container only, please. Don't add a BC layer when there was nothing there before in the first place. :-)

tim.plunkett’s picture

To clarify, don't add the load wrappers, just call entity_load() directly.
Eventually there will be a way to load them from the container, but that's out of scope for this issue.

Jelle_S’s picture

@Crell, @tim.plunkett: Those wrapper functions are there mostly so we can have %breakpoint and %breakpoint_group as menu arguments (for example in the Breakpoints UI module). Is there/will there be an alternative for that as well?

Crell’s picture

Jelle_S: #1798214: Upcast request arguments/attributes to full objects

Once that gets in, type hint your controller and the rest is magic. :-)

Gábor Hojtsy’s picture

@Crell: well, yeah that is not in yet though, so we need the wrappers for now. Any other concerns, notes?

Crell’s picture

hm. I guess for menu load callback purposes we have to have those for now. But please mark them with a @todo that they're really only for menu load callback purposes. We have this shiny new reduced-spaghetti model; we should be using it as much as possible.

attiks’s picture

FileSize
44.3 KB

New patch, I removed all uses of load, load_all except for the menu callback

Gábor Hojtsy’s picture

As far as I see, all my concerns were resolved. Any other concerns?

attiks’s picture

FileSize
55.77 KB

Fixed a typo and added import functionality for breakpoints defined by modules as well as import functionality of breakpoint groups defined by themes/modules.

Status: Needs review » Needs work

The last submitted patch, breakpoint.patch, failed testing.

attiks’s picture

Status: Needs work » Needs review
FileSize
56.11 KB

added test to make sure settings are updated

Status: Needs review » Needs work

The last submitted patch, breakpoint.patch, failed testing.

attiks’s picture

Testbot result

Non-pass:
Drupal\breakpoint\Tests\BreakpointThemeTest: The test did not complete due to a fatal error.
Drupal\system\Tests\Entity\EntityFormTest: Entity not found in the database.

attiks’s picture

Status: Needs work » Needs review
Issue tags: -Usability, -mobile, -frontend performance, -media queries, -Responsive Design, -Design Initiative, -d8mux

#67: breakpoint.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Usability, +mobile, +frontend performance, +media queries, +Responsive Design, +Design Initiative, +d8mux

The last submitted patch, breakpoint.patch, failed testing.

attiks’s picture

Status: Needs work » Needs review
FileSize
55.83 KB

Testbot is failing on:


    module_uninstall(array('breakpoint_theme_test'));
    $this->assertFalse(entity_load('breakpoint_group', 'module_test'), 'Breakpoint group is removed if module is uninstalled.');

So those lines are commented out, test works fine locally.

Code review is green on http://qa.drupal.org/pifr/test/316298

attiks’s picture

Status: Needs review » Needs work
Issue tags: -Usability, -mobile, -frontend performance, -media queries, -Responsive Design, -Design Initiative, -d8mux

The last submitted patch, breakpoint.patch, failed testing.

attiks’s picture

Status: Needs work » Needs review
Issue tags: +Usability, +mobile, +frontend performance, +media queries, +Responsive Design, +Design Initiative, +d8mux

#72: breakpoint.patch queued for re-testing.

attiks’s picture

Test in #72 failed on Drupal\system\Tests\Common\CascadingStylesheetsTest, table 'config' doesn't exist :/

attiks’s picture

FileSize
55.81 KB

Test was failing because of a dpm :/

Status: Needs review » Needs work

The last submitted patch, breakpoint.patch, failed testing.

attiks’s picture

Status: Needs work » Needs review

retested from qa

attiks’s picture

First try for the docs

This module allows theme and modules to define breakpoints and breakpoint groups.

Breakpoint

A breakpoint consist of a label and a media query.

Themes and modules can define breakpoints by creating a config file called myThemeOrModule.breakpoints.yml

This file contains multiple lines, each line defines one breakpoint and consist of a label (lowercase letters only), followed by a colon and a valid media query.

The order of the lines define the order of the breakpoints, they have to be ordered from the 'smalles' to the 'widest'.

Example (bartik.breakpoints.yml)

mobile: '(min-width: 0px)'
narrow: 'all and (min-width: 560px) and (max-width: 850px)'
wide: 'all and (min-width: 851px)'

Breakpoint group

A breakpoint group is a combination of certain breakpoints, each theme or module can define zero or more breakpoint groups using a file named myThemeOrModule.breakpoint_groups.yml. For each theme or module there's a breakpoint group created automatically, containing all defined breakpoints.

Each breakpoint added to a group can be followed by an array of multipliers, see group2 in the example below. Be carefull you always have to add the colon.

Example (auto created for Bartik theme)

breakpoints:
  - theme.bartik.mobile
  - theme.bartik.narrow
  - theme.bartik.wide
id: bartik
label: Bartik

To define your own breakpoint group, use the following

group1:
  label: Group 1
  breakpoints:
    mobile:
    wide: []
group2:
  label: Group 2
  breakpoints:
    mobile: ['2x']
    wide: ['2x']

Advanced use

You can also add breakpoints defined by other modules or themes, but you'll have to use the full name.

Example

moduletest:
  label: Test Module
  breakpoints:
    theme.breakpoint_test_theme.mobile: []
    theme.breakpoint_test_theme.narrow: ['3x', '4x']
    theme.breakpoint_test_theme.wide:

Multipliers

Multipliers are simple string that are needed to support 'retina' displays, they consist of a number followed by an x

Example

'1.5x' // support Android
'2x' // support Mac retina display
batigolix’s picture

Great start for documentation!

Do you want to revise these docs within this issue? Why not add the documentation already to the d.o. community docs? And do all further revising over there? (there is revisions to keep track of changes.) . So that you can focus on the code in this issue.

For the moment it could be a child page in the Media section besides the core image module docs.

attiks’s picture

Doc page created at http://drupal.org/node/1803874

attiks’s picture

FileSize
56.46 KB

Some more minor fixes

attiks’s picture

FileSize
439 bytes
56.54 KB

Added import breakpoints defined by modules when enabling breakpoint

Status: Needs review » Needs work

The last submitted patch, breakpoint.patch, failed testing.

attiks’s picture

bot problems: failing on Drupal\system\Tests\Entity\EntityFormTest

attiks’s picture

Status: Needs work » Needs review
Issue tags: -Usability, -mobile, -frontend performance, -media queries, -Responsive Design, -Design Initiative, -d8mux

#84: breakpoint.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Usability, +mobile, +frontend performance, +media queries, +Responsive Design, +Design Initiative, +d8mux

The last submitted patch, breakpoint.patch, failed testing.

attiks’s picture

Status: Needs work » Needs review

Failing on Drupal\system\Tests\Entity\EntityTranslationTest: "Two entities loaded by uid without caring about property translatability."

attiks’s picture

FileSize
9.84 KB
55.47 KB

some more refactoring

Status: Needs review » Needs work

The last submitted patch, breakpoint.patch, failed testing.

attiks’s picture

Status: Needs work » Needs review
FileSize
55.69 KB

go bot

Status: Needs review » Needs work

The last submitted patch, breakpoint.patch, failed testing.

attiks’s picture

Status: Needs work » Needs review
FileSize
56.34 KB
attiks’s picture

FileSize
56.78 KB

Some changes:
Both breakpoints and breakpoint groups can be duplicated
Breakpoints and breakpoint groups defined by a theme or module can be overridden/reverted, the same config file is used, but the status (overridden) is tracked.
createDuplicate is replaced by duplicate on Breakpoint and BreakpointGroup

attiks’s picture

Time for some reviews, the clock is ticking ....

attiks’s picture

FileSize
4.02 KB
58.02 KB

Check if breakpoint group/breakpoint already exists while importing/reverting so the uuid doesn't change.

tstoeckler’s picture

Status: Needs review » Needs work

Here's a review. Sorry, if I sound negative and for nagging about so many things. I really love this patch and especially the media query validation and also the tests are really extensive. So while I propose some minor refactoring below, I think this is actually quite far along. Especially since those tests are already in place, refactoring should be easy(er). All in all, awesome work!!!

+++ b/core/modules/breakpoint/breakpoint.module
@@ -0,0 +1,512 @@
+/**
+ * Implements hook_themes_enabled().
+ *
+ * Import breakpoints from all new enabled themes.
+ *
+ * @param array $theme_list
+ *   An array of theme names.
+ */
+function breakpoint_themes_enabled($theme_list) {
...
+/**
+ * Implements hook_modules_enabled().
+ *
+ * Import breakpoints from all new enabled modules.
+ *
+ * @param $modules
+ *   An array of the modules that were enabled.
+ */
+function breakpoint_modules_enabled($modules) {

There is a lot of duplication between these functions, I think that could use a helper function.

On the other hand, I don't really understand what the use-case is for allowing modules to provide breakpoints. This seems to be clearly in the realm of themes, by definition. Could you elaborate on that?

+++ b/core/modules/breakpoint/breakpoint.module
@@ -0,0 +1,512 @@
+      // Build a new breakpoint group.
+      $breakpoint_group = new BreakpointGroup();
+      $breakpoint_group->id = $id;
+      $breakpoint_group->label = $label;
+      $breakpoint_group->source = $id;
+      $breakpoint_group->sourceType = $source_type;

This should use entity_create(). Even better, instead of passing in $label, $id and $source_type, why don't we let the callers of this function load the group on their own. I didn't check, but a caller should check whether the breakpoint exists, i.e. whether to call entity_load() or entity_create().

+++ b/core/modules/breakpoint/breakpoint.module
@@ -0,0 +1,512 @@
+    foreach ($media_queries as $name => $media_query) {
+      // Use the existing breakpoint if it exists.
+      $breakpoint = entity_load('breakpoint', $source_type . '.' . $id . '.' . $name);
+      if (!$breakpoint) {
+        // Build a new breakpoint.
+        $breakpoint = new Breakpoint;
+        $breakpoint->name = $name;
+        $breakpoint->label = drupal_ucfirst($name);
+        $breakpoint->mediaQuery = $media_query;
+        $breakpoint->source = $id;
+        $breakpoint->sourceType = $source_type;
+        $breakpoint->status = TRUE;
+        $breakpoint->weight = $weight++;
+        $breakpoint->save();
+      }

Similar this should use entity_create(), but I also wonder if it wouldn't be cleaner to pass in $breakpoints directly.

That would make this function something like:
function breakpoint_group_set_breakpoints($group, $breakpoints)
which would be a pretty clean, and probably even public function. Even cooler would be a method on the breakpoint group class. Maybe I'm going a bit overboard, though.

+++ b/core/modules/breakpoint/breakpoint.module
@@ -0,0 +1,512 @@
+      // Breakpoints is mandatory.
+      if (isset($data['breakpoints']) && !empty($data['breakpoints'])) {

Then why do we check for it? :-)
There is no else below either, so I think the condition can be removed.

+++ b/core/modules/breakpoint/breakpoint.module
@@ -0,0 +1,512 @@
+          $breakpoint_group = new BreakpointGroup();

Again, should use entity_create()

+++ b/core/modules/breakpoint/breakpoint.module
@@ -0,0 +1,512 @@
+          $breakpoint_group->source = $group_id;
+          $breakpoint_group->id = $id;

The difference between $group_id and $id is completely unclear to me from the PHPDoc and the surrounding code. This definitely needs an in-line comment in case they are not identical.

+++ b/core/modules/breakpoint/breakpoint.module
@@ -0,0 +1,512 @@
+          if (isset($data['label']) && !empty($data['label'])) {
+            $breakpoint_group->label = $data['label'];
+          }
+          else {
+            $breakpoint_group->label = drupal_ucfirst($data['label']);
+          }
+        }

!empty() should suffice here, the additional isset() is needless IMO.

Even better: this code could be shortened with cool new ?: short ternary.

+++ b/core/modules/breakpoint/breakpoint.module
@@ -0,0 +1,512 @@
+          if (isset($data['label']) && !empty($data['label'])) {
+            $breakpoint_group->label = $data['label'];
+          }
+          else {
+            $breakpoint_group->label = drupal_ucfirst($data['label']);
+          }

Likewise.

+++ b/core/modules/breakpoint/breakpoint.module
@@ -0,0 +1,512 @@
+          // Check if breakpoint exists, assume short name.
+          $breakpoint = entity_load('breakpoint', $source_type . '.' . $group_id . '.' . $breakpoint_id);
+          // If the breakpoint doesn't exist, try using the full name.
+          if (!$breakpoint) {
+            $breakpoint = entity_load('breakpoint', $breakpoint_id);
+          }

It seems the short name is longer than the full name?! (If I am correct, the comments are correct, but the code should be the other way around.)

Also this duplicity seems to be a novelty and I wonder if it is necessary in terms of complexity. If I read your above documentation correctly, this is to allow modules/themes to define breakpoints on behalf of other modules/themes. I don't think we allow this for any other config (or configEntities), so what is the use-case for that? Removing this would help reduce complexity, IMO.

+++ b/core/modules/breakpoint/breakpoint.module
@@ -0,0 +1,512 @@
+            // Check if the multipliers are set, if not set them now.

Nit-pick: I think the comma should be a semi-colon.

+++ b/core/modules/breakpoint/breakpoint.module
@@ -0,0 +1,512 @@
+            if (is_array($multipliers) && !empty($multipliers)) {

This also seems like baby-sitting breaking code. The !empty() check should suffice. If someone puts something other than an array in there, tough luck.

+++ b/core/modules/breakpoint/breakpoint.module
@@ -0,0 +1,512 @@
+              $settings = breakpoint_settings();
+              $defined_multipliers = $settings->multipliers;
+              $new_multipliers = array_diff($multipliers, $defined_multipliers);
+              if (!empty($new_multipliers)) {

The first two lines can be written in one-line. That seems like a minor detail, but I think in a level with this many levels of code-nesting, one variable less to keep track of in your head is actually a great help.

FWIW, this could even be inlined together with the next 2 lines (i.e. all 4 in one), but that is debatable, I guess.

+++ b/core/modules/breakpoint/breakpoint.module
@@ -0,0 +1,512 @@
+                $defined_multipliers = array_merge($defined_multipliers, $new_multipliers);
+                breakpoint_settings_save_multipliers($defined_multipliers);

This re-purposing of an existing local variable is a WTF, IMO. Inlining this as well would help.

+++ b/core/modules/breakpoint/breakpoint.module
@@ -0,0 +1,512 @@
+              $multipliers = drupal_map_assoc(array_values($multipliers));

This needs a comment. From simply reading the code, it seems completely unneccesary.

+++ b/core/modules/breakpoint/breakpoint.module
@@ -0,0 +1,512 @@
+                $breakpoint->multipliers += $new_multipliers;
+                $breakpoint->save();

Why do we need to call breakpoint_settings_save_multipliers() and then call $breakpoint->save() with the new multipliers afterwards? That seems 100% redundant.

+++ b/core/modules/breakpoint/breakpoint.module
@@ -0,0 +1,512 @@
+    if ($breakpoint_group->sourceType == $source_type) {
+      // delete the default group.
+      $breakpoint_group->delete();

This condition should be documented in the function's PHPDoc, i.e. if you pass the ID of a breakpoint group that belongs to a theme, and set $source_type to Breakpoint::SOURCE_TYPE_MODULE, the breakpoint group will *not* be deleted. This is a private function, but still, when a function is called *_delete() and it doesn't delete() stuff (in certain conditions) we should tell people.

+++ b/core/modules/breakpoint/breakpoint.module
@@ -0,0 +1,512 @@
+      // delete all breakpoints defined by this theme/module.

Nitpick: delete -> Delete

+++ b/core/modules/breakpoint/breakpoint.module
@@ -0,0 +1,512 @@
+      foreach ($names as &$name) {
+        $name = drupal_substr($name, drupal_strlen($entity_info['config prefix']) + 1);
+      }

Does this have to do with the duplicity of names mentioned above? In any case, this needs a comment.

+++ b/core/modules/breakpoint/breakpoint.module
@@ -0,0 +1,512 @@
+        if ($breakpoint->sourceType == $source_type && $breakpoint->source = $breakpoint_group->id) {
+          $breakpoint->delete();

This check (the first one) means that we support breakpoint groups that have $source_type == Breakpoint::SOURCE_TYPE_THEME to contain breakpoints that have $source_type == Breakpoint::SOURCE_TYPE_MODULE. That seems like a weird edge-case and huge WTF.

+++ b/core/modules/breakpoint/breakpoint.module
@@ -0,0 +1,512 @@
+  // Deletet groups defined by a module/theme even if that module/theme didn't
+  // define any breakpoints.
+  foreach ($ids as $id) {
+    // delete all breakpoint groups defined by the theme or module.
+    _breakpoint_delete_breakpoint_groups($id, $source_type);
+  }

Typo: Deletet -> Delete

Also this needs to explain how this is different from the $breakpoint_group->delete() above, which I
totally don't get.

From actually reading _breakpoint_delete_breakpoint_groups() it seems the only difference is that that calls entity_load_multiple() without the $id argument, and then checks all breakpoint groups for the correct ID and deletes it. That seems totally needless IMO, and is huge WTF.

+++ b/core/modules/breakpoint/breakpoint.module
@@ -0,0 +1,512 @@
+function breakpoint_settings() {
...
+function breakpoint_settings_save_multipliers($multipliers) {

In general, it seems these functions only exist as wrappers to interact with config(). I think we should drop those, they create needless indirection, IMO.

+++ b/core/modules/breakpoint/breakpoint.module
@@ -0,0 +1,512 @@
+  return (object)$config->get();

There should be space after the typecast.

Also is there any need to cast to an object. I think we try to avoid anonymous standard classes lingering around, and I think it would be more standard to have $settings be an array.

+++ b/core/modules/breakpoint/breakpoint.module
@@ -0,0 +1,512 @@
+      return _breakpoint_import_breakpoints($themes[$theme_key]->info['name'], $theme_key, Breakpoint::SOURCE_TYPE_THEME, $theme_breakpoints);

I commented above, that instead of passing $media_queries, we maybe should be passing $breakpoints. Here the variable is already called $theme_breakpoints. That is a WTF for me, and also indicates that $theme_breakpoints actually does not contain Breakpoint objects. :-( Looking into breakpoint_get_theme_breakpoint_list() it actually returns config arrays, but I don't see why it does not use entity_load() instead of config()->get().

+++ b/core/modules/breakpoint/breakpoint.module
@@ -0,0 +1,512 @@
+function breakpoint_entity_info() {

It seems strange to define these in a (non-required) module, as this seems like a pretty "base system" sort of entity. But as
1. The breakpoints aren't actually required *for now*
2. Entity types currently need to be provided by a module
3. We don't want to lump this in system module
I guess it's the best we can do for now.

+++ b/core/modules/breakpoint/breakpoint.module
@@ -0,0 +1,512 @@
+/**
+ * Load all breakpoint groups as select options.
+ *
+ * @return array
+ *   An array containing breakpoint group labels indexed by their ids.
+ */
+function breakpoint_group_select_options() {

This should be renamed to breakpoint_group_labels().
Even if the only usage might end up being select list options, the function is really not specific to that.

+++ b/core/modules/breakpoint/breakpoint.module
@@ -0,0 +1,512 @@
+ * Load all breakpoints as select options.
...
+function breakpoint_select_options() {

Likeweise.

+++ b/core/modules/breakpoint/breakpoint.module
@@ -0,0 +1,512 @@
+    $options[$breakpoint->id()] = $breakpoint->label() . ' (' . $breakpoint->source . ' - ' . $breakpoint->sourceType .   ') [' . $breakpoint->mediaQuery . ']';

It seems a bit strange to show the media query, and not the multiplier here (I would have assumed to find either both or neither). That said, if the media query is much more valuable information than the multiplier, that might very well be valid.

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.php
@@ -0,0 +1,374 @@
+  /**
+   * The original media query.
+   *
+   * @var string
+   */
+  public $originalMediaQuery = '';

I find it strange to track only this, and not the $label or $multipliers or $weight, ...

It seems this should be ripped out in favor of overriding support in ConfigEntityBase?!

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.php
@@ -0,0 +1,374 @@
+  /**
+   * The breakpoint source type.
+   *
+   * @var string
+   *   Allowed values:
+   *     Breakpoint::SOURCE_TYPE_THEME
+   *     Breakpoint::SOURCE_TYPE_MODULE
+   *     Breakpoint::SOURCE_TYPE_CUSTOM
+   */
+  public $sourceType = Breakpoint::SOURCE_TYPE_CUSTOM;

As hinted at above, I think this should not be a property of the breakpoint at all, if we already have it on the breakpoint group.

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.php
@@ -0,0 +1,374 @@
+   * The Breakpoint overridden status.
+   *
+   * @var boolean
+   */
+  public $overridden = FALSE;

The overridden logic, should really be ported to ConfigEntityBase. That said, if it works, and is tested, that could be a follow-up.

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.php
@@ -0,0 +1,374 @@
+  /**
+   * Overrides Drupal\config\ConfigEntityBase::__construct().
+   */
+  public function __construct(array $values = array(), $entity_type = 'breakpoint') {
+    parent::__construct($values, $entity_type);
+    if (!isset($this->uuid)) {
+      $uuid = new Uuid();
+      $this->uuid = $uuid->generate();
+    }
+  }

Likewise.

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.php
@@ -0,0 +1,374 @@
+    // Add '1x' multiplier.
+    if (!array_key_exists('1x', $this->multipliers)) {
+      $this->multipliers = array('1x' => '1x') + $this->multipliers;
+    }

In case we don't want to support the '1x' key having a different value than '1x' this could simply be.
$this->multiplies['1x'] = '1x';
I also am not sure why this should be hardcoded, but that could very well be totally valid.

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.php
@@ -0,0 +1,374 @@
+    return $this->sourceType
+      . '.' . $this->source
+      . '.' . $this->name;

I'm not aware that do that sort of line break anywhere else in core, and I find it rather unnecessary here.

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.php
@@ -0,0 +1,374 @@
+      throw new Exception(
+          t(
+            "Expected one of '@custom', '@module' or '@theme' for breakpoint sourceType property but got '@sourcetype'.",
+            array(
+              '@custom' => Breakpoint::SOURCE_TYPE_CUSTOM,
+              '@module' => Breakpoint::SOURCE_TYPE_MODULE,
+              '@theme' => Breakpoint::SOURCE_TYPE_THEME,
+              '@sourcetype' => $this->sourceType,
+            )
+          )

First of all, this should not use t() but format_string(). Second of all, I like verbosity in errors/exceptions, but a simple "Invalid source type @source_type." seems as though it would suffice, IMO.

Also it seems like this check and a bunch other below belong into ::isValid() instead of ::buildConfigName()

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.php
@@ -0,0 +1,374 @@
+    return $this->sourceType
+      . '.' . $this->source
+      . '.' . $this->name;

This is identical to ::getConfigName(), which seems weird.

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.php
@@ -0,0 +1,374 @@
+  public function duplicate() {
...
+  public function enable() {
...
+  public function disable() {

These also seem like candidates for ConfigEntityBase

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.php
@@ -0,0 +1,374 @@
+  public static function isValidMediaQuery($media_query) {

Just for reference, this function does a bunch of complicated checks, which I did not (bother to) understand, quite frankly. I basically skipped this in my review.

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.php
@@ -0,0 +1,374 @@
+    $media_features = array(
+      'width' => 'length', 'min-width' => 'length', 'max-width' => 'length',
+      'height' => 'length', 'min-height' => 'length', 'max-height' => 'length',
+      'device-width' => 'length', 'min-device-width' => 'length', 'max-device-width' => 'length',
+      'device-height' => 'length', 'min-device-height' => 'length', 'max-device-height' => 'length',
+      'orientation' => array('portrait', 'landscape'),
+      'aspect-ratio' => 'ratio', 'min-aspect-ratio' => 'ratio', 'max-aspect-ratio' => 'ratio',
+      'device-aspect-ratio' => 'ratio', 'min-device-aspect-ratio' => 'ratio', 'max-device-aspect-ratio' => 'ratio',
+      'color' => 'integer', 'min-color' => 'integer', 'max-color' => 'integer',
+      'color-index' => 'integer', 'min-color-index' => 'integer', 'max-color-index' => 'integer',
+      'monochrome' => 'integer', 'min-monochrome' => 'integer', 'max-monochrome' => 'integer',
+      'resolution' => 'resolution', 'min-resolution' => 'resolution', 'max-resolution' => 'resolution',
+      'scan' => array('progressive', 'interlace'),
+      'grid' => 'integer',
+    );

Even though that costs a bunch of more lines, can we split each key-value pair onto its own line?, that would be awesome and make this much easier to read.

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/BreakpointGroup.php
@@ -0,0 +1,179 @@
+  public function save() {
+    // Only save the keys, but return the full objects.
+    $this->breakpoints = array_keys($this->breakpoints);
+    parent::save();
+    $this->loadAllBreakpoints();
+  }

This seems like it should be a facility in CofigEntityBase, as this will need to be solved by all nested ConfigEntites. Again, if it works for now, let's do that in a follow-up.

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Tests/BreakpointApiTest.php
@@ -0,0 +1,90 @@
+class BreakpointApiTest extends BreakpointTestBase {

Nitpick: Per our semi-new coding standards API should be capitalized, i.e.
BreakpointAPITest

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Tests/BreakpointApiTest.php
@@ -0,0 +1,90 @@
+    catch (Exception $e) {
...
+    catch (Exception $e) {
...
+    catch (Exception $e) {
...
+    catch (Exception $e) {

I didn't mention this above, but the Exceptions thrown (and caught) here should be typed, i.e. either \InvalidArgumentException or something more specific like InvalidBreakpointSourceException. Either way, the new standard is to use \Exception instead of Exception (both when throwing and catching).

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Tests/BreakpointApiTest.php
@@ -0,0 +1,90 @@
+    $this->assertEqual((string) $breakpoint->id(), '', t('breakpoint_config_name: No id is set when an invalid sourceType is entered.'));

I guess assertNull() could be used here?

+++ b/core/modules/breakpoint/breakpoint.module
@@ -140,42 +140,22 @@
+    // Build a breakpoint group.
+    $breakpoint_group = new BreakpointGroup();
+    $breakpoint_group->id = $id;
+    $breakpoint_group->label = $label;
+    $breakpoint_group->source = $id;
+    $breakpoint_group->sourceType = $source_type;

This should use entity_create()

+++ b/core/modules/breakpoint/breakpoint.module
@@ -140,42 +140,22 @@
+      $breakpoint = new Breakpoint;
+      $breakpoint->name = $name;
+      $breakpoint->label = drupal_ucfirst($name);
+      $breakpoint->mediaQuery = $media_query;
+      $breakpoint->source = $id;
+      $breakpoint->sourceType = $source_type;
+      $breakpoint->status = TRUE;
+      $breakpoint->weight = $weight++;
+      $breakpoint->save();

Likewise.

webchick’s picture

Can you explain why entity_create is preferable to straight-up OO PHP code that everyone can understand? I also filed #1785360: Remove entity_create() wrapper? about this, since that wrapper function makes zero sense to me.

tstoeckler’s picture

Re #99: I commented over there.
That made me find the following code

  // Assign a new UUID if there is none yet.
  if ($this->uuidKey && !isset($entity->{$this->uuidKey})) {
    $uuid = new Uuid();
    $entity->{$this->uuidKey} = $uuid->generate();
  }

in DatabaseStorageController::create(), so the UUID generation in the Breakpoint and BreakpointGroup is unneccessary.

attiks’s picture

#98 Thanks for the review, some feedback

_breakpoint_import_breakpoints

Some clarification on _breakpoint_import_breakpoints/_breakpoint_import_breakpoint_groups: this function is called to read breakpoints defined by the theme (or module) as added by this issue #1775774: Allow themes to identify their breakpoints to Drupal. A theme defines breakpoints as follows

/core/themes/bartik/config/bartik.breakpoints.yml

mobile: '(min-width: 0px)'
narrow: 'all and (min-width: 560px) and (max-width: 850px)'
wide: 'all and (min-width: 851px)'

So the only things defined are the theme_key ('bartik'), the machine_name ('mobile') and the media query ('(min-width: 0px)').

When a theme is enabled, it will call _breakpoint_import_breakpoints('bartik', 'bartik', 'theme', array('mobile' => '...', ...)) , so the breakpoint group and breakpoints don't exist. But the same function is also used when the breakpoint group is reverted, at that time both the breakpoint group and the breakpoints exists. If the caller is supposed to create the breakpoint group that code would be replicated to hook_themes_enabled, hook_modules_enabled en BreakpointGroup->revert(). It used to be like this, but I refactored it to the current situation.

I like the idea of moving everything to BreakpointGroup, so I'll try to do that. It will make the code cleaner.

if (isset($data['breakpoints']) && !empty($data['breakpoints'])) { is needed since the data is coming from a yml file. Maybe it's better to throw an exception?

Breakpoints defined by a module

I think modules like gridbuilder/layout might want to do this, so they can define some defaults.

Breakpoint group using breakpoints from other sources

This is added to the import to make it as flexible as possible, so a theme can use breakpoints from a subtheme. But once the breakpoint group is created it makes sense that breakpoints can be added to an existing group. Using the bartik breakpoints as an example: if I also want a breakpoint for tablets in landscape orientation, I can create a new breakpoint (source_type = custom), override the bartik breakpoint group and add my breakpoint to that group. If the bartik theme is disabled later, it shouldn't delete my own custom breakpoint.

Deleting

Since a theme/module can define breakpoints, or define breakpoint groups, or define both we need to handle all those cases in the case of a delete.

Multipliers

Multipliers are defined by the breakpoint module (defaults are 1x 1.5x and 2x) but themes can define a new multiplier when defining a group. So we have to check if the multiplier exists, and if it doesn't it has to be added to the general settings as well

"It seems a bit strange to show the media query, and not the multiplier here" multipliers are a property of a breakpoint, the most significant part of a breakpoint is the media query, for the moment the only module using the multiplier is the picture module.

The 1x multipliers should always be defined, since these are defined in a breakpoint.yml and a user could alter those, we make sure (babysit?) that 1x is defined.

The overridden logic

This is added, because the createDuplicate from ConfigEntityBase, creates a new config object, with a different name. So if you override a breakpoint defined by a theme, it will generate a new config object, meaning all references to the original config key are lost.

entity_create()

#99 I used new Breakpoint, because it's easier to read and you get free autocomplete in your editor, but after reading the comments at #1785360: Remove entity_create() wrapper?, it make sense why this is needed
#100 I duplicated this, because I wasn't using the entity_create

I'll have a look at the code and refactor it. Thanks again for the extensive review.

attiks’s picture

To summarize:

  1. use entity_create
  2. move as much as possible from breakpoint.module to Breakpoint/BreakpointGroup
  3. add better inline docs
attiks’s picture

Status: Needs work » Needs review
FileSize
10.91 KB
57.47 KB

Fase 1: use entity_create

attiks’s picture

Fase 2: most is moved to BreakpointGroup, except for the deletes, since the deletes can operate on multiple breakpoints/breakpoint groups

attiks’s picture

FileSize
24.01 KB
17.07 KB

Messed up the interdiffs

attiks’s picture

more cleaning, all remarks from #98 should be addressed except for:
Exceptions
Breakpoint::isValidMediaQuery() they are arranged so that min/max variants are on the same line.

Changes:
Multipliers can no longer be defined by using a bartik.breakpoint_groups.yml file

andypost’s picture

#101 better to include into issue summary - this could be a part of docs

attiks’s picture

Exceptions added

attiks’s picture

Issue summary: View changes

Updated issue summary.

attiks’s picture

Title: Move breakpoints into core » Move breakpoint module into core

Title renamed

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Tests/BreakpointAPITest.phpundefined
@@ -0,0 +1,94 @@
+    ¶

grrr

attiks’s picture

White space fixed.
Module added to MAINTAINERS.txt

For me this looks like it's complete, so feel free to review.

attiks’s picture

Found an error in the media query validation, floating numbers are allowed as well.

New test added.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Looked through the whole patch. Mostly code style comments but also a couple more important comments in there:

+++ b/core/modules/breakpoint/breakpoint.moduleundefined
@@ -0,0 +1,371 @@
+use \Drupal\breakpoint\Breakpoint;
+use \Drupal\breakpoint\BreakpointGroup;

I don't think the leading \ is needed? It is not used at other places in core.

+++ b/core/modules/breakpoint/breakpoint.moduleundefined
@@ -0,0 +1,371 @@
+ * @param $modules
+ *   An array of the modules that were enabled.
+ */
...
+ * @param $modules
+ *   An array of the modules that were uninstalled.
+ */

array (include type)

+++ b/core/modules/breakpoint/breakpoint.moduleundefined
@@ -0,0 +1,371 @@
+ * @param string $group_id
+ *   Id of the breakpoint group.
...
+ * @param array $ids
+ *   Id's of the breakpoint group.
+ * @param string $sourceType

Identifier of ...
Identifiers of...

+++ b/core/modules/breakpoint/breakpoint.moduleundefined
@@ -0,0 +1,371 @@
+ *
+ * @return boolean
+ */
+function _breakpoint_import_breakpoint_groups($source, $source_type) {

What does the return value mean?

+++ b/core/modules/breakpoint/breakpoint.moduleundefined
@@ -0,0 +1,371 @@
+ *
+ */
+function _breakpoint_delete_breakpoints($ids, $source_type) {
...
+ *
+ */
+function _breakpoint_delete_breakpoint_groups($group_id, $source_type) {

Extra newline

+++ b/core/modules/breakpoint/breakpoint.moduleundefined
@@ -0,0 +1,371 @@
+ *
+ * @todo Needed for menu_callback and machine_name.
+ *
+ * @see http://drupal.org/node/1798214
...
+ *
+ * @todo Needed for menu_callback and machine_name.
+ *
+ * @see http://drupal.org/node/1798214
+ *
+ */

While it is great to see this here, I think this does not bring it across that the @todo is for removal. Also I think the @see is directly related to the @todo, which case it would be without an extra newline, no?

Also, finally, the last quoted section has one too many newlines.

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.phpundefined
@@ -0,0 +1,356 @@
+  /**
+   * The possible values for sourceType.
+   */
+  const SOURCE_TYPE_THEME = 'theme';
+  const SOURCE_TYPE_MODULE = 'module';
+  const SOURCE_TYPE_CUSTOM = 'custom';

Should document these one-by-one as per our doc guidelines.

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.phpundefined
@@ -0,0 +1,356 @@
+   * The original media query.
+   * This is tracked separately, because a user can override a single breakpoint
+   * and reloading the media query from the theme/module is expensive.

There should be a newline after the first comment. Also, I don't understand this comment.

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.phpundefined
@@ -0,0 +1,356 @@
+  /**
+   * The Breakpoint overridden status.

This is not a capital B elsewhere, so make it lowercase here too :)

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.phpundefined
@@ -0,0 +1,356 @@
+   *
+   */
...
+   *
+   */
...
+   *
+   */

Extra newline.

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/BreakpointGroup.phpundefined
@@ -0,0 +1,321 @@
+   * The BreakpointGroup ID (machine name).
+   *
...
+   * The BreakpointGroup UUID.
+   *
...
+   * The BreakpointGroup label.
+   *
...
+   * The BreakpointGroup breakpoints.
+   *
+   * @var array
...
+   * The breakpoint source: theme or module name.
...
+   * The BreakpointGroup source type.
...
+   * The BreakpointGroup overridden status.

These should all say "breakpoint group" no? We are explaining this in a human language :)

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/BreakpointGroup.phpundefined
@@ -0,0 +1,321 @@
+   *
+   */
+  public function duplicate() {

Yet another extra newline.

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/BreakpointGroup.phpundefined
@@ -0,0 +1,321 @@
+   * @param string $id
+   *   Name of the breakpoint group.

Id is the name?! I think this points to an interesting discrepancy in the data design. Looking at breakpoints, it has an $id for config name and $name for machine name, while breakpoint groups only have $id which is the machine name. This is pretty confusing.

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/BreakpointGroup.phpundefined
@@ -0,0 +1,321 @@
+    /* @var $breakpoint_group \Drupal\breakpoint\BreakpointGroup */
...
+    /* @var $breakpoint_group \Drupal\breakpoint\BreakpointGroup */

I think these would be correct as // inline comments, we don't use /* and */ as inline comment markers.

attiks’s picture

Thanks for reviewing

"Id is the name?! I think this points to an interesting discrepancy in the data design. Looking at breakpoints, it has an $id for config name and $name for machine name, while breakpoint groups only have $id which is the machine name. This is pretty confusing."

For breakpoints we have a name (narrow) used as part of the config name, the id (theme.bartik.narrow) is the full config name
For breakpoint groups we only need a name or id

attiks’s picture

Status: Needs work » Needs review

/* @var $breakpoint_group \Drupal\breakpoint\BreakpointGroup */ is removed, but this is the only format recognized by IDE's

attiks’s picture

renamed breakpoint group name to id

Bojhan’s picture

Assigned: Unassigned » JohnAlbin
Issue tags: -Design Initiative

@John Given that is directly in the mobile initiative, I imagine you should give this a full review.

Bojhan’s picture

For module description, maybe something like; "Manage breakpoint settings for responsive themes."

Gábor Hojtsy’s picture

The latest patch resolved all my concerns, so indeed, new reviewers are welcome to come out with any more :)

jwilson3’s picture

"Manage breakpoint settings for responsive themes."

To further clarify that we arent talking about programming/debugging breakpoints, what about module description:

"Manage screen size breakpoints for responsive themes."

jessebeach’s picture

breakpoint_ui should be claimed as a project namespace unless the intent is to package it inside the breakpoint module.

attiks’s picture

attiks’s picture

FileSize
13.49 KB
69.84 KB

Some more cleaning thanks to Jelle_S:

  • added a reload_from_module to revert breakpoint groups defined by modules
  • custom breakpoints / breakpoint groups cannot be overridden
  • more documentation
  • more tests
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

The latest changes look good!

I have also been trying to get ahold of John but was not successful. Since this is just introducing a pretty generic API and it does not even include any UI, I don't think we should block this on John's availability. Further work in the mobile initiative to introduce responsive images with the picture element is blocked on this and we have very little time left.

I've reviewed this patch multiple times and have been working with attiks to make sure it works well. I'm also confident he would work with followups to fix any concerns coming up later (with Jelle_S).

attiks’s picture

One sec, you (and I) missed this throw new \Exception("something went wrong :s");

Will be solved in an hour or so ;-)

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work
attiks’s picture

Status: Needs work » Needs review
FileSize
1.03 KB
69.74 KB

new patch

attiks’s picture

core committers, please give credit to Jelle_S as well

Bojhan’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

Status: Reviewed & tested by the community » Needs work

Here's a first pass through. My main concern with this patch is there appears to be a lot of logic here to handle overriding and reverting breakpoints and breakpoint groups, which is something that should be handled by the respective CMI issues, not handled as a one-off - especially given those features aren't used anywhere in the patch. #1497268: Add revert functionality to Config entities is one example. It looks though like this could be removed from the patch without breaking everything (since it's not actually used).

+++ b/core/modules/breakpoint/breakpoint.moduleundefined
@@ -0,0 +1,399 @@
+function breakpoint_enable() {
+  $themes = list_themes();
+  breakpoint_themes_enabled(array_keys($themes));
+  $modules = module_list();
+  breakpoint_modules_enabled(array_keys($modules));

Please factor the hook implementations out into helper functions rather than calling them directly from another hook implementation.

+++ b/core/modules/breakpoint/breakpoint.moduleundefined
@@ -0,0 +1,399 @@
+      $message = t('The breakpoints from %label are imported.', array(
+        '%label' => $label,
+      ));
+      if (module_exists('breakpoint_ui') && $uri) {
+        $message .= '<p>' . l(t('A new breakpoint group is created for %label.', array(
+          '%label' => $label,
+        )), $uri, $uri_options);
+      }
+      drupal_set_message($message, 'status');

breakpoint_ui isn't included in the patch, why reference it?

Also the message doesn't look very useful for me, could be logged to watchdog instead if at all?

+++ b/core/modules/breakpoint/breakpoint.moduleundefined
@@ -0,0 +1,399 @@
+      // Breakpoints is mandatory, extra check since this is coming from config.
+      if (isset($data['breakpoints']) && !empty($data['breakpoints'])) {
+        if ($breakpoint_group = BreakpointGroup::ImportBreakpointGroup($source, $source_type, $id, isset($data['label']) ? $data['label'] : drupal_ucfirst($data[$id]), $data['breakpoints'])) {
+          $breakpoint_group->save();
+        }
+      }
+    }
+  }

Why's the check required when this comes from config? And why does this fail silently if the check fails?

+++ b/core/modules/breakpoint/breakpoint.moduleundefined
@@ -0,0 +1,399 @@
+/**
+ * Remove breakpoints from all disabled themes or uninstalled modules.
+ * The source type has to match the original source type, otherwise the group
+ * will not be deleted.

The summary isn't one line.

More importantly, is it OK to remove breakpoints from disabled themes? At the moment you can still use them (like disabled admin themes, although maybe this was fixed recently).

+++ b/core/modules/breakpoint/breakpoint.moduleundefined
@@ -0,0 +1,399 @@
+ * The source type has to match the original source type, otherwise the group

Why does it have to match?

+++ b/core/modules/breakpoint/breakpoint.moduleundefined
@@ -0,0 +1,399 @@
+ * @param array $ids

breakpoint groups?

+++ b/core/modules/breakpoint/breakpoint.moduleundefined
@@ -0,0 +1,399 @@
+function _breakpoint_delete_breakpoints($ids, $source_type) {
+  $breakpoint_groups = entity_load_multiple('breakpoint_group', $ids);
+  foreach ($breakpoint_groups as $breakpoint_group) {
+    if ($breakpoint_group->sourceType == $source_type) {
+      // Delete the automatically created breakpoint group.
+      $breakpoint_group->delete();
+
+      // Get all breakpoints defined by this theme/module.
+      $breakpoint_ids = drupal_container()->get('config.storage')->listAll('breakpoint.breakpoint.' . $source_type . '.' . $breakpoint_group->id() . '.');
+      $entity_info = entity_get_info('breakpoint');
+
+      // Remove the breakpoint.breakpoint part of the breakpoint identifier.
+      foreach ($breakpoint_ids as &$breakpoint_id) {
+        $breakpoint_id = drupal_substr($breakpoint_id, drupal_strlen($entity_info['config prefix']) + 1);
+      }
+      $breakpoints = entity_load_multiple('breakpoint', $breakpoint_ids);
+
+      // Make sure we only delete breakpoints defined by this theme/module.
+      foreach ($breakpoints as $breakpoint) {
+        if ($breakpoint->sourceType == $source_type && $breakpoint->source = $breakpoint_group->id) {
+          $breakpoint->delete();
+        }
+      }
+    }

This hunk is very confusing.

There's breakpoint_group config entities. breakpoint config entities, as well as raw interaction with the config storage. What's the raw config storage calls for?

+++ b/core/modules/breakpoint/breakpoint.moduleundefined
@@ -0,0 +1,399 @@
+  // Delete groups defined by a module/theme even if that module/theme didn't
+  // define any breakpoints.
+  foreach ($ids as $id) {
+    // Delete all breakpoint groups defined by the theme or module.
+    _breakpoint_delete_breakpoint_groups($id, $source_type);

The summary of the function should mention this detail - it'll also delete breakpoint groups, not just breakpoints.

+++ b/core/modules/breakpoint/breakpoint.moduleundefined
@@ -0,0 +1,399 @@
+/**
+ * Remove breakpoint groups from all disabled themes or uninstalled modules.
+ *
+ * @param array $group_id
+ *   Identifier of the breakpoint group.
+ * @param string $sourceType
+ *   Either Breakpoint::SOURCE_TYPE_THEME or Breakpoint::SOURCE_TYPE_MODULE.
+ */

Why is only one $group_id passed in, but the function removes multiple groups?

+++ b/core/modules/breakpoint/breakpoint.moduleundefined
@@ -0,0 +1,399 @@
+  foreach ($breakpoint_groups as $breakpoint_group) {
+    if ($breakpoint_group->sourceType == $source_type && $breakpoint_group->source == $group_id) {
+      $breakpoint_group->delete();

No delete multiple?

+++ b/core/modules/breakpoint/breakpoint.moduleundefined
@@ -0,0 +1,399 @@
+/**
+ * Reload breakpoint groups as they were defined in the theme.
+ *
+ * @param string $theme_key
+ *   The name of the theme.
+ *
+ * @return Drupal\breakpoint\BreakpointGroup|false
+ *   Returns a BreakpointGroup containing the breakpoints defined by the theme.
+ */

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/BreakpointGroup.phpundefined
@@ -0,0 +1,347 @@
+class BreakpointGroup extends ConfigEntityBase {

This function is only called from one place - BreakpointGroup::revert(). And BreakpointGroup::revert() is only called by tests.

1. Why not inline it?

2. Why not do the extra cache clearing in the tests themselves?

(also, see below)

+++ b/core/modules/breakpoint/breakpoint.moduleundefined
@@ -0,0 +1,399 @@
+function breakpoint_get_theme_media_queries($theme_key) {
+  $themes = list_themes();
+  if (!isset($themes[$theme_key])) {
+    return array();

Why fail silently if an incorrect theme_key is passed. Also won't the function do that anyway since config() won't return anything?

+++ b/core/modules/breakpoint/breakpoint.moduleundefined
@@ -0,0 +1,399 @@
+/**
+ * Get a list of available breakpoints from a specified module.
+ *
+ * @param string $module
+ *   The name of the module.
+ *
+ * @return array
+ *   An array of breakpoints in the form $breakpoint['name'] = 'media query'.
+ */
+function breakpoint_get_module_media_queries($module) {
+  if (!module_exists($module)) {
+    return array();
+  }

Same question as $theme_key above.

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.phpundefined
@@ -0,0 +1,391 @@
+  /**
+   * Denotes that a breakpoint or breakpoint group is defined by a theme.
+   */
+  const SOURCE_TYPE_THEME = 'theme';
+
+  /**
+   * Denotes that a breakpoint or breakpoint group is defined by a module.
+   */

Why is this distinction important here? Also, given that theme or module-defined breakpoints should be present in default configuration, couldn't this be inferred?

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.phpundefined
@@ -0,0 +1,391 @@
+    if (empty($this->label)) {
+      $this->label = drupal_ucfirst($this->name);

Why the ucfirst call?

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.phpundefined
@@ -0,0 +1,391 @@
+    // Remove ununsed multipliers.

Typo: ununsed.

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.phpundefined
@@ -0,0 +1,391 @@
+    // Always add '1x' multiplier.
+    if (!array_key_exists('1x', $this->multipliers)) {
+      $this->multipliers = array('1x' => '1x') + $this->multipliers;

Can this array key legitimately be NULL? If not we could just use isset().

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.phpundefined
@@ -0,0 +1,391 @@
+  /**
+   * Get config name.
+   *
+   * @return string
+   */
+  public function getConfigName() {
+    return $this->sourceType . '.' . $this->source . '.' . $this->name;

I guess this explains why the module vs. theme vs. custom type.

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.phpundefined
@@ -0,0 +1,391 @@
+  /**
+   * Override a breakpoint and save it.
+   *
+   * @return Drupal\breakpoint\Breakpoint|false
+   */
+  public function override() {
+    // Custom breakpoint can't be overridden.
+    if ($this->overridden || $this->sourceType === Breakpoint::SOURCE_TYPE_CUSTOM) {
+      return FALSE;
+    }
+
+    // Mark breakpoint as overridden.
+    $this->overridden = TRUE;
+    $this->originalMediaQuery = $this->mediaQuery;
+    $this->save();
+    return $this;

What's the override functionality for? This looks a lot like some of the issues listed in #1790610: [META] Ensure the Configuration and ConfigEntity systems fully support Views CRUD and status operations - if that's the case it should be removed since CMI should either take care of it, or it'll be a missing feature in 8.x, but we shouldn't be introducing one-off implementations for each ConfigEntity that gets added.

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.phpundefined
@@ -0,0 +1,391 @@
+  /**
+   * Is the breakpoint editable.
+   *
+   * @return boolean
+   */
+  public function isEditable() {
+    // Custom breakpoints are always editable.
+    if ($this->sourceType == Breakpoint::SOURCE_TYPE_CUSTOM) {
+      return TRUE;
+    }
+    // Overridden breakpoints are editable.
+    if ($this->overridden) {
+      return TRUE;
+    }
+    return FALSE;

This all looks unnecessary again for an initial patch.

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.phpundefined
@@ -0,0 +1,391 @@
+  /**
+   * Check if a mediaQuery is valid.
+   *
+   * @throws Drupal\breakpoint\InvalidBreakpointMediaQueryException
+   *

Could this be generically useful in case another module wants to validate media queries? I didn't review the function itself. I did do a quick google search for a breakpoint validator and was only able to find the W3C's.

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/BreakpointGroup.phpundefined
@@ -0,0 +1,347 @@
+   */
+  public static function ImportMediaQueries($id, $label, $source_type, $media_queries) {
+    $breakpoint_group = entity_load('breakpoint_group', $source_type . '.' . $id);
+    if (!$breakpoint_group) {
+      // Build a new breakpoint group.
+      $breakpoint_group = entity_create('breakpoint_group', array(
+        'id' => $id,
+        'label' => $label,
+        'source' => $id,
+        'sourceType' => $source_type,
+      ));
+    }
+    else {
+      // Reset label.
+      $breakpoint_group->label = $label;
+    }
+
+    foreach ($media_queries as $name => $media_query) {
+      $breakpoint_group->addBreakpointFromMediaQuery($name, $media_query);
+    }
+    return $breakpoint_group;
+  }
+
+  /**
+   * Import breakpoint groups from theme or module.
+   *
+   * @param string $source
+   *   Source of the breakpoint group: theme_key or module name.
+   * @param string $sourceType
+   *   Either Breakpoint::SOURCE_TYPE_THEME or Breakpoint::SOURCE_TYPE_MODULE.
+   * @param string $id
+   *   Identifier of the breakpoint group.
+   * @param string $label
+   *   Human readable name of the breakpoint group.
+   * @param array $breakpoints
+   *   Array of breakpoints, using either the short name or the full name.
+   *
+   * @return \Drupal\breakpoint\BreakpointGroup|false
+   *   Return the new breakpoint group containing all breakpoints.
+   */
+  public static function ImportBreakpointGroup($source, $source_type, $id, $label, $breakpoints) {

Why are these static functions on the breakpoint_group config entity? The calls to entity_load() and entity_create() inside here don't feel good in here.

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/BreakpointGroup.phpundefined
@@ -0,0 +1,347 @@
+  /**
+   * Load all breakpoints, remove non-existing ones.
+   *
+   * @return array
+   *   Array containing breakpoints keyed by their id.
+   */
+  protected function loadAllBreakpoints() {
+    $breakpoints = $this->breakpoints;
+    $this->breakpoints = array();
+    foreach ($breakpoints as $breakpoint_id) {
+      $breakpoint = breakpoint_load($breakpoint_id);
+      if ($breakpoint) {
+        $this->breakpoints[$breakpoint_id] = $breakpoint;
+      }
+    }

Why would a breakpoint group include breakpoints that don't exist?

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/InvalidBreakpointException.phpundefined
@@ -0,0 +1,15 @@
+
+use RuntimeException;
+

Global classes like this can just be \RuntimeException without the use statement now.

catch’s picture

Sorry for multiple posts.

Module is also missing a hook_help(), that's need for any new core module.

attiks’s picture

#130 I'll add that

1/ About override/reverting: I'll agree that it would be nice if it gets added to ConfigEntityBase, and you're right it isn't needed for this patch. But it is needed because site users will need to be able to alter breakpoints defined by a theme. I would rather add it now and remove it once CMI has everything sorted out. If not the adding of the breakpoint UI will be delayed as well as adding picture.

2/ "Why's the check required when this comes from config" because it's config coming from a user created file, if they leave out the breakpoints key, this will blow-up. If we remove the check we'll assume that config files are always correct. If there's a better way to handle this, I'm all ears.

3/ "remove breakpoints from disabled themes", if #1067408: Themes do not have an installation status gets committed it will behave the same as modules. If we don't remove them, you'll end up with a lot of breakpoints and there's no longer a way to delete them.

4/ "Why does it have to match?" because a breakpoint group can contain breakpoints from different sources, but while deleting the group, you can only delete the breakpoints defined by that group.

5/ "This hunk is very confusing" the config call is to load all breakpoints defined by a certain theme or module; but could use config_get_storage_names_with_prefix instead

6/ "No delete multiple?" unclear what you mean, care to clarify?

7/ "->isEditable()", true not needed, but it's needed for UI, but since it's part of the object it made more sense in adding it now?

8/ isValidMediaQuery is static, so it can be used by others. I can't think of a use case where somebody wants to validate a media query, but doesn't want to use breakpoint/breakpoint group

9/ ImportMediaQueries used to be a 'helper' function inside breakpoint.module, I'll rewrite it to get rid of the static/entity_create

10/ "Why would a breakpoint group include breakpoints that don't exist?"
Because it got deleted in the mean time, and if a breakpoint gets deleted we don't check all existing breakpoint groups, we only check if the group is used.

I'll work on the code, will post an update later today

tim.plunkett’s picture

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.phpundefined
@@ -0,0 +1,391 @@
+   * The breakpoint ID (config name).
+   *
+   * @var string
+   */
+  public $id;
...
+  /**
+   * The breakpoint name (machine name).
+   *
+   * @var string
+   */
+  public $name;

What's the difference?

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.phpundefined
@@ -0,0 +1,391 @@
+  /**
+   * Overrides Drupal\config\ConfigEntityBase::__construct().
+   */
+  public function __construct(array $values = array(), $entity_type = 'breakpoint') {
+    parent::__construct($values, $entity_type);

How is this helpful?

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.phpundefined
@@ -0,0 +1,391 @@
+  /**
+   * Get config name.
+   *
+   * @return string
+   */
+  public function getConfigName() {
+    return $this->sourceType . '.' . $this->source . '.' . $this->name;

This should explain why it needs a name other than $this->id()

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.phpundefined
@@ -0,0 +1,391 @@
+   * @return boolean
...
+   * @return true

bool, missing a description

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/BreakpointGroup.phpundefined
@@ -0,0 +1,347 @@
+  public function __construct(array $values = array(), $entity_type = 'breakpoint_group') {

Also, this is weird.

In addition, almost all of the method one-liners don't have an 's', and the @param/@return are all missing descriptions.

catch’s picture

1/ About override/reverting: I'll agree that it would be nice if it gets added to ConfigEntityBase, and you're right it isn't needed for this patch. But it is needed because site users will need to be able to alter breakpoints defined by a theme. I would rather add it now and remove it once CMI has everything sorted out. If not the adding of the breakpoint UI will be delayed as well as adding picture.

Is there an issue for the breakpoint UI? Does picture really depend on that? I'm quite strongly against adding one-off implementations of these features when there's already existing issues trying to deal with them generically - afaik the views in core patch removed a lot of that code to put it in back later (although I've not reviewed that 2mb patch yet).

attiks’s picture

Status: Needs review » Needs work

#133: #1775302: Do a UX review of Breakpoint module and #1775530: Move picture into core

I'll remove them.

Does picture need breakpoint UI? Yes, because setting breakpoints is site specific. Also layouts & blocks is normally going to use breakpoints as well (for the gridbuilder)

attiks’s picture

Status: Needs work » Needs review
FileSize
43.82 KB
60.38 KB

New patch with enable/disable, override/revert and normally all comments addressed.

attiks’s picture

Status: Needs work » Needs review

note to self: add hook_help

attiks’s picture

Proposal for hook_help:

About

The Breakpoint module allows the management of breakpoints and breakpoint groups for responsive designs.

Uses

Breakpoints
Breakpoints can be defined by themes or other modules, a breakpoints consist of a name and a media query.
Breakpoint groups
Breakpoints can be organized into breakpoint groups, so that it is easier to manage and use them.
Multipliers
Multipliers can be defined for each breakpoint and are needed to handle screens with high dpi.
attiks’s picture

FileSize
76.83 KB

New patch including hook_help from #137 we can always change the text in a follow up.

Jelle_S’s picture

core committers, please give credit to Jelle_S as well

Thanks! =D

+++ b/core/modules/breakpoint/breakpoint.moduleundefined
@@ -0,0 +1,555 @@
+      $output .= '<dd>' . t('Breakpoints can be defined by themes or other modules, a breakpoints consist of a name and a media query.') . '</dd>';

One small remark: a breakpoints -> a breakpoint.

Not sure if I should put this back to needs work for this? This minor change might be done when committing it?

But I'm not the right person to put this on RTBC I guess ;-)

webchick’s picture

Nah, one of the core committers can clean that up on commit if that's the only thing wrong with it. :)

Status: Needs review » Needs work

The last submitted patch, breakpoint-138.patch, failed testing.

attiks’s picture

Status: Needs work » Needs review
FileSize
1.39 KB
61.44 KB

Getting late :/

jessebeach’s picture

Issue tags: +Dynamic layouts

Added "Dynamic layouts" tag.

JohnAlbin’s picture

+++ b/core/modules/breakpoint/breakpoint.module
@@ -0,0 +1,423 @@
+      $output .= '<dd>' . t('Breakpoints can be defined by themes or other modules, a breakpoints consist of a name and a media query.') . '</dd>';

Should be “a breakpoint consists of”…

+++ b/core/modules/breakpoint/breakpoint.module
@@ -0,0 +1,423 @@
+      $output .= '<dd>' . t('Breakpoints can be organized into breakpoint groups, so that it is easier to manage and use them.') . '</dd>';

Correct me if I'm wrong, but its not that breakpoints _can_ be organized into groups, its that they _are_ organized into groups. “Breakpoints are organized into breakpoint groups so that they are easier to manage and use.”

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.php
@@ -0,0 +1,283 @@
+    // Build an id if non is set.

"none" not "non".

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.php
@@ -0,0 +1,283 @@
+  public static function isValidMediaQuery($media_query) {
+    $media_features = array(
+      'width' => 'length', 'min-width' => 'length', 'max-width' => 'length',
+      'height' => 'length', 'min-height' => 'length', 'max-height' => 'length',
+      'device-width' => 'length', 'min-device-width' => 'length', 'max-device-width' => 'length',
+      'device-height' => 'length', 'min-device-height' => 'length', 'max-device-height' => 'length',
+      'orientation' => array('portrait', 'landscape'),
+      'aspect-ratio' => 'ratio', 'min-aspect-ratio' => 'ratio', 'max-aspect-ratio' => 'ratio',
+      'device-aspect-ratio' => 'ratio', 'min-device-aspect-ratio' => 'ratio', 'max-device-aspect-ratio' => 'ratio',
+      'color' => 'integer', 'min-color' => 'integer', 'max-color' => 'integer',
+      'color-index' => 'integer', 'min-color-index' => 'integer', 'max-color-index' => 'integer',
+      'monochrome' => 'integer', 'min-monochrome' => 'integer', 'max-monochrome' => 'integer',
+      'resolution' => 'resolution', 'min-resolution' => 'resolution', 'max-resolution' => 'resolution',
+      'scan' => array('progressive', 'interlace'),
+      'grid' => 'integer',
+    );

I can see there's a lot of thought put into the isValidMediaQuery method. But is there any technical reason we need to validate the media queries? I immediately see a problem that there are no vendor-prefixed media features. If we can get by without validating the media features, then we simply free up our codebase from trying to chase both the CSS spec and browser vendor experimentation.

Architecturally, this looks very, very good. My issues are pretty minor. :-D

attiks’s picture

#144 Thanks for reviewing, regarding the validation: i think it's needed, we will have site administrators changing media queries, if we don't validate them it means the browser is going to skip them. Consequence is that we get a wrong picture output, and - assuming blocks & layouts will use breakpoints - a messed up page layout.

attiks’s picture

#144 comments fixed

JohnAlbin’s picture

Ok, but you haven't addressed:

I immediately see a problem that there are no vendor-prefixed media features.

There's -webkit-min-device-pixel-ratio and -o-min-device-pixel-ratio, etc., etc. Wouldn't validation fail if a user tries to use those? And, again, what about future CSS media query types?

attiks’s picture

I changed the validation part, unrecognized features are allowed, we only check if they contain only valid characters. Only recognized features are checked so make sure they contain valid values.

Status: Needs review » Needs work
Issue tags: -Usability, -mobile, -frontend performance, -media queries, -Responsive Design, -d8mux, -Dynamic layouts

The last submitted patch, breakpoint-148.patch, failed testing.

attiks’s picture

Status: Needs work » Needs review
Issue tags: +Usability, +mobile, +frontend performance, +media queries, +Responsive Design, +d8mux, +Dynamic layouts

#148: breakpoint-148.patch queued for re-testing.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

All review comments since RTBC were addressed. John agrees it looks good architecturally, so let's get this in!

webchick’s picture

Status: Reviewed & tested by the community » Needs work

First of all, frigging fantastic work with this. The API is very clear, the tests are very thorough, and you've really done an amazing job with this. It's very close.

Since this is a big patch, what follows is a big review. I really hope this does not discourage you. A lot of my feedback falls under the following headings:

- Improved documentation/naming
- Coding standards/conventions
- Code streamlining
- Kinda weird stuff that just doesn't really fly

I normally don't hold up features over the first two, but there are a lot of them, so i feel uncomfortable committing without at least one more go-around. I'm willing to hear some push-back if you feel some of these requests are unreasonable.

Heeeeere goes...

index fff63c1..d937411 100644
--- a/core/MAINTAINERS.txt

--- a/core/MAINTAINERS.txt
+++ b/core/MAINTAINERS.txtundefined

@@ -173,6 +173,9 @@ Block module
+Breakpoint module
+- Peter Droogmans 'attiks' http://drupal.org/user/105002

A new module that comes with its own MAINTAINERS.txt entry. I like! :D

+++ b/core/modules/breakpoint/breakpoint.moduleundefined
@@ -0,0 +1,423 @@
+function breakpoint_help($path, $arg) {

Yay for hook_help(). However, this needs a bit of work. Remember, this is the page people are going to go to when they want to know what in the heck this module is about. We can't assume they have too much pre-existing knowledge.

+++ b/core/modules/breakpoint/breakpoint.moduleundefined
@@ -0,0 +1,423 @@
+      $output .= '<p>' . t('The Breakpoint module allows the management of breakpoints and breakpoint groups for responsive designs.') . '</p>';

A little more detail here would not go amiss. I don't think knowledge of what the heck a "breakpoint" is is as wide-spread as it might seem. Can you add a sentence or so of explanation, and why they're important?

+++ b/core/modules/breakpoint/breakpoint.moduleundefined
@@ -0,0 +1,423 @@
+      $output .= '<dd>' . t('Breakpoints can be defined by themes or other modules, a breakpoints consists of a name and a media query.') . '</dd>';

Can you provide an example of a media query?

+++ b/core/modules/breakpoint/breakpoint.moduleundefined
@@ -0,0 +1,423 @@
+      $output .= '<dd>' . t('Multipliers can be defined for each breakpoint and are needed to handle screens with high dpi.') . '</dd>';

"DPI" ... but I have no idea what this sentence means. :) Can we try and explain it as though the person reading it is kind of a dummy when it comes to mobile?

+++ b/core/modules/breakpoint/breakpoint.moduleundefined
@@ -0,0 +1,423 @@
+function breakpoint_enable() {
...
+function breakpoint_themes_enabled($theme_list) {
...
+function breakpoint_themes_disabled($theme_list) {
...
+function breakpoint_modules_enabled($modules) {
...
+function breakpoint_modules_uninstalled($modules) {

Good lord, that's an awful lot of shenanigans you have to do here to keep config in sync with module/theme changes. It would be nice if the configuration system handled this itself. :( Can you briefly pow-wow with the CMI folks about this and see if we need some @todos about removing these hook implementations?

+++ b/core/modules/breakpoint/breakpoint.moduleundefined
@@ -0,0 +1,423 @@
+function _breakpoint_import_media_queries($group_name, $label, $source_type, $media_queries) {
...
+function _breakpoint_import_breakpoint_groups($source, $source_type) {
...
+function _breakpoint_delete_breakpoints($list, $source_type) {
...
+function _breakpoint_delete_breakpoint_groups($group_id, $source_type) {

Is there a reason these are prefixed with an underscore/considered "internal"? They seem like they would be useful general API functions.

+++ b/core/modules/breakpoint/breakpoint.moduleundefined
@@ -0,0 +1,423 @@
+function breakpoint_get_theme_media_queries($theme_key) {
+  $themes = list_themes();
+  if (!isset($themes[$theme_key])) {
+    throw new \Exception('Illegal theme_key passed.');
+  }
+
+  $config = config($theme_key . '.breakpoints');
+  if ($config) {
+    return $config->get();
+  }
+  return array();
...
+function breakpoint_get_module_media_queries($module) {
+  if (!module_exists($module)) {
+    throw new \Exception('Illegal module name passed.');
+  }
+
+  $config = config($module . '.breakpoints');
+  if ($config) {
+    return $config->get();
+  }
+  return array();

There are a lot of these types of functions that are 99% the same except that they do a module_exists() instead of a isset($themes[$theme_key]). I wonder if it's worth a wrapper function that can handle the "is it a module or is it a theme?" question and dispatch accordingly.

+++ b/core/modules/breakpoint/breakpoint.moduleundefined
@@ -0,0 +1,423 @@
+ * @todo Remove this in a follow-up issue.
+ * @see http://drupal.org/node/1798214
+ */
+function breakpoint_group_load($id) {
+  return entity_load('breakpoint_group', $id);
+}
...
+ * @todo Remove this in a follow-up issue.
+ * @see http://drupal.org/node/1798214
+ */
+function breakpoint_load($id) {
+  return entity_load('breakpoint', $id);
+}

I don't get this. Why would we want to remove what looks like a perfectly handy function?

The referenced issue seems to be solely about router arguments, but I can think of other scenarios where these load() functions would be useful.

+++ b/core/modules/breakpoint/breakpoint.moduleundefined
@@ -0,0 +1,423 @@
+ * Load all breakpoint groups as select options.
...
+function breakpoint_group_labels() {
...
+ * Load all breakpoints as select options.
...
+function breakpoint_labels() {

See, now this is the kind of function I would underscore. And also maybe rename to something more like "breakpoint_[group]_select_options" unless there's anything else you'd possibly use these for outside of that context?

+++ b/core/modules/breakpoint/breakpoint.moduleundefined
@@ -0,0 +1,423 @@
+  foreach ($breakpoints as $breakpoint) {
+    $options[$breakpoint->id()] = $breakpoint->label() . ' (' . $breakpoint->source . ' - ' . $breakpoint->sourceType .   ') [' . $breakpoint->mediaQuery . ']';
+  }
+
+  return $options;

You did an asort() on the options in breakpoint_group_labels(), why not here as well?

+++ b/core/modules/breakpoint/breakpoint.moduleundefined
@@ -0,0 +1,423 @@
+function _breakpoint_group_create_or_load($name, $label, $source, $source_type) {

Hm. Not really a fan of this function. We don't have one like it in any of the other entities in core. What's wrong with calling breakpoint_load() and breakpoint_save()?

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.phpundefined
@@ -0,0 +1,295 @@
+  /**
+   * Denotes that a breakpoint or breakpoint group is defined by the user.
+   */
+  const SOURCE_TYPE_CUSTOM = 'custom';

SOURCE_TYPE_USER_DEFINED would probably be clearer. I wasn't sure if CUSTOM referred to a custom module/theme or customizing YML files locally or what that referred to.

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.phpundefined
@@ -0,0 +1,295 @@
+   *   Allowed values:
+   *     Breakpoint::SOURCE_TYPE_THEME
+   *     Breakpoint::SOURCE_TYPE_MODULE
+   *     Breakpoint::SOURCE_TYPE_CUSTOM

I don't understand SOURCE_TYPE_CUSTOM. When would a breakpoint be coming from not a module or a theme?

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.phpundefined
@@ -0,0 +1,295 @@
+  /**
+   * Duplicates a breakpoint.
+   *
+   * The new breakpoint inherits the media query.
+   *
+   * @return Drupal\breakpoint\Breakpoint
+   */
+  public function duplicate() {
+    return entity_create('breakpoint', array(
+      'mediaQuery' => $this->mediaQuery,
+    ));

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/BreakpointGroup.phpundefined
@@ -0,0 +1,214 @@
+  /**
+   * Duplicates a breakpoint group.
+   *
+   * The new breakpoint group inherits the breakpoints.
+   *
+   * @return Drupal\breakpoint\BreakpointGroup
+   */
+  public function duplicate() {
+    return entity_create('breakpoint_group', array(
+      'breakpoints' => array_keys($this->breakpoints),
+      'name' => 'clone_of_' . $this->name,
+    ));

Is there a use case for these methods outside of unit testing? If not, I wonder if it makes sense to move them to Breakpoint[Group]TestBase instead?

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.phpundefined
@@ -0,0 +1,295 @@
+    $media_features = array(
+      'width' => 'length', 'min-width' => 'length', 'max-width' => 'length',
+      'height' => 'length', 'min-height' => 'length', 'max-height' => 'length',

I do not understand at all what these key => value pairs represent. Can we get a comment that explains it?

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.phpundefined
@@ -0,0 +1,295 @@
+      // Check mediaQuery_list: S* [mediaQuery [ ',' S* mediaQuery ]* ]?
...
+          // Check expression: '(' S* media_feature S* [ ':' S* expr ]? ')' S*
...
+          // Check [ONLY | NOT]? S* media_type
...
+          // Check illegal [ONLY | NOT]? S* media_type

That is Greek to me. :) It looks like a little bit like a cat trompled over your keyboard. :D

I trust that people who can understand regex syntax can just as well read the code below. What we need is a bit of English that explains what the logic is doing...

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.phpundefined
@@ -0,0 +1,295 @@
+                    if (preg_match('/^(\-)?(\d+(?:\.\d+)?)?((?:|em|ex|px|cm|mm|in|pt|pc|deg|rad|grad|ms|s|hz|khz|dpi|dpcm))$/i', trim($value), $length_matches)) {

Could we also get a comment above this one?

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Breakpoint.phpundefined
@@ -0,0 +1,295 @@
+            // We need to allow vendor prefixed media fetures and make sure we
+            // are future proof, so only check allowed characters.
+            if (!preg_match('/^[a-zA-Z0-9\-\\ ]+$/i', trim($query_part), $matches)) {

...like this one!

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/BreakpointGroup.phpundefined
@@ -0,0 +1,214 @@
+   * @param type $breakpoints
+   *   Array containing breakpoints keyed by their id.

Think you're missing a type here. Should be array?

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Tests/BreakpointAPITest.phpundefined
@@ -0,0 +1,94 @@
+  /**
+   * Drupal\simpletest\WebTestBase\getInfo().

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Tests/BreakpointCrudTest.phpundefined
@@ -0,0 +1,57 @@
+  /**
+   * Drupal\simpletest\WebTestBase\getInfo().

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Tests/BreakpointGroupAPITest.phpundefined
@@ -0,0 +1,81 @@
+  /**
+   * Drupal\simpletest\WebTestBase\getInfo().

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Tests/BreakpointGroupCrudTest.phpundefined
@@ -0,0 +1,72 @@
+  /**
+   * Drupal\simpletest\WebTestBase\getInfo().

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Tests/BreakpointGroupTestBase.phpundefined
@@ -0,0 +1,66 @@
+  /**
+   * Drupal\simpletest\WebTestBase\setUp().

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Tests/BreakpointMediaQueryTest.phpundefined
@@ -0,0 +1,125 @@
+  /**
+   * Drupal\simpletest\WebTestBase\getInfo().

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Tests/BreakpointTestBase.phpundefined
@@ -0,0 +1,55 @@
+  /**
+   * Drupal\simpletest\WebTestBase\setUp().

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Tests/BreakpointThemeTest.phpundefined
@@ -0,0 +1,136 @@
+  /**
+   * Drupal\simpletest\WebTestBase\getInfo().
...
+  /**
+   * Drupal\simpletest\WebTestBase\setUp().

Although I intensely dislike this convention, the convention is nontheless to NOT document getInfo() and setUp() in test classes. :\

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Tests/BreakpointCrudTest.phpundefined
@@ -0,0 +1,57 @@
+  public function testBreakpointCrud() {

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Tests/BreakpointGroupCrudTest.phpundefined
@@ -0,0 +1,72 @@
+  public function testBreakpointGroupCrud() {

(nitpick) Since CRUD is an acronym, the word should be in ALL CAPS in the function name.

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Tests/BreakpointGroupAPITest.phpundefined
@@ -0,0 +1,81 @@
+    $this->assertTrue($exception, t('An exception is thrown when an invalid sourceType is entered.'));
...
+    $this->assertTrue($exception, t('An exception is thrown when an invalid source is entered.'));
...
+    $this->assertFalse($exception, t('No exception is thrown when a valid data is passed.'));

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Tests/BreakpointGroupCrudTest.phpundefined
@@ -0,0 +1,72 @@
+    $this->assertFalse(entity_load('breakpoint_group', $group->id()), t('breakpoint_group_load: Loading a deleted breakpoint group returns false.'), t('Breakpoints API'));

(everywhere)

There should not be t()s around assertion messages.

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Tests/BreakpointMediaQueryTest.phpundefined
@@ -0,0 +1,125 @@
+    $media_queries = array(
+      // Bartik breakpoints.
+      '(min-width: 0px)',
+      'all and (min-width: 560px) and (max-width:850px)',
+      'all and (min-width: 851px)',
+      // Seven breakpoints.
+      '(min-width: 0em)',
+      'screen and (min-width: 40em)',
+      // Stark breakpoints.
+      '(min-width: 0px)',
+      'all and (min-width: 480px) and (max-width: 959px)',
+      'all and (min-width: 960px)',

A little concerned here. Do we need to keep this constantly up to speed with what these themes are doing? If so, is there a way to read in these values dynamically?

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Tests/BreakpointMediaQueryTest.phpundefined
@@ -0,0 +1,125 @@
+      '(orientation)',
+      'all and (orientation)',
+      'not all and (orientation)',
+      'only all and (orientation)',
+      'screen and (width)',
+      'screen and (width: 0)',
+      'screen and (width: 0px)',
+      'screen and (width: 0em)',
+      'screen and (min-width: -0)',
+      'screen and (max-width: 0)',
+      'screen and (max-width: 0.3)',
+      'screen and (min-width)',

This chunk does not seem to be about Stark, so probably needs its own comment heading.

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Tests/BreakpointMediaQueryTest.phpundefined
@@ -0,0 +1,125 @@
+      $this->assertTrue(Breakpoint::isValidMediaQuery($media_query), $media_query . ' is valid.');
...
+        $this->assertFalse(Breakpoint::isValidMediaQuery($media_query), $media_query . ' is not valid.');
...
+        $this->assertTrue(TRUE, $media_query . ' is not valid.');

These should be using format_string() rather than concatenation.

+++ b/core/modules/breakpoint/tests/config/breakpoint_theme_test.breakpoint_groups.ymlundefined
@@ -0,0 +1,6 @@
+module_test:
+  label: Test Module

+++ b/core/modules/breakpoint/tests/themes/breakpoint_test_theme/config/breakpoint_test_theme.breakpoint_groups.ymlundefined
@@ -0,0 +1,6 @@
+test:
+  label: Test

Should these be "theme_test" and "Test Theme" for parity?

Jelle_S’s picture

@webchick

+++ b/core/modules/breakpoint/breakpoint.moduleundefined
@@ -0,0 +1,423 @@
+ * @todo Remove this in a follow-up issue.
+ * @see http://drupal.org/node/1798214
+ */
+function breakpoint_group_load($id) {
+  return entity_load('breakpoint_group', $id);
+}
...
+ * @todo Remove this in a follow-up issue.
+ * @see http://drupal.org/node/1798214
+ */
+function breakpoint_load($id) {
+  return entity_load('breakpoint', $id);
+}

I don't get this. Why would we want to remove what looks like a perfectly handy function?

The referenced issue seems to be solely about router arguments, but I can think of other scenarios where these load() functions would be useful.

To clarify: the comment is there because of #57 (and next) where Crell strongly discouraged wrapper functions, but currently we still need them to be able to use %breakpoint and %breakpoint_group as menu arguments

attiks’s picture

Status: Needs review » Needs work

#152 Some feedback

  1. hook_help: this is a first start, and will be expanded by the Breakpoint UI module
  2. Enable/Disable hooks: as discussed on IRC, this is needed so themes/modules can use theme_key.breakpoints.yml to define the breakpoints, if we don't do this themes have to use something like breakpoint.breakpoints.theme.theme_key.mobile.yml (with a more complex syntax) to define their breakpoints. Follow up created #1813100: Allow different naming schema for yaml files
  3. prefixed with an underscore (_breakpoint_import_media_queries, _breakpoint_import_breakpoint_groups, _breakpoint_delete_breakpoints, _breakpoint_delete_breakpoint_groups): These are internal functions, other modules should never call these directly since they are only needed to support the importing of breakpoints defined by themes/modules.
  4. breakpoint_group_load and breakpoint_load, as Jelle_S said
  5. breakpoint_group_labels was called breakpoint_group_select_options before, do you want me to rename it back? The labels are sorted because the groups don't have a weight. This function is a helper for modules building on top of breakpoint (Breakpoint UI and Picture).
  6. breakpoint_labels was called breakpoint_select_options before, do you want me to rename it back? The labels are not sorted because breakpoints are ordered by weight. This function is a helper for modules building on top of breakpoint (Breakpoint UI and Picture).
  7. duplicate isn't used right now, I'll remove it
  8. the convention is nontheless to NOT document getInfo() and setUp() in test classes: a shame indeed, but I'll remove them.
  9. Do we need to keep this constantly up to speed with what these themes are doing: no, but it would be a good idea to do so, so we're sure that the media queries are valid.

I'll change the other things in code, thanks for reviewing.

attiks’s picture

There are a lot of these types of functions that are 99% the same except that they do a module_exists() instead of a isset($themes[$theme_key]). I wonder if it's worth a wrapper function that can handle the "is it a module or is it a theme?" question and dispatch accordingly.

Follow up created #1813110: Introduce a function "is it a module or is it a theme?"

attiks’s picture

Status: Needs work » Needs review
FileSize
28.8 KB
61.73 KB

New patch:

  • duplicate is removed
  • breakpoint_group_labels -> breakpoint_group_select_options
  • breakpoint_labels -> breakpoint_select_options + sort added
  • _breakpoint_group_create_or_load is still there, it's just aninternal helper function.
  • more/better comments
  • crud -> CRUD
  • no more t()
attiks’s picture

Status: Needs work » Reviewed & tested by the community

Back to RTBC

webchick’s picture

Title: Move breakpoint module into core » Change notice: Move breakpoint module into core
Category: feature » task
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Awesome. The interdiff looks great. AND, as of about an hour ago we're back under thresholds again! :D

The one thing I didn't want to let go of in my review was hook_help() so I asked attiks for another couple of sentences, but instead he and Bojhan and jessebeach and jhodgdon and I don't know who else collaborated together on a really nice help page that explains things very well in terms mere mortals can understand.

So I've updated the patch with that text, which was:

"
About

The Breakpoints module keeps track of the height, width, and resolution breakpoints where a responsive design needs to change in order to respond to different devices being used to view the site.

Uses

Defining breakpoints

The Breakpoint module can be used by themes and other modules to define breakpoints, which separate the height or width of viewports (screens, printers, and other media output types) into steps. For instance, a width breakpoint of 40em creates two steps: one for widths up to 40em and one for widths above 40em. Breakpoints can be used to define when layouts should shift from one form to another, when images should be resized, and other changes that need to respond to changes in viewport height or width.

Media queries are a formal way to encode breakpoints. For instance, a width breakpoint at 40em would be written as the media query '(min-width: 40em)'. Breakpoints are really just media queries with some additional meta-data, such as a name and multiplier information.

Assigning resolution multipliers to breakpoints

Multipliers are a measure of the viewport's device resolution, defined to be the ratio between the physical pixel size of the active device and the device-independent pixel size. The Breakpoint module defines multipliers of 1, 1.5, and 2; when defining breakpoints, modules and themes can define which multipliers apply to each breakpoint.

Defining breakpoint groups

Breakpoints can be organized into groups. Modules and themes should use groups to separate out breakpoints that are meant to be used for different purposes, such as breakpoints for layouts or breakpoints for image sizing.
"

And for the first time ever, I kinda understand what a breakpoint, media query, and multiplier are, now. :)

Soooooooo.... *drumrolllllllll*

Committed and pushed to 8.x. YEAH! :D (with that hook_help() fix) GREAT work on this, attiks, and everyone else!!

Marking for change notice. A quick turnaround on this would be appreciated, since it now blocks other features.

webchick’s picture

Note that in my original commit message given all the excitement, I totally spaced crediting Jelle_S, so I reverted it and made a second commit which did this (and also Gábor since he seemed to do a lot of reviews). Sorry about the mess. git commit --amend doesn't work once things have been pushed. :(

attiks’s picture

I've created http://drupal.org/node/1813914, but since this is my first change notice I would love an extra pair of eyes.

Thanks to all for helping moving this forward.

Time to finalize #1775530: Move picture into core

Gábor Hojtsy’s picture

Category: task » feature
Priority: Critical » Major
Status: Active » Fixed
Issue tags: -Needs change record

I'm implementing this breakpoint system for Spark layouts in D8 contrib now, and I understand the change notice especially with the more information link pointing to http://drupal.org/node/1803874 there :) Thanks all!

Gábor Hojtsy’s picture

Title: Change notice: Move breakpoint module into core » Move breakpoint module into core
Gábor Hojtsy’s picture

Initial integration with gridbuilder module for the curious: http://drupalcode.org/project/gridbuilder.git/commitdiff/fa2748a329ff5d0...

MustangGB’s picture

As one of the aforementioned curious, I will be having a play with this when time allows.
Great job everyone!

eigentor’s picture

@gabor: trying to create a D8 theme today I discovered the mythemename.breakpoints.yml is required. Else I got a "YourthemeXY could not be found" though the Screenshot was shown. I was trying to declare the theme a Stark subtheme, did not check without being a subtheme.

Is it intentional that the .yml file (I guess inside the config directory) is needed? If so I guess it needs documentation.

dcrocks’s picture

A question. Will themes inherit the breakpoint settings of (multiple?)base themes?

attiks’s picture

#165 I just tested this, I removed bartik.breakpoints.yml and stark.breakpoints.yml and installed Drupal 8 from scratch without any problems. I enabled the stark theme and it works.

I created a new subtheme (just an theme.info file) and tried enabling it, I got the same error. So I cleared my cache and tried again, it worked. Can you create a new issue since this is a regression. BTW: this has nothing to do with theme.breakpoints.yml.

#166They don't inherit the breakpoints from the sub theme, can you open a new issue so we can keep track of it.

Link the issues back to this and link this issue to the new ones, thanks.

JohnAlbin’s picture

Assigned: JohnAlbin » Unassigned
dcrocks’s picture

I have created an issue #1828606: Make BREAKPOINT resources inheritable to discuss breakpoints and their inheritance..

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.