This is a sub-issue to #1813898: [META] Add editable responsive layouts to Drupal core

Problem/motivation

Various grid systems are a best practice for web layouts and they serve as excellent positioning guides for new website editors who would otherwise need free form width and placement tools for dynamic layouts proposed in #1813898: [META] Add editable responsive layouts to Drupal core. However, there are many types of grid systems and the probably most widely known equal column fluid/fixed grids (such as 960gs or twitter bootstrap's fluid grids can or might already been obsoleted).

Proposed solution

Add pluggable grid system support to core. The grids themselves would not (yet) be useful for anything without other issues in #1813898: [META] Add editable responsive layouts to Drupal core fulfilled, however as the layout plugin introduction proved (in #1787846: Themes should declare their layouts), unexpected good side effects are not uncommon.

This patch adds a grid.module which provides an API and UI for managing Grid config entities. Each Grid config entity has a "type" which maps to a plugin that's responsible for exposing the options appropriate for that type and generating the CSS for a given set of options. The only type included in this patch is EqualColumn: contrib can add more.

This patch does not require that all (or any) themes use it for their CSS. It provides a common way of working with UI configurable grids, so only themes, layouts, and other tools that want to interact with user-configured grids need to deal with this.

Files: 
CommentFileSizeAuthor
#32 grids-in-core-32.patch27.81 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 48,173 pass(es).
[ View ]
#32 interdiff.txt2.43 KBGábor Hojtsy
#31 grids-in-core-31.patch28.29 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 46,598 pass(es).
[ View ]
#31 interdiff.txt1.73 KBeffulgentsia
#30 grids-in-core-30.patch28.29 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 46,518 pass(es).
[ View ]
#30 interdiff.txt9.21 KBGábor Hojtsy
#29 grids-in-core-29.patch23.48 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 46,430 pass(es).
[ View ]
#29 interdiff.txt2.94 KBGábor Hojtsy
#28 grids-in-core-28.patch23.36 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 46,424 pass(es), 0 fail(s), and 3 exception(s).
[ View ]
#28 interdiff.txt530 bytesGábor Hojtsy
#27 interdiff.txt5.95 KBGábor Hojtsy
#27 grids-in-core-27.patch23.29 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 1,513 pass(es), 594 fail(s), and 231 exception(s).
[ View ]
#24 interdiff.txt11.74 KBGábor Hojtsy
#24 grids-in-core-24.patch22.86 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/grid/lib/Drupal/grid/GridFormController.php.
[ View ]
#23 grids-in-core-23.patch12.36 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 46,273 pass(es).
[ View ]
#23 interdiff.txt3.39 KBGábor Hojtsy
#19 grids-in-core-19.patch12.48 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 46,258 pass(es).
[ View ]
#18 grids-in-core-18.patch11.18 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 46,261 pass(es).
[ View ]
#17 grids-in-core-17.patch11.39 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 46,261 pass(es).
[ View ]
#17 interdiff.txt1008 bytesGábor Hojtsy
#16 grids-in-core-16.patch11.38 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 46,264 pass(es).
[ View ]
#16 interdiff.txt10.76 KBGábor Hojtsy
#12 gridbuilder-for-core-12.patch12.7 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 46,274 pass(es).
[ View ]
#11 gridbuilder-for-core-11.patch12.74 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 46,270 pass(es).
[ View ]
#1 gridbuilder-for-core-1.patch35.08 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 42,896 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new35.08 KB
PASSED: [[SimpleTest]]: [MySQL] 42,896 pass(es).
[ View ]

First patch.

no please dont
Q1. Do we even want a UI for grid editing in core?
No we do not, grids is a tool its not the end solution for design implementations

Q2. What kind of grids do we want to support initially?
We dont know what will happen in the future so adding any kind of grid will lock us yet again down into a techneque that will be serioysly outdated when D8 is finnaly in use.

This can live fine in contrip but please stop adding all kinds of "sitebuilding silver bullets" into core

@mortendk: well, the point of the plugin system is whatever grid support we add to core it will be infinitely extensible in core. Ie. if you ask the plugin system for grids supported, you'd get ones provided by any core plugins the same way you'd get them from a contrib plugin.

As much as I appreciate the development direction of the responsive and layout modules, I wonder why any of this should be part of Drupal Core (be it as plugin or not). As a designer, I think of a grid as a visual aid in designing and laying out things. Shouldn't this be done by the theming system? (seperation of structure and layout - UI, templates, ...). In the future I might want to render a page for one medium with a specific typographic baseline grid and for another medium with no grid at all and for a third with a maximum fixed width centered responsive grid.

