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
- Work on breakpoints UI. See also #1775302: Do a UX review of Breakpoint module
- Add picture element, see #1775530: Move picture into core
Original report by [Jeff Burnz]
see #1170478: Responsive Images for the background discussion
Related discussions
Comment | File | Size | Author |
---|---|---|---|
#156 | breakpoint-156.patch | 61.73 KB | attiks |
#156 | interdiff_156_148.txt | 28.8 KB | attiks |
#148 | breakpoint-148.patch | 62.38 KB | attiks |
#148 | interdiff_148_146.txt | 3.47 KB | attiks |
#146 | breakpoint-146.patch | 61.44 KB | attiks |
Comments
Comment #1
attiks CreditAttribution: attiks commentedtagging
Comment #2
attiks CreditAttribution: attiks commentedPatch to add breakpoints definition to Bartik theme. Breakpoints are only read at the time a theme or the breakpoint module is enabled.
Comment #3
Bojhan CreditAttribution: Bojhan commentedIt might make sense if this issue follows the issue template, I have a hard time following the action points.
Comment #4
jessebeach CreditAttribution: jessebeach commented@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.
Comment #5
attiks CreditAttribution: attiks commented@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
Comment #6
attiks CreditAttribution: attiks commented@Bojhan you're completely right, will try to write something by Monday.
Comment #6.0
attiks CreditAttribution: attiks commentedUpdated issue summary.
Comment #6.1
attiks CreditAttribution: attiks commentedUpdated issue summary.
Comment #7
attiks CreditAttribution: attiks commentedMockups (balsamiq) of the breakpoints module so
Breakpoints are essentially media queries that can be defined by
User roles involved:
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.
Comment #8
aspilicious CreditAttribution: aspilicious commentedIn drupal 8 shouldn't this be a config file? $theme.breakpoints.yml
(yes themes can have a config file)
Comment #9
attiks CreditAttribution: attiks commented@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.
Comment #10
aspilicious CreditAttribution: aspilicious commentedEverything 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)
Comment #11
attiks CreditAttribution: attiks commentedNo database used, everything is stored in config
Comment #12
Wim Leers#10: I don't think we're moving "settings" from module/theme info files into config files?
Comment #13
attiks CreditAttribution: attiks commented#12 If I understand you correctly you're saying breakpoints should use database tables to store the config?
Comment #14
aspilicious CreditAttribution: aspilicious commentedWe 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.
Comment #15
attiks CreditAttribution: attiks commentedfrom IRC:
Comment #16
RobW CreditAttribution: RobW commentedIf 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.
Comment #17
Wim LeersI'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.
Comment #17.0
Wim LeersUpdated issue summary.
Comment #17.1
attiks CreditAttribution: attiks commentedUpdated issue summary.
Comment #17.2
attiks CreditAttribution: attiks commentedUpdated issue summary.
Comment #18
JohnAlbinDuring 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.
Comment #19
jessebeach CreditAttribution: jessebeach commented@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.
Comment #20
attiks CreditAttribution: attiks commentedFor picture see #1775530: Move picture into core
Comment #20.0
attiks CreditAttribution: attiks commentedUpdated issue summary.
Comment #21
attiks CreditAttribution: attiks commented#18 The UI is going to be reviewed in #1775302: Do a UX review of Breakpoint module
Comment #22
RobW CreditAttribution: RobW commentedIs 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.
Comment #22.0
RobW CreditAttribution: RobW commentedUpdated issue summary.
Comment #23
attiks CreditAttribution: attiks commented@RobW, there are not many docs yet, but a short intro, if I find some time I'll try to rewrite this later.
Breakpoints
Multipliers
Use cases
1/ Responsive images / picture
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.
Comment #24
attiks CreditAttribution: attiks commented@RobW (and others) feel free to ping me in IRC or send me an email to clarify certain parts.
Comment #25
attiks CreditAttribution: attiks commentedAnother potential use case #1621594: Media Query Detection
Comment #26
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #27
Gábor Hojtsy@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.
Comment #28
Gábor Hojtsy(Note the grid editor is a plugin component and would not be part of the breakpoints feature per say).
Comment #29
attiks CreditAttribution: attiks commented#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
Comment #30
jessebeach CreditAttribution: jessebeach commentedRE: #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 theirmax
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 withmax-width:400px
and then the next withmin-width: 450px
.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?
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 :)
Comment #31
attiks CreditAttribution: attiks commented#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
"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
Comment #32
webchickWait. Can we not have the UX discussion in two places?
Should this be postponed on the other, or..?
Comment #33
Bojhan CreditAttribution: Bojhan commentedYes, it should be. Postponed on #1775302: Do a UX review of Breakpoint module
Comment #34
jessebeach CreditAttribution: jessebeach commentedMaybe 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.
Comment #35
attiks CreditAttribution: attiks commentedThe 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
Comment #36
JohnAlbin> 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.
Comment #37
attiks CreditAttribution: attiks commentedI'm rewriting most of the code to catch up with core, will try to make something by next week.
Comment #39
attiks CreditAttribution: attiks commentedNew patch
Comment #40
attiks CreditAttribution: attiks commentedFYI: comment #38 removed because of #1799954: Testbot 958 went silent (patch kills testbot)
Comment #41
attiks CreditAttribution: attiks commentednew patch, sandbox at http://drupal.org/sandbox/attiks/1800924, branch
Comment #42
Gábor HojtsyCore module's don't have a readme, I guess people are not expected to go and read them anyway....
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...
I don't think these need to be listed for any reason now?
Empty file for install not needed AFAIS.
Now this should be more descriptive. See other core modules for examples. Ironically the breakpoint test module & theme have a much better description. :)
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.
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?!?
Should have an empty line between function description and initial phpdoc line.
Not just these, also seen elsewhere.
!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.
$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?
The only general settings are multipliers? Then the getter should reflect that or the setter should be more generic if not.
Lacks explanation for the param and return.
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 :).
Mising newline.
Would be probably worth it to document possible sources here.
And source types here (I see it below, but cross-linking or listing them here in phpdoc would be useful).
Not sure about current coding standards, would class constants need this much namespacing? It is already under the breakpoint class.
Should have an empty line before @see in each case.
Sounds like this would in fact be easier to do compared to the rest of the validator :)
Document that this contains Breakpoint type instances (right?).
Document possible values.
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.
Breakpoints theme test(not title case).
Comment #43
Gábor HojtsyBTW the test coverage is great and very extensive, congrats on that!
Comment #44
attiks CreditAttribution: attiks commented"Generally Drupal core avoids using plurals for base objects" there's already a breakpoint module
I've send himerus an email
Comment #45
Gábor HojtsyWell, 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.
Comment #46
attiks CreditAttribution: attiks commentedAbout 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?
Comment #47
attiks CreditAttribution: attiks commentedNew patch using breakpoint as name, so no interdiff
most of #42 is fixed
Comment #48
tstoecklerHow about:
breakpoint.module
breakpoint
breakpoint_set
That would be consistent with node.module/node, comment.module/comment, etc.
Comment #49
attiks CreditAttribution: attiks commented#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.
Comment #50
tstoecklerRight, 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...
Comment #51
attiks CreditAttribution: attiks commented#50 good point, I like it. I'll wait another day so other people can have their say as well.
Comment #52
Gábor HojtsyThere 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 --):
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)?
Comment #53
attiks CreditAttribution: attiks commentedSo let's go for
breakpoint_load() and breakpoint_group_load()
Classes will be Breakpoint and BreakpointGroup
Comment #54
attiks CreditAttribution: attiks commentedpatch with the renames
Comment #55
attiks CreditAttribution: attiks commented"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
Comment #56
attiks CreditAttribution: attiks commentedadded some more comments
Comment #57
Crell CreditAttribution: Crell commentedFor 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. :-)
Comment #58
tim.plunkettTo 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.
Comment #59
Jelle_S@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?Comment #60
Crell CreditAttribution: Crell commentedJelle_S: #1798214: Upcast request arguments/attributes to full objects
Once that gets in, type hint your controller and the rest is magic. :-)
Comment #61
Gábor Hojtsy@Crell: well, yeah that is not in yet though, so we need the wrappers for now. Any other concerns, notes?
Comment #62
Crell CreditAttribution: Crell commentedhm. 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.
Comment #63
attiks CreditAttribution: attiks commentedNew patch, I removed all uses of load, load_all except for the menu callback
Comment #64
Gábor HojtsyAs far as I see, all my concerns were resolved. Any other concerns?
Comment #65
attiks CreditAttribution: attiks commentedFixed a typo and added import functionality for breakpoints defined by modules as well as import functionality of breakpoint groups defined by themes/modules.
Comment #67
attiks CreditAttribution: attiks commentedadded test to make sure settings are updated
Comment #69
attiks CreditAttribution: attiks commentedTestbot 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.
Comment #70
attiks CreditAttribution: attiks commented#67: breakpoint.patch queued for re-testing.
Comment #72
attiks CreditAttribution: attiks commentedTestbot is failing on:
So those lines are commented out, test works fine locally.
Code review is green on http://qa.drupal.org/pifr/test/316298
Comment #73
attiks CreditAttribution: attiks commentedIssue opened for testbot #1803292: Testbot has problem with module_uninstall
Comment #75
attiks CreditAttribution: attiks commented#72: breakpoint.patch queued for re-testing.
Comment #76
attiks CreditAttribution: attiks commentedTest in #72 failed on Drupal\system\Tests\Common\CascadingStylesheetsTest, table 'config' doesn't exist :/
Comment #77
attiks CreditAttribution: attiks commentedTest was failing because of a dpm :/
Comment #79
attiks CreditAttribution: attiks commentedretested from qa
Comment #80
attiks CreditAttribution: attiks commentedFirst 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)
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)
To define your own breakpoint group, use the following
Advanced use
You can also add breakpoints defined by other modules or themes, but you'll have to use the full name.
Example
Multipliers
Multipliers are simple string that are needed to support 'retina' displays, they consist of a number followed by an x
Example
Comment #81
batigolixGreat 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.
Comment #82
attiks CreditAttribution: attiks commentedDoc page created at http://drupal.org/node/1803874
Comment #83
attiks CreditAttribution: attiks commentedSome more minor fixes
Comment #84
attiks CreditAttribution: attiks commentedAdded import breakpoints defined by modules when enabling breakpoint
Comment #86
attiks CreditAttribution: attiks commentedbot problems: failing on Drupal\system\Tests\Entity\EntityFormTest
Comment #87
attiks CreditAttribution: attiks commented#84: breakpoint.patch queued for re-testing.
Comment #89
attiks CreditAttribution: attiks commentedFailing on Drupal\system\Tests\Entity\EntityTranslationTest: "Two entities loaded by uid without caring about property translatability."
Comment #90
attiks CreditAttribution: attiks commentedsome more refactoring
Comment #92
attiks CreditAttribution: attiks commentedgo bot
Comment #94
attiks CreditAttribution: attiks commentedComment #95
attiks CreditAttribution: attiks commentedSome 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
Comment #96
attiks CreditAttribution: attiks commentedTime for some reviews, the clock is ticking ....
Comment #97
attiks CreditAttribution: attiks commentedCheck if breakpoint group/breakpoint already exists while importing/reverting so the uuid doesn't change.
Comment #98
tstoecklerHere'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!!!
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?
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().
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.
Then why do we check for it? :-)
There is no
else
below either, so I think the condition can be removed.Again, should use entity_create()
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.
!empty() should suffice here, the additional isset() is needless IMO.
Even better: this code could be shortened with cool new ?: short ternary.
Likewise.
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.
Nit-pick: I think the comma should be a semi-colon.
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.
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.
This re-purposing of an existing local variable is a WTF, IMO. Inlining this as well would help.
This needs a comment. From simply reading the code, it seems completely unneccesary.
Why do we need to call breakpoint_settings_save_multipliers() and then call $breakpoint->save() with the new multipliers afterwards? That seems 100% redundant.
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.
Nitpick: delete -> Delete
Does this have to do with the duplicity of names mentioned above? In any case, this needs a comment.
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.
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.
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.
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.
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().
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.
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.
Likeweise.
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.
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?!
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.
The overridden logic, should really be ported to ConfigEntityBase. That said, if it works, and is tested, that could be a follow-up.
Likewise.
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.
I'm not aware that do that sort of line break anywhere else in core, and I find it rather unnecessary here.
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()
This is identical to ::getConfigName(), which seems weird.
These also seem like candidates for ConfigEntityBase
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.
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.
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.
Nitpick: Per our semi-new coding standards API should be capitalized, i.e.
BreakpointAPITest
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).
I guess assertNull() could be used here?
This should use entity_create()
Likewise.
Comment #99
webchickCan 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.
Comment #100
tstoecklerRe #99: I commented over there.
That made me find the following code
in DatabaseStorageController::create(), so the UUID generation in the Breakpoint and BreakpointGroup is unneccessary.
Comment #101
attiks CreditAttribution: attiks commented#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
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.
Comment #102
attiks CreditAttribution: attiks commentedTo summarize:
Comment #103
attiks CreditAttribution: attiks commentedFase 1: use entity_create
Comment #104
attiks CreditAttribution: attiks commentedFase 2: most is moved to BreakpointGroup, except for the deletes, since the deletes can operate on multiple breakpoints/breakpoint groups
Comment #105
attiks CreditAttribution: attiks commentedMessed up the interdiffs
Comment #106
attiks CreditAttribution: attiks commentedmore 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
Comment #107
andypost#101 better to include into issue summary - this could be a part of docs
Comment #108
attiks CreditAttribution: attiks commentedExceptions added
Comment #108.0
attiks CreditAttribution: attiks commentedUpdated issue summary.
Comment #109
attiks CreditAttribution: attiks commentedTitle renamed
grrr
Comment #110
attiks CreditAttribution: attiks commentedWhite space fixed.
Module added to MAINTAINERS.txt
For me this looks like it's complete, so feel free to review.
Comment #111
attiks CreditAttribution: attiks commentedFound an error in the media query validation, floating numbers are allowed as well.
New test added.
Comment #112
Gábor HojtsyLooked through the whole patch. Mostly code style comments but also a couple more important comments in there:
I don't think the leading \ is needed? It is not used at other places in core.
array (include type)
Identifier of ...
Identifiers of...
What does the return value mean?
Extra newline
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.
Should document these one-by-one as per our doc guidelines.
There should be a newline after the first comment. Also, I don't understand this comment.
This is not a capital B elsewhere, so make it lowercase here too :)
Extra newline.
These should all say "breakpoint group" no? We are explaining this in a human language :)
Yet another extra newline.
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.
I think these would be correct as // inline comments, we don't use /* and */ as inline comment markers.
Comment #113
attiks CreditAttribution: attiks commentedThanks 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
Comment #114
attiks CreditAttribution: attiks commented/* @var $breakpoint_group \Drupal\breakpoint\BreakpointGroup */ is removed, but this is the only format recognized by IDE's
Comment #115
attiks CreditAttribution: attiks commentedrenamed breakpoint group name to id
Comment #116
Bojhan CreditAttribution: Bojhan commented@John Given that is directly in the mobile initiative, I imagine you should give this a full review.
Comment #117
Bojhan CreditAttribution: Bojhan commentedFor module description, maybe something like; "Manage breakpoint settings for responsive themes."
Comment #118
Gábor HojtsyThe latest patch resolved all my concerns, so indeed, new reviewers are welcome to come out with any more :)
Comment #119
jwilson3To further clarify that we arent talking about programming/debugging breakpoints, what about module description:
"Manage screen size breakpoints for responsive themes."
Comment #120
jessebeach CreditAttribution: jessebeach commentedbreakpoint_ui should be claimed as a project namespace unless the intent is to package it inside the breakpoint module.
Comment #121
attiks CreditAttribution: attiks commentedThanks, created https://drupal.org/project/breakpoint_ui
Comment #122
attiks CreditAttribution: attiks commentedSome more cleaning thanks to Jelle_S:
Comment #123
Gábor HojtsyThe 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).
Comment #124
attiks CreditAttribution: attiks commentedOne sec, you (and I) missed this
throw new \Exception("something went wrong :s");
Will be solved in an hour or so ;-)
Comment #125
Gábor HojtsyComment #126
attiks CreditAttribution: attiks commentednew patch
Comment #127
attiks CreditAttribution: attiks commentedcore committers, please give credit to Jelle_S as well
Comment #128
Bojhan CreditAttribution: Bojhan commentedComment #129
catchHere'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).
Please factor the hook implementations out into helper functions rather than calling them directly from another hook implementation.
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?
Why's the check required when this comes from config? And why does this fail silently if the check fails?
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).
Why does it have to match?
breakpoint groups?
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?
The summary of the function should mention this detail - it'll also delete breakpoint groups, not just breakpoints.
Why is only one $group_id passed in, but the function removes multiple groups?
No delete multiple?
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)
Why fail silently if an incorrect theme_key is passed. Also won't the function do that anyway since config() won't return anything?
Same question as $theme_key above.
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?
Why the ucfirst call?
Typo: ununsed.
Can this array key legitimately be NULL? If not we could just use isset().
I guess this explains why the module vs. theme vs. custom type.
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.
This all looks unnecessary again for an initial patch.
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.
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.
Why would a breakpoint group include breakpoints that don't exist?
Global classes like this can just be \RuntimeException without the use statement now.
Comment #130
catchSorry for multiple posts.
Module is also missing a hook_help(), that's need for any new core module.
Comment #131
attiks CreditAttribution: attiks commented#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
instead6/ "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
Comment #132
tim.plunkettWhat's the difference?
How is this helpful?
This should explain why it needs a name other than $this->id()
bool, missing a description
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.
Comment #133
catchIs 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).
Comment #134
attiks CreditAttribution: attiks commented#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)
Comment #135
attiks CreditAttribution: attiks commentedNew patch with enable/disable, override/revert and normally all comments addressed.
Comment #136
attiks CreditAttribution: attiks commentednote to self: add hook_help
Comment #137
attiks CreditAttribution: attiks commentedProposal for hook_help:
About
The Breakpoint module allows the management of breakpoints and breakpoint groups for responsive designs.
Uses
Comment #138
attiks CreditAttribution: attiks commentedNew patch including hook_help from #137 we can always change the text in a follow up.
Comment #139
Jelle_SThanks! =D
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 ;-)
Comment #140
webchickNah, one of the core committers can clean that up on commit if that's the only thing wrong with it. :)
Comment #142
attiks CreditAttribution: attiks commentedGetting late :/
Comment #143
jessebeach CreditAttribution: jessebeach commentedAdded "Dynamic layouts" tag.
Comment #144
JohnAlbinShould be “a breakpoint consists of”…
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.”
"none" not "non".
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
Comment #145
attiks CreditAttribution: attiks commented#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.
Comment #146
attiks CreditAttribution: attiks commented#144 comments fixed
Comment #147
JohnAlbinOk, but you haven't addressed:
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?Comment #148
attiks CreditAttribution: attiks commentedI 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.
Comment #150
attiks CreditAttribution: attiks commented#148: breakpoint-148.patch queued for re-testing.
Comment #151
Gábor HojtsyAll review comments since RTBC were addressed. John agrees it looks good architecturally, so let's get this in!
Comment #152
webchickFirst 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...
A new module that comes with its own MAINTAINERS.txt entry. I like! :D
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.
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?
Can you provide an example of a media query?
"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?
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?
Is there a reason these are prefixed with an underscore/considered "internal"? They seem like they would be useful general API functions.
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.
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.
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?
You did an asort() on the options in breakpoint_group_labels(), why not here as well?
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()?
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.
I don't understand SOURCE_TYPE_CUSTOM. When would a breakpoint be coming from not a module or a theme?
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?
I do not understand at all what these key => value pairs represent. Can we get a comment that explains it?
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...
Could we also get a comment above this one?
...like this one!
Think you're missing a type here. Should be array?
Although I intensely dislike this convention, the convention is nontheless to NOT document getInfo() and setUp() in test classes. :\
(nitpick) Since CRUD is an acronym, the word should be in ALL CAPS in the function name.
(everywhere)
There should not be t()s around assertion messages.
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?
This chunk does not seem to be about Stark, so probably needs its own comment heading.
These should be using format_string() rather than concatenation.
Should these be "theme_test" and "Test Theme" for parity?
Comment #153
Jelle_S@webchick
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 argumentsComment #154
attiks CreditAttribution: attiks commented#152 Some feedback
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).I'll change the other things in code, thanks for reviewing.
Comment #155
attiks CreditAttribution: attiks commentedFollow up created #1813110: Introduce a function "is it a module or is it a theme?"
Comment #156
attiks CreditAttribution: attiks commentedNew patch:
Comment #157
attiks CreditAttribution: attiks commentedBack to RTBC
Comment #158
webchickAwesome. 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.
Comment #159
webchickNote 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. :(
Comment #160
attiks CreditAttribution: attiks commentedI'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
Comment #161
Gábor HojtsyI'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!
Comment #162
Gábor HojtsyComment #163
Gábor HojtsyInitial integration with gridbuilder module for the curious: http://drupalcode.org/project/gridbuilder.git/commitdiff/fa2748a329ff5d0...
Comment #164
MustangGB CreditAttribution: MustangGB commentedAs one of the aforementioned curious, I will be having a play with this when time allows.
Great job everyone!
Comment #165
eigentor CreditAttribution: eigentor commented@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.
Comment #166
dcrocks CreditAttribution: dcrocks commentedA question. Will themes inherit the breakpoint settings of (multiple?)base themes?
Comment #167
attiks CreditAttribution: attiks commented#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.
Comment #168
JohnAlbinComment #169
dcrocks CreditAttribution: dcrocks commentedI have created an issue #1828606: Make BREAKPOINT resources inheritable to discuss breakpoints and their inheritance..
Comment #170.0
(not verified) CreditAttribution: commentedUpdated issue summary.