Q1. No.
Q2. Concentrate on themeability and flexibility (so that people like me can add new grid types)
Q3-Q5. I cannot say anything to this, but I like the way the content is assigned to layout regions in the panels interface. At the themeing layer I'd prefer to be able to customize the CSS for the selected breakpoints (every grid needs exceptions).

I am interested to learn more - maybe I can be of help at a later stage with some grid design or testing.

each ConfigEntity is wrapped in a Plugin instance

I'm still getting my head around this patch, but my first impression is that this is wrong. I think usually, we have ConfigEntities as the larger objects containing plugin instances, not the reverse. For example, an image style is (well, not yet, but should be) a ConfigEntity containing the configuration of image effect instances, where each image effect is a plugin. Similarly for ViewsStorage containing the configuration of display instances where each display is a plugin. This order of the relationship is needed, because different plugins need different configuration (for example, $padding_width and $gutter_width might not make sense as configuration concepts of a different grid plugin, so don't belong as properties of the ConfigEntity). I need to think a bit more on how to map this to the case of Grid, where each Grid config entity requires one grid plugin (rather than the image styles / Views examples where it's 1:many).

@effulgentsia: thanks for the review! I think it makes more sense indeed to wrap plugins into config entities then the other way around (like with the current patch), IF we consider grids would be config first and foremost. For layouts for example, the committed solution is that layouts are plugins but not configuration, so if someone wants to make editable layouts, there is no other way but to wrap plugins around config there. So you are proposing we approach this one the other way around.

That also obscures the storage of the data, since the CMI object would be a very limited shell for storing arbitrary plugin data, ie. in effect the config would have a $data key storing whatever structure the plugin needs and the config would merely be responsible for channeling that data to CMI. The plugin would need to maintain the data structure, the forms, etc. Given that config entities have a rich form handling flow, not sure what would be the best practice for tying the plugin form components to the configentity then?

In short, sounds like we could not use much of the data structure or form handling features of configentities and rather use them as a very light container just so we can use config entity lists and maintain the grids with CMI regardless of their plugin implementation. So the plugin would need to provide forms, UIs, and data structures?

Is this a fair summary?

i think mortendk #2 and kjauslin #4 have raised some valid points.

in general, i have a hard time following the discussions regarding blocks and layouts as from my perspective they introduce substancial features while most discussion is targeted not at a conceptual level but on implementation details.

most professional theming nowadays is based on css preprocessors like Sass and LESS. can we provide a perspective if and how these tools could interact with pluggable grid systems? otherwise we should clearly state that this is only for the site builder-type themer who prefers to configure everything within drupal but doesn't rely on for example ruby/compass based Sass.

also, i think adding example usages of GridBuilderInterface ->getGridCss() would greatly help understanding the approach being proposed.

i understand that given the close deadline you prefer to come up with implementations rather than long discussions but on the other hand i'd prefer seeing some of this ideas evolve in contrib rather than baking them into drupal core without a clear high-level concept of how all this will make sense for drupal 8 site builders and themers in a future-proof way - for the next say 7 years.

@dasjo: indeed, the discussion focuses on making things pluggable exactly because *right now* some people prefer to build sites on a GUI and others prefer ruby/compass/Sass/LESS tools. Drupal 8 is going to be out for quite a few years (with the current support model it would be out and supported until Drupal 10 is released, so for at least 5-6 years from now based on the current pace of development). Who can tell what is going to be best practice in 4-5 years (when Drupal 9 might not yet take over as the primary platform used)? The reason we do focus on the lower level discussion of making it pluggable is so that contrib can help apply best practices as they evolve through the years.

I think your observation is spot on that the focus here is site-builders and not those knowledgeable themers who want to hardcode their layouts with up to date best practices at all times and have full control over their full CSS. This would not be a mandatory feature, and indeed used primarily for editable layouts, which is also not a target for precision themers. However, we would love the feedback to still have this feature as best as possible with the constraints it needs to be in.

Status:Needs review» Needs work

Discussed this with Tim Plunkett in IRC as well:

[6:12pm] GaborHojtsy: timplunkett: hey
[6:12pm] timplunkett: GaborHojtsy: hi
[6:13pm] GaborHojtsy: timplunkett: so in http://drupal.org/node/1816650#comment-6620164 we want to introduce a new thing in core that is pluggable (uses plugins) and its default implementation (and possibly but not necessarily all assumed contrib interfaces) would use config for storage of backend data
[6:13pm] Druplicon: http://drupal.org/node/1816650 => Add swappable (dynamic) grid systems to core => Drupal core, other, normal, needs review, 8 comments, 5 IRC mentions
[6:13pm] GaborHojtsy: timplunkett: the question is how would we ideally combine plugins and config entities
[6:13pm] GaborHojtsy: timplunkett: which is probably very much at the heart of Views for D8 as well
[6:13pm] timplunkett: oh wow
[6:14pm] timplunkett: GaborHojtsy: well no, our configentity references several plugin types
[6:14pm] timplunkett: GaborHojtsy: but you can't swap out the config entity for something else
[6:14pm] GaborHojtsy: timplunkett: yeah, that possibly makes sense, but then your config entity cannot really make use of its form controller, etc. or it would need to relay al that to the plugin
[6:15pm] timplunkett: GaborHojtsy: rewriting the Views UI as a form controller is something i hope to work on at badcamp...
[6:15pm] GaborHojtsy: timplunkett: so you just use the config entity as a broker to store config data and plugins to provide the real behaviour logic / data formats, etc?
[6:16pm] GaborHojtsy: timplunkett: that is a config entity with one property $data, or somesuch
[6:16pm] timplunkett: GaborHojtsy: correct. think views exports: http://drupalcode.org/project/views.git/blob/refs/heads/8.x-3.x:/config/...
[6:17pm] timplunkett: GaborHojtsy: each time you see "id:" in there, its a plugin ID
[6:17pm] GaborHojtsy: timplunkett: ok, that makes sense, thanks!
[6:17pm] timplunkett: GaborHojtsy: just a side note for your patch, can you not add a procedural wrapper for the manager? just use drupal_container directly
[6:18pm] timplunkett: it'll save us clean-up later
[6:18pm] GaborHojtsy: timplunkett: yeah, I just took it from the layout stuff, but I can definitely take it out
[6:18pm] GaborHojtsy: timplunkett: thanks!

In short wrapping plugins in CMI objects is what Tim suggests (and what Views uses) for this type of problem as well.

+++ b/core/modules/gridbuilder/gridbuilder.moduleundefined
@@ -0,0 +1,242 @@
+  $items['admin/structure/grids/grid/%gridbuilder/edit'] = array(
...
+  $items['admin/structure/grids/grid/%gridbuilder/delete'] = array(
...
+    'path' => 'admin/structure/grids/grid/' . $grid->id(),

The common pattern here should be admin/structure/grids/manage/ id()

Status:Needs work» Needs review
StatusFileSize
new12.74 KB
PASSED: [[SimpleTest]]: [MySQL] 46,270 pass(es).
[ View ]

Ok, took some drastic measures to help this get accepted:

- cut out everything related to a user interface (I did leave in one debug function to show you it works)
- made config instantiate plugins instead of the other way around
- made and proved the CSS generation actually works now (so feel free to bring your pitchforks and tear it apart, it is certainly not up to the standards we want, but that is the greatness of the community and collective knowledge :)

So you define a grid like this in .yml:

A six column fluid grid:

id: six_column_fluid
label: Six column fluid
pluginId: equal_column_grid
pluginConfiguration:
  type: 'fluid'
  width: 100
  columns: 6
  padding_width: 1.5
  gutter_width: 2

A 12 column 960px fixed grid:

id: ninesixty_12
label: '960px wide, 12 column grid'
pluginId: equal_column_grid
pluginConfiguration:
  type: 'fixed'
  width: 960
  columns: 12
  padding_width: 20
  gutter_width: 10

The grids are fully plugin based so you give the plugin id and the plugin configuration here, and the plugin is instantiated later using those. The system is fully pluggable, so none of the grids are required to have equal column widths or have a concept of fixed/fluid or have gutters and padding. This might be dangerous for the usefulness of grids (eg. we are not even sure a number of columns is represented in the plugin configuration which would be needed in some way for the editable layout efforts). We can rectify this by introducing a getNumberOfColumns() method on the plugin or somesuch that would compute/retrieve/return the number of columns, etc. So this does not necessarily need to be a limitation on the config.

You can load the grids using regular entity functions:

<?php
  $grid1
= entity_load('grid', 'six_column_fluid');
 
$grid2 = entity_load('grid', 'ninesixty_12');
?>

These will load the entity with the above configuration but will not yet instantiate the plugin for them, until you actually use it. You can get a plugin by calling $grid->getPlugin() or directly render the CSS via the grid object (which will use the plugin to render the CSS):

<?php
  $plugin
= $grid1->getPlugin();
 
// .. do something with $plugin
  // OR render CSS via the plugin (but you don't need to know its a plugin :)
 
return $grid->getGridCss();
?>

Any UI for this would need to use the plugin system as well to render the forms and validate/process the data. I'm not confident we can get a UI in in the first patch, so just proposing this simpler backend only implementation for now (like breakpoints in core).

We do definitely need tests for this, the included gridbuilder_help() debug function helps you see it actually works, and generates these CSS snippets respectively for the two grids:

.grid-six_column_fluid .grid-col { border: 0px solid rgba(0,0,0,0); float: left; -webkit-box-sizing: border-box; -moz-box-sizing: border-box; box-sizing: border-box; -moz-background-clip: padding-box; -webkit-background-clip: padding-box; background-clip: padding-box; margin-left: 2%; padding: 0 1.5%; } .grid-six_column_fluid .grid-col.grid-col_first { margin-left: 0; clear: both; } .grid-six_column_fluid .grid-col_1 { width: 15%; } .grid-six_column_fluid .grid-col_2 { width: 32%; } .grid-six_column_fluid .grid-col_3 { width: 49%; } .grid-six_column_fluid .grid-col_4 { width: 66%; } .grid-six_column_fluid .grid-col_5 { width: 83%; } .grid-six_column_fluid .grid-col_6 { width: 100%; margin-left: 0; }

.grid-ninesixty_12 .grid-col { border: 0px solid rgba(0,0,0,0); float: left; -webkit-box-sizing: border-box; -moz-box-sizing: border-box; box-sizing: border-box; -moz-background-clip: padding-box; -webkit-background-clip: padding-box; background-clip: padding-box; margin-left: 10px; padding: 0 20px; } .grid-ninesixty_12 .grid-col.grid-col_first { margin-left: 0; clear: both; } .grid-ninesixty_12 .grid-col_1 { width: 70.833333333333px; } .grid-ninesixty_12 .grid-col_2 { width: 151.66666666667px; } .grid-ninesixty_12 .grid-col_3 { width: 232.5px; } .grid-ninesixty_12 .grid-col_4 { width: 313.33333333333px; } .grid-ninesixty_12 .grid-col_5 { width: 394.16666666667px; } .grid-ninesixty_12 .grid-col_6 { width: 475px; } .grid-ninesixty_12 .grid-col_7 { width: 555.83333333333px; } .grid-ninesixty_12 .grid-col_8 { width: 636.66666666667px; } .grid-ninesixty_12 .grid-col_9 { width: 717.5px; } .grid-ninesixty_12 .grid-col_10 { width: 798.33333333333px; } .grid-ninesixty_12 .grid-col_11 { width: 879.16666666667px; } .grid-ninesixty_12 .grid-col_12 { width: 960px; margin-left: 0; }

The selectors are pretty flexibly configurable, these are just the defaults to scope the CSS such that multiple grid systems can be used on the page for different things (eg. for different breakpoints :).

Hopefully the scaled back scope of this issue helps focus on the real problems and the discussion to be on the base functionality :)

Not posting an interdiff, since this is pretty much a rewrite.

StatusFileSize
new12.7 KB
PASSED: [[SimpleTest]]: [MySQL] 46,274 pass(es).
[ View ]

Fix minor CSS rendering bug. Any feedback on this approach (summary in above comment)?

Status:Needs review» Needs work

Overall, I like the approach.

+++ b/core/modules/gridbuilder/config/grid.ninesixty_12.yml
@@ -0,0 +1,9 @@
+id: ninesixty_12
+label: '960px wide, 12 column grid'
+pluginId: equal_column_grid
+pluginConfiguration:
+  type: 'fixed'
+  width: 960
+  columns: 12
+  padding_width: 20
+  gutter_width: 10

I don't like pluginId and pluginConfiguration as the key names. Instead of 'pluginId', I suggest 'type'. Instead of 'pluginConfiguration', I suggest 'options' (which is what Views uses, I don't know if Views plans on renaming it to 'configuration' at some point, if so, I suggest we rename when it does to maintain consistency).

There's no reason for 'type' (pluginId) to have a 'grid' suffix. 'equal_column' is sufficient.

I don't think 'fluid'/'fixed' should be a 'type' within 'options'. I think we could instead do one of the following:
- Separate at the class/plugin level. In other words, instead of a EqualColumnGrid class, have a EqualColumnBase class, and EqualColumnFixed and EqualColumnFluid subclasses. Then the top-level 'type' key could be set to either 'equal_column_fixed' or 'equal_column_fluid'.
- Or, if it makes more sense for 'fluid' to be an option of a single EqualColumn plugin, then have a 'fluid' boolean key, or overload 'width' to allow a value of 'fluid', or allow 'width' values to contain their own suffixes (e.g., '100%'), or have a 'units' key that could be 'px', 'em', or '%', or something else along these lines.

+++ b/core/modules/gridbuilder/lib/Drupal/gridbuilder/Grid.php
@@ -0,0 +1,80 @@
+  public function getGridCss() {
+    return $this->getPlugin()->getGridCss();
+  }

If we're implementing this method on the Grid class, let's also declare that Grid implements GridBuilderInterface (which I suggest renaming to GridInterface), and accept and forward the parameters declared by the interface.

+++ b/core/modules/gridbuilder/lib/Drupal/gridbuilder/Plugin/Type/GridBuilderManager.php
@@ -0,0 +1,31 @@
+  protected $defaults = array(
+    'class' => 'Drupal\gridbuilder\Plugin\gridbuilder\gridbuilder\EqualColumnGrid',
+  );

This shouldn't be necessary. AnnotatedClassDiscovery always sets the 'class' key.

+++ b/core/modules/gridbuilder/lib/Drupal/gridbuilder/Plugin/gridbuilder/gridbuilder/EqualColumnGrid.php
@@ -0,0 +1,111 @@
+ * @Plugin(
+ *  id = "equal_column_grid",
+ *  derivative = "Drupal\gridbuilder\Plugin\Derivative\GridBuilder"
+ * )

'grid' is superfluous in the 'id' and the 'derivative' key can be removed.

+++ b/core/modules/gridbuilder/lib/Drupal/gridbuilder/Plugin/gridbuilder/gridbuilder/EqualColumnGrid.php
@@ -0,0 +1,111 @@
+  public function __construct(array $configuration, $plugin_id, DiscoveryInterface $discovery) {
+    // Get definition by discovering the declarative information.
+    $definition = $discovery->getDefinition($plugin_id);
+    parent::__construct($configuration, $plugin_id, $discovery);
+  }

This doesn't seem to do anything useful.

Also, for consistency with other module naming patterns in core, we should rename this to grid.module (in the issue that adds a UI, we can discuss whether to do so in the same module, or split out a grid_ui.module, since we do not yet have a clear cut pattern for when a UI piece warrants its own module).

I think these are good suggestions. For why it is not a GridInterface and grid.module, etc. there is http://drupal.org/project/grid existing, which is about input fields in a grid. Granted it did not have a D7 release and looks pretty much abandoned for years, so probably not an issue if we take over that namespace in core. Will work on an updated patch later today.

Status:Needs work» Needs review
StatusFileSize
new10.76 KB
new11.38 KB
PASSED: [[SimpleTest]]: [MySQL] 46,264 pass(es).
[ View ]

- renamed pluginId to type as suggested
- renamed pluginConfiguration to options as suggested (I think these two obscure how it works a bit, but I'm not crazy about it either way)
- equal_column_grid renamed to equal_column

- @todo: I kept type fluid/fixed in options for now; I don't think its misleading, although I see if we use 'type' for plugin identification purposes, then this might be misleading :) - I think it makes sense for it to be an option on the equal column grid; it might be best to collapse with the width key; other's feedback welcome!

- renamed everything from gridbuilder => grid, GridBuilder => Grid (eg. GridInterface, GridBundle, etc.)
- made Grid implement GridInterface and accept and forward the interface parameters proper
- removed the $defaults class key
- made all suggested changes to the EqualColumn annotation and removed its useless constructor
- renamed the module to grid.module and removed the breakpoint requirement that is not anymore valid

I think this resolves all suggestions but the fluid/fixed. It still works the same way as before but I agree is much more clean :)

StatusFileSize
new1008 bytes
new11.39 KB
PASSED: [[SimpleTest]]: [MySQL] 46,261 pass(es).
[ View ]

Found one typo in the three column grid, and the Grid class properties were not properly renamed.

StatusFileSize
new11.18 KB
PASSED: [[SimpleTest]]: [MySQL] 46,261 pass(es).
[ View ]

Time to drop debugging code. :)

StatusFileSize
new12.48 KB
PASSED: [[SimpleTest]]: [MySQL] 46,258 pass(es).
[ View ]

Thanks. Getting much cleaner!

This patch:
- Moves GridManager out of the Type namespace. I don't know if the Type namespace is obsolete at this point or not: aggregator doesn't use it anymore, but Views still does. Personally, I prefer to have the *Manager and the *Interface at the same level.
- Makes Grid::$plugin protected.
- Renames (three|six|twelve)_column_fluid.yml to fluid_(3|6|12) to match the 960_* ones.
- Reworks the EqualColumn class.

I still have some questions remaining that I'll post in a separate comment.

+1 on moving GridManager out of the Type namespace, we just made that change on the entity info patch, and Views will be moving soon as well.

Status:Needs review» Needs work

+++ b/core/modules/grid/grid.info
@@ -0,0 +1,8 @@
+configure = admin/structure/grids

This patch doesn't implement this URL as far as I can tell, unless there's some automagic in ConfigEntity I'm not aware of.

+++ b/core/modules/grid/grid.module
@@ -0,0 +1,26 @@
+    'label' => 'Grid',

needs to be t('Grid').

+++ b/core/modules/grid/lib/Drupal/grid/Plugin/grid/grid/EqualColumn.php
@@ -0,0 +1,179 @@
+    if ($type === 'fluid') {
+      $this->width = 100;
+      $this->units = '%';
+    }
+    else {
+      $this->width = $width;
+      $this->units = 'px';
+    }

Does 'fluid' always mean 100% or would it be useful to allow fluid types with some other percentage? I'm still not crazy about using 'type' for what is essentially a boolean (fluid or not fluid).

Also, do we want to support 'em' units? In which case, why not remove 'type' and add 'units' as a direct config option.

+++ b/core/modules/grid/lib/Drupal/grid/Plugin/grid/grid/EqualColumn.php
@@ -0,0 +1,179 @@
+  public function getGridCss($wrapper_selector = NULL, $col_selector_prefix = NULL, $skip_spacing = FALSE) {

Why have wrapper_selector and col_selector_prefix as parameters of this method? Would they make more sense as config options, or are there use cases where the same grid entity needs to have its CSS rendered with different selectors?

+++ b/core/modules/grid/lib/Drupal/grid/Plugin/grid/grid/EqualColumn.php
@@ -0,0 +1,179 @@
+    $css[$wrapper_selector . ' .grid-col' . $col_selector_prefix . 'first'] = array(
+      'margin-left' => '0',
+      'clear' => 'both',
+    );
+    for ($i = 1; $i <= $this->columns; $i++) {
+      $selector = $wrapper_selector . ' .grid-col' . $col_selector_prefix . $i;

Do we need 'grid-col-first' as a separate class from 'grid-col-1'? Also, why do we hard-code the general .grid-col class despite allowing the prefix for the specific classes to be passed in?

+++ b/core/modules/grid/lib/Drupal/grid/Plugin/grid/grid/EqualColumn.php
@@ -0,0 +1,179 @@
+        // Other columns absorb all columns that they need to include and one
+        // less margin before them.
+        $css[$selector] = array(
+          'width' => (($colwidth * $i) + ($gutter * ($i - 1))) . $this->units,
+        );

It's so weird to me that each column's width includes the width of previous columns. Then again, I'm confused by all sorts of stuff in CSS-land.

Commenting on the Outstanding Questions from the issue summary:

Do we even want a UI for grid editing in core?

If we have this module in core at all, and if grids are architected as config (as they are in this patch), then yes, we need a UI, even if it's very bare bones. We never (except for a couple exceptions in settings.php) have configuration in Drupal core for which we don't provide a UI.

As far as whether the concept of Grids as ConfigEntitities belongs in core (a concern raised in #2, #4, and #7), I think they do, because:

  1. It's a prerequisite for a layout builder in core.
  2. Even if we're not able to get a layout builder into core, having a common architecture for creating, configuring, listing, and storing grids, will allow multiple layout builder modules and other tools that want to use an administrator-configurable grid to exist in contrib and be more easily interoperable and compatible with each other.

If we do, do we want it in the first patch?

I think so. Adding it will help clarify the relationship between the Grid entity and the EqualColumn plugin (i.e., providing a form for the 'options' needs to be done by the plugin, whereas the basic CRUD UI needs to be done by the entity). Unless there's some other issue that is urgently blocked on this one, in which case, I think a follow up, while less ideal, is ok.

What kind of grids do we want to support initially? The current code supports equal column grids with optional gutters and padding, which seems like baseline.

I think the EqualColumn plugin is sufficient for now. Contrib can add more. If front-end developers want to make a case for other very common grid patterns that they would like to see in core, let's add follow up issues for them, but looks to me like the front-end devs on this issue are asking for as little baked into core as possible.

However, while I think the EqualColumn plugin should stay, I think we should remove the 5 specific configurations (960/12, 960/16, fluid/3, fluid/6, and fluid/12) from this patch. We have no use case for them yet. When we add something to core that needs a specific default grid to work with, then we can add that default at that time.

Where do we source grids from?

Done. Patch implements them as config.

What is the responsibility of the ConfigEntity vs. the Plugin layer.

Done. The config entity is the top-level object that forwards things that need to be pluggable on an instance by instance basis to the plugin. Same pattern as we use elsewhere (image styles, views, and more to come).

Do we even need even more plugability? Eg. to attach more custom data to grids. The current core breakpoint relation is directly implemented on the grid, which might not be ideal.

The first part seems done. Anything in 'options' is forwarded to the plugin: each plugin can decide what options, including any custom ones, it wants. I don't understand the breakpoint comment: is it still relevant?

StatusFileSize
new3.39 KB
new12.36 KB
PASSED: [[SimpleTest]]: [MySQL] 46,273 pass(es).
[ View ]

@effulgentsia:

1. I don't see how reworking the EqualColumn constructor to take argument like this works as a swappable plugin. Other plugins could easily not have a fixed column number or have no gutters and paddings. You essentially dropped the implementation of PluginBase from the equal column grid.

2. URLs, was indeed a leftover, but you already removed it :)

3. t('Grid') is implemented now.

4. Type => Units, made that change.

5.

Why have wrapper_selector and col_selector_prefix as parameters of this method? Would they make more sense as config options, or are there use cases where the same grid entity needs to have its CSS rendered with different selectors?

Definitely NOT config options (as in options for the .yml files). We need to be able to render the same CSS with different selectors based on the use case we apply them. Eg. if we build a responsive page where different grids apply at different breakpoints, we'd need to apply different selectors per grid, we cannot just use the same selectors for all grids, we need to vary them. The responsive layout module uses this functionality to apply different selectors.

6.

Do we need 'grid-col-first' as a separate class from 'grid-col-1'?

and also

It's so weird to me that each column's width includes the width of previous columns. Then again, I'm confused by all sorts of stuff in CSS-land.

Well, grid-col-first is a class to apply to the first content in row regardless of its width, and grid-col-1 is a 1 wide column. Similarly, the reason for each column to be accumulating all previous columns is because grid-col-5 means its a 5 column wide width element, while grid-col-3 is just 3 column wide.

7.

Also, why do we hard-code the general .grid-col class despite allowing the prefix for the specific classes to be passed in?

We can rework this making any class overridable. What is your suggestion?

8.

We never (except for a couple exceptions in settings.php) have configuration in Drupal core for which we don't provide a UI.

That is not entirely true. We definitely have breakpoints that have no UI.

9.

However, while I think the EqualColumn plugin should stay, I think we should remove the 5 specific configurations (960/12, 960/16, fluid/3, fluid/6, and fluid/12) from this patch. We have no use case for them yet. When we add something to core that needs a specific default grid to work with, then we can add that default at that time.

I don't think a grid module without reasonable default config looks good, but if you believe it should be started off fresh (unless other modules land needing grids, then well, ok). We do need some grids in a testing module then anyway (similarly to how layouts were introduced).

10.

The first part seems done. Anything in 'options' is forwarded to the plugin: each plugin can decide what options, including any custom ones, it wants.

How do you imagine this happening? This cycles back to my point in 1? You made the EqualColumn plugin not even a plugin implementation and not very generic at all with its specific constructor, so I'm not sure how this is envisioned.

11.

I don't understand the breakpoint comment: is it still relevant?

At least for the responsive layout module, we need breakpoints to be associated with grids. Breakpoints do not have 'extensible' metadata capabilities, but at least grids to some degree had (before your last patch). So what I did in #1822950: Add responsive layout builder to core is to tack on breakpoint associations in the grid options. Lacking extensibility in breakpoints and grids, we'd need to introduce yet another new object there which is a grid-breakpoint bridge relating the two together. I don't like that idea much, given we already have way too many moving parts/objects and this is a relatively minor relation problem. However we'd ideally have a reasonably nice way to include this kind of extension configuration with the grid. Just tacking on the data might not be reasonable, but building a whole extra plugin layer might be overkill.

I kept this needs work for 1 and 10 at least.

Status:Needs work» Needs review
StatusFileSize
new22.86 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/grid/lib/Drupal/grid/GridFormController.php.
[ View ]
new11.74 KB

Here is a very quick and untested stab at an admin UI:

- added back menu items
- added back form controller
- added back grid.admin.inc
- made the form controller delegate entity prepare, form and submission to the plugin

And this latest one is the most interesting. I did not touch your grid instantiation changes, but those make the grid (plugin?) class not really aware of the plugin structure (how the options are stored), but the entity preparation, form and form validation does not have much of a way around it (and also cannot avoid that data to be stored in an array). So your changes for instantiation look even more odd with these in.

I need to a quick errand now, so just posting for debates. Might not pass :)

Status:Needs review» Needs work

The last submitted patch, grids-in-core-24.patch, failed testing.

I don't see how reworking the EqualColumn constructor to take argument like this works as a swappable plugin. Other plugins could easily not have a fixed column number or have no gutters and paddings. You essentially dropped the implementation of PluginBase from the equal column grid.

It's the magic of ReflectionFactory. Plugin instantiation is done by calling $plugin_manager->createInstance($plugin_id, $configuration). The ReflectionFactory looks at what constructor arguments the particular plugin class wants and passes them from the keys in $configuration. It allows any given plugin class to have a well defined and PHPDoc'd constructor and protected properties without enforcing that all plugin classes have the same constructor signature.

There is no requirement that plugin classes inherit from PluginBase. All PluginBase does is provide an implementation of PluginInspectionInterface, and not all plugins need to be inspectable (i.e., given a $plugin, for calling code to be able to get its id and definition). PluginInspectionInterface was added to support Field API conversion where plugins were passed around in interesting ways with calling code complex enough to need to inspect the plugin, and possibly Grid code will evolve to need that too, at which point we could add it. All it requires is for the constructor to also take $plugin_id and $discovery as additional constructor args, which isn't hard, but it's clutter if not needed.

grid-col-5 means its a 5 column wide width element

Oh, ok! I was tripped up by the code comment that said "first column" for i=1 and "last column" for i=n. $i referring to number of columns taken up by an element makes sense: we just need to fix those comments to match.

We do need some grids in a testing module then anyway

Yep. Adding a testing module and moving some/all of the default grids to it sounds great.

Thanks for #24. I'll try it out shortly.

Status:Needs work» Needs review
StatusFileSize
new23.29 KB
FAILED: [[SimpleTest]]: [MySQL] 1,513 pass(es), 594 fail(s), and 231 exception(s).
[ View ]
new5.95 KB

1. Due to #1763974: Convert entity type info into plugins the hook_entity_info() part needed to go to annotations and the move of that to a plugin class (using other plugins nonetheless) was required.
2. Also fixed things noticed by @mbrett5062 in #1813910: Add region module to Drupal core (for editable responsive layouts) in the similar base code (name of delete confirm form function, lack of hook_help, $items initialisation in hook_menu(), etc).
3. Fixed the parse errors in the form controller.

StatusFileSize
new530 bytes
new23.36 KB
FAILED: [[SimpleTest]]: [MySQL] 46,424 pass(es), 0 fail(s), and 3 exception(s).
[ View ]

Also need to use the annotation classes in the plugin class file to make it work at all.

StatusFileSize
new2.94 KB
new23.48 KB
PASSED: [[SimpleTest]]: [MySQL] 46,430 pass(es).
[ View ]

Found a couple more issues with how the form is constructed.

StatusFileSize
new9.21 KB
new28.29 KB
PASSED: [[SimpleTest]]: [MySQL] 46,518 pass(es).
[ View ]

All right, got down to do some tests and uncovered some bugs.

- the delete page had a wrong callback path

- the form options sub-element was not #tree TRUE, but then again, the error setup would need to know about the wrapper structure or would need to get one layer up to register errors for it, so I decided to go for simple instead and just merge the plugin fields on the top level; there should always only be one plugin and it should know the default fields

In the tests:

- moved default grids to the test, no grids defined now in the module itself
- have tests for the default grids, deleting and editing those
- then the creation of a fluid and a fixed grid and the editing and deletion of one of them

I did not add coverage yet for the CSS generated since we did not get much commentary on that yet.

Any comments overall pro or contra?

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new1.73 KB
new28.29 KB
PASSED: [[SimpleTest]]: [MySQL] 46,598 pass(es).
[ View ]

Trivial docs cleanup. I think this is complete and polished enough for commit. Let's see if the core committers agree.

Issue summary:View changes

Updated issue summary.

StatusFileSize
new2.43 KB
new27.81 KB
PASSED: [[SimpleTest]]: [MySQL] 48,173 pass(es).
[ View ]

Trivial change to centralize the test initialisation. Should keep this RTBC.

Status:Reviewed & tested by the community» Needs work
Issue tags:-Dynamic layouts, -Spark, -Spark Sprint 7

The last submitted patch, grids-in-core-32.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Dynamic layouts, +Spark, +Spark Sprint 7

#32: grids-in-core-32.patch queued for re-testing.

I don't quite get why this went to RTBC, I only see patches/reviews going back forth no feedback from our themers who would end up using this? Its also adding a UI piece?

#32: grids-in-core-32.patch queued for re-testing.

Status:Needs review» Postponed

Well, anyway, due to core things not happening below this layer that we wanted to build on, we just need to go and focus on those things and will pretty likely not have a responsive layout builder (and therefore this module) in core. Unless someone comes and want to run with all these. Start from #1813898: [META] Add editable responsive layouts to Drupal core.

Version:8.x-dev» 9.x-dev
Status:Postponed» Needs work

Given feature freeze, this is not going to happen in Drupal 8. Watch http://drupal.org/project/gridbuilder for this purpose in Drupal 8. Moving this to Drupal 9.

Issue summary:View changes

Updated issue summary.