This is a sub-issue of #1787634: [META] Decouple layouts from themes.

Problem/motivation

With all page components hopefully converted to blocks (such as page title, primary menus, secondary menus, etc) *and* the proposed possibility to create landing pages with separate block setup / placement / configuration, we need a way to control master layouts where the common elements are placed, so when you create a page with a layout, you don't need to start over and place all the common elements all over again.

Proposed solution

Ideally we would have a way to create multiple master layouts that can be used to create pages with different block setup, however, for the pre-feature freeze scope, it seems possible to only support master front end and master backend layouts, where all pages are derived from this layout. Also while we call these layouts on the frontend, the backend implementation calls them displays.

PageTerminology.jpg

Designs for proposed solution

Listing of two specific layouts possible:
LayoutsList.jpg

Editing of the layout:
EditLayout 2.jpg

CommentFileSizeAuthor
#95 1841584-master_displays-95.patch179 KBsidharthap
#70 interdiff-66-70.txt18.62 KBfrega
#70 1841584-master_displays-70.patch165.84 KBfrega
PASSED: [[SimpleTest]]: [MySQL] 53,170 pass(es). View
#67 1841584-master_displays-66.patch155.17 KBfrega
PASSED: [[SimpleTest]]: [MySQL] 53,145 pass(es). View
#67 interdiff-63-66.txt15.5 KBfrega
#65 1841548-initial.png63.34 KBfrega
#65 1841584-unstyled-dialog.png79.74 KBfrega
#65 1841584-modified.png65.54 KBfrega
#65 1841584-dnd.png62.01 KBfrega
#64 masterlayout.png33.5 KBBojhan
#63 1841584-master_displays-63.patch152.13 KBfrega
FAILED: [[SimpleTest]]: [MySQL] 53,077 pass(es), 1 fail(s), and 0 exception(s). View
#63 interdiff-60-63.txt15.12 KBfrega
#60 1841584-master_displays-60.patch157.23 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 53,046 pass(es). View
#55 display-ui-1841584-55.patch146.12 KBfrega
FAILED: [[SimpleTest]]: [MySQL] 48,912 pass(es), 2 fail(s), and 0 exception(s). View
#55 interdiff-47-55.txt16.09 KBfrega
#47 display-ui-1841584-47.patch144.14 KBfrega
FAILED: [[SimpleTest]]: [MySQL] 48,925 pass(es), 2 fail(s), and 0 exception(s). View
#47 interdiff-1841584-33-47.txt43.56 KBfrega
#46 interdiff-1841584-33-46.txt38.02 KBfrega
#46 display-ui-1841584-46.patch139.02 KBfrega
FAILED: [[SimpleTest]]: [MySQL] 48,923 pass(es), 2 fail(s), and 0 exception(s). View
#44 display-ui-1841584-44.patch135.34 KBfrega
FAILED: [[SimpleTest]]: [MySQL] 48,931 pass(es), 2 fail(s), and 0 exception(s). View
#38 display-ui-1841584-38.patch134.65 KBfrega
FAILED: [[SimpleTest]]: [MySQL] 49,282 pass(es), 2 fail(s), and 0 exception(s). View
#38 interdiff.txt27.43 KBfrega
#37 display-ui-1841584-37.patch122.55 KBfrega
FAILED: [[SimpleTest]]: [MySQL] 49,285 pass(es), 3 fail(s), and 0 exception(s). View
#37 interdiff.txt14.27 KBfrega
#35 frega-display-ui-35.patch20.62 KBfrega
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch frega-display-ui-35.patch. Unable to apply patch. See the log in the details link for more information. View
#33 display-ui-33.patch114.21 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 49,298 pass(es), 2 fail(s), and 0 exception(s). View
#33 interdiff.txt660 bytesGábor Hojtsy
#30 testchange.txt2.79 KBGábor Hojtsy
#30 display-ui-29.patch114.21 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 48,931 pass(es), 2 fail(s), and 0 exception(s). View
#28 display-ui-28.patch114.87 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/layout/layout.module. View
#28 Layouts.jpg52.4 KBGábor Hojtsy
#28 EditLayout.jpg54.21 KBGábor Hojtsy
#27 displays-27.patch110.95 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 48,978 pass(es), 6 fail(s), and 0 exception(s). View
#27 interdiff.txt3.99 KBGábor Hojtsy
#25 displays-25.patch107.6 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 48,789 pass(es), 5 fail(s), and 0 exception(s). View
#25 interdiff.txt3.44 KBGábor Hojtsy
#22 interdiff.txt2.2 KBsdboyer
#22 display-ui-1841584-22.patch105.44 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] 48,777 pass(es), 6 fail(s), and 0 exception(s). View
#16 interdiff.txt7.1 KBGábor Hojtsy
#16 display-ui-16.patch104.1 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 48,359 pass(es). View
#16 LayoutEdit-1.jpg53.19 KBGábor Hojtsy
#15 display-ui-15.patch100.46 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 48,298 pass(es). View
#15 interdiff.txt9.44 KBGábor Hojtsy
#13 interdiff.txt13.33 KBGábor Hojtsy
#13 display-ui-13.patch97.66 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 48,290 pass(es). View
#13 display-ui-13.patch100.04 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 48,277 pass(es). View
#13 EditMasterLayout.jpg94.21 KBGábor Hojtsy
#11 1841584-interdiff-11.txt1.67 KBandypost
#11 1841584-display-11.patch10.68 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 48,241 pass(es). View
#9 LayoutList.jpg32.57 KBGábor Hojtsy
#9 LayoutEdit.jpg14.72 KBGábor Hojtsy
#9 interdiff.txt604 bytesGábor Hojtsy
#9 display-ui-9.patch9.02 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 48,217 pass(es). View
#8 display-ui-8.patch8.96 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 48,217 pass(es). View
#8 Layouts1.jpg50.76 KBGábor Hojtsy
#8 Layout2.jpg21.73 KBGábor Hojtsy
EditLayout 2.jpg90.25 KBGábor Hojtsy
LayoutsList.jpg49.97 KBGábor Hojtsy
PageTerminology.jpg27.02 KBGábor Hojtsy
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Gábor Hojtsy’s picture

Title: Add page to configure global displays (called global layouts on the UI) » Add page to configure master displays (called master layouts on the UI)

Fix naming in title. Oh these names.

duckzland’s picture

Nice design, one question though, where can user add new block or region?

I've implemented a similar system in my Sigmaone base theme and utilizes 1140.css grid system. Will layout in core supports css grid system as well?.

Gábor Hojtsy’s picture

@duckzland: the layout/template comes from a module or theme. That module or theme can use any grid system and/or make the regions editable and configurable. That is not really the role of this screen. This screen takes that template and lets people put master blocks in them (that would be useful for all pages at specific places). The huge + buttons on the side of the regions allow for block placement in each region in this design.

duckzland’s picture

so themes css is responsible to set the width of the layout item (region or block) as in the image? along with custom classes (if the theme is using a css grid system)?

Gábor Hojtsy’s picture

@duckzland: see #1787846: Themes should declare their layouts and its changelog at http://drupal.org/node/1813372 for how this is structured.

effulgentsia’s picture

duckzland’s picture

ahh thanks for the links, I got it now.

Gábor Hojtsy’s picture

FileSize
21.73 KB
50.76 KB
8.96 KB
PASSED: [[SimpleTest]]: [MySQL] 48,217 pass(es). View

Ok, here is an initial patch.

1. OMG terminology galore. Displays edited on the UI as layouts, where they depend on layouts on the backend, and OMG anyway. See and participate in #1841824: Terminology: figure out proper words to use in the UI for describing unpopulated layout vs. populated layout.

2. I introduced a new display.module in this patch. The code added might be as part of the layout module, but that does not really matter at this point IMHO. We can move around code for months and #1839278: Add layout template demonstration is well entrenched into modifying layout module in similar ways for other reasons, so I wanted to expressly avoid conflict.

3. I've added a display_entity_info_alter() to alter in our list/form, etc. controllers. I could have modified the existing displays code as well. I guess we should discuss if this UI will be a required piece of core (in which case the references should go there) or not (in which case the references need to be altered in AFAIS).

4. It is not possible to add or delete displays. This is by design as per the design above. You have a front end master and a backend master and that is it.

5. The label() method does not work well on the Display config object. I have not been able to track this down, but that is why there are missing labels all around when you attempt to run the patch.

6. It is only possible so far to edit the layout assigned to a display, not the blocks configured. (Also the blocks are not yet plugins and the blocks we want to usually edit here such as site name or logo are not yet blocks even, so we'll need to stuff this with dummy things for a while).

Screenshots for list:
Layouts1.jpg

For editing:
Layout2.jpg

Gábor Hojtsy’s picture

Status: Active » Needs review
FileSize
9.02 KB
PASSED: [[SimpleTest]]: [MySQL] 48,217 pass(es). View
604 bytes
14.72 KB
32.57 KB

While doing my daily workout, I realized the label was obviously missing due to the entity keys not properly populated. Looks much better now :)

List:
LayoutList.jpg

Edit:
LayoutEdit.jpg

andypost’s picture

Status: Needs review » Needs work

@Gabor: idea is great, suppose we need a element and method to provide a layout list to be used on pages where layout chosen so this entity should have a render/preview view mode. Please update issue summary about related issues about page-management roadmap (spark?)

Provider detection is broken.

+++ b/core/modules/display/lib/Drupal/display/DisplayListController.phpundefined
@@ -0,0 +1,63 @@
+    $layout = drupal_container()->get('plugin.manager.layout')->getDefinition($entity->layout);
+    $provider_info = system_get_info($layout['provider']['type'], $layout['provider']['provider']);

There's no "provider" key. Suppose we need a way to get module that provides this layout

andypost’s picture

Status: Needs work » Needs review
FileSize
10.68 KB
PASSED: [[SimpleTest]]: [MySQL] 48,241 pass(es). View
1.67 KB

Suppose label should be defined, also fixed lost provider key

Gábor Hojtsy’s picture

@andypost: I'd prefer to keep the patches involving this set of issues separately apply-able. #1787634: [META] Decouple layouts from themes has all the related issues, the provider key is set up in #1839278: Add layout template demonstration. Also, I fixed the label problem with #9. I also posted outstanding questions as to whether we want to make this module/code required, in which case the properties would go on the class, otherwise they should not go there and altered in by the module (like now). Discussing these would be preferred :)

Gábor Hojtsy’s picture

FileSize
94.21 KB
100.04 KB
PASSED: [[SimpleTest]]: [MySQL] 48,277 pass(es). View
97.66 KB
PASSED: [[SimpleTest]]: [MySQL] 48,290 pass(es). View
13.33 KB

Here comes an interesting update with two new things: layout hot-swapping demonstration and block additions/move around interactivity. The layout hot-swapping is gracefully degrading, the block interaction not at all.

1. Added a layout demonstration feature to the display configurator. When you change the layout, the demonstration reloads with Ajax. (None of the modified config you made before that is saved or kept in any way). The server side uses layout remapping with the Display so it should move around any blocks that exist (although in this example, none exist yet).

2. Added *prototype* styling from @tkoleary, see the CSS. There are many selectors cancelled out now with "z#...", those are not currently used. Left there for easy referencing.

3. Added *prototype* JS code to make adding blocks work. Since we don't yet have blocks as plugins (however it is just around the corner), this code just adds a div appropriately formatted for our block presentation with a random label. The popup for block configuration will come from the blocks as plugins patch: #1535868: Convert all blocks into plugins

4. Added *prototype* JS to move around the resulting regions using simple jQuery UI sortables. There is nothing easier than this :D You can drag blocks around inside regions or across regions as you wish.

Note that *there is no code yet* to actually save these "blocks". Because the display system wants blocks as plugins, we'll need to continue mock them until they are committed. So I think we'll keep working with these pseudo blocks, so we can figure out the remapping, etc.

So once again, in short you can add "blocks", move them around, the regions of the defined template are being demonstrated properly BUT you cannot save it, and you will loose your blocks when you switch to another template.

There is also a Display error on the page, caused by internal problems within the Display system. I think this is pre-existing to my patch. I did not find any code in Display's that would let me set the blocks through the API and they would default to arrays in the abstract class, but not actually defaulting to that when instantiated. This needs to be explored in more detail.

EditMasterLayout.jpg

Gábor Hojtsy’s picture

(Take the second/bigger file from the previous comment, for some reason the smaller one was not properly removed from the post, it is missing some pieces).

Gábor Hojtsy’s picture

FileSize
9.44 KB
100.46 KB
PASSED: [[SimpleTest]]: [MySQL] 48,298 pass(es). View

- Changed all underscores in CSS selectors to hyphens.
- Wrapped JS code in once() and used context specific find()s so JS actions are only tied up once (with previous patches if you swapped templates, the add button was attached twice, etc).

Gábor Hojtsy’s picture

FileSize
53.19 KB
104.1 KB
PASSED: [[SimpleTest]]: [MySQL] 48,359 pass(es). View
7.1 KB

Worked on actually serialising the resulting block information in the blockInfo structure expected by displays:

- Added a generic textarea to the form that hold JSON to communicate the changes back to the server (not one by one as it happens so the server is not overwhelmed)
- The JavaScript *badly* *needed* predictable class names for layouts (and some predictability in layout markup) to make this work; so modified/fixed the existing layouts to have a common set of region classes
- The regions/blocks info list is paired up with the region type info on the submission side and the blockInfo array is produced as needed

HOWEVER I could not figure out why the blockInfo cannot be made to save, so I did not reach to implementing loading back the existing blocks and displaying them (since I could not make to save any).

LayoutEdit-1.jpg

sdboyer’s picture

we need to communicate back step by step, and use TempStore to save the in-progress object. JSON is an OK temporary measure, but this is exactly what the TempStore is for.

working on an in-depth review.

Gábor Hojtsy’s picture

@sdboyer:

we need to communicate back step by step, and use TempStore to save the in-progress object. JSON is an OK temporary measure, but this is exactly what the TempStore is for.

Well, I naturally prefer to post patches on notable steps and not dump everything at once on people, so they understand the pieces and the thought process. That does not seem to work here since I'm not getting reviews though... :/ Anyway I believe we'd store the built display object in tempstore on the server side, so we need to figure out where and how we build it, and a possible set of tools for that are in the patch. I do assume for example that configured blocks via the modal will be saved permanently, and not in the tempstore or we'll need to reference temporary blocks from a temporary edited display, which does not sound like fun, given they are independent objects. And if those blocks are saved, I'm not sure of the value of tempstore here, since you'll not be able to view the display live anyway until you save it, there is no preview functionality even on the northstar (ideal goal) design. Is the only reason consistency on the editing backend?!

Looking forward for your review and help in fixing the display save problem.

sdboyer’s picture

Status: Needs review » Needs work

ok, more review. i think we're making good progress here.

if we're going to add a displays module, then we should probably move all of the displays code that's currently in the layout module to that.

i'm working on fixing the issues with saving now, but putting this review up for some discussion while i do.

diff --git a/core/modules/display/config/display.bound.admin_master.yml b/core/modules/display/config/display.bound.admin_master.yml
new file mode 100644
index 0000000..65aaf23
--- /dev/null
+++ b/core/modules/display/config/display.bound.admin_master.yml
@@ -0,0 +1,5 @@
+id: admin_master
+label: Default admin layout
+layout: static_layout:layout__two-col
+layoutSettings: { }
+blockInfo:
diff --git a/core/modules/display/config/display.bound.front_master.yml b/core/modules/display/config/display.bound.front_master.yml
new file mode 100644
index 0000000..54cf310
--- /dev/null
+++ b/core/modules/display/config/display.bound.front_master.yml
@@ -0,0 +1,5 @@
+id: front_master
+label: Default layout
+layout: static_layout:layout__two-col
+layoutSettings: { }
+blockInfo:

we haven't yet put the logic in place that would, at install time, convert unbound displays to bound displays based on some global default layout, but once we do, these should probably be unbound displays.

+++ b/core/modules/display/display.module
@@ -0,0 +1,90 @@
+ * @param \Drupal\layout\Plugin\Core\Entity\Display $display

This should probably specify BoundDisplayInterface.

+++ b/core/modules/display/display.module
@@ -0,0 +1,90 @@
+ * @param \Drupal\layout\Plugin\Core\Entity\Display $display

This should specify BoundDisplayInterface, i think.

+++ b/core/modules/display/lib/Drupal/display/DisplayListController.php
@@ -0,0 +1,63 @@
+class DisplayListController extends ConfigEntityListController {

This can only be a stopgap at best. We can't have just one entity list controller for all displays, as they can serve any number of different purposes. If that means we have to further differentiate the config namespace on something like a per-use-case basis (e.g., "display.bound.page." for this use case), i think that's totally fine.

Gábor Hojtsy’s picture

@sdboyer: (I removed two duplicate pieces from your comment after carefully checking it was probably a dreditor issue). For your comments:

1. On display module, I used this because I have similar changes in layout module for the demonstration at #1839278: Add layout template demonstration. It has been RTBC for 3 days. Once/if it is committed, we can move all this to layout module. I wanted to have patches that independently apply so people can test things without complicated dependency chains.

2,3,4. Agree with these, but the installer feature is not implemented, so it is not implementable on our side either ATM.

5. For the last one, that sounds interesting...

This can only be a stopgap at best. We can't have just one entity list controller for all displays, as they can serve any number of different purposes. If that means we have to further differentiate the config namespace on something like a per-use-case basis (e.g., "display.bound.page." for this use case), i think that's totally fine.

Are you advocating NOT implementing this as a config list controller or inheriting a class for master displays only, so we can list them here? As it is now, each (config) entity can only have one list controller class assigned (with one listing URL bound to it). I specifically altered in these so that we can discuss whether to make those changes to the class annotation itself or not.

sdboyer’s picture

Are you advocating NOT implementing this as a config list controller or inheriting a class for master displays only, so we can list them here?

yes, i'm advocating not implementing them as a config list controller. we're designing some patterns in core for how displays will be used, but we need to expect contrib will create others. as you point out, there can be only one list controller; therefore, nobody should use it. we might reuse some of the same patterns, or even classes (which would set a good pattern for contrib to follow), but we shouldn't bind it so tightly to the entity type.

sdboyer’s picture

Status: Needs work » Needs review
FileSize
105.44 KB
FAILED: [[SimpleTest]]: [MySQL] 48,777 pass(es), 6 fail(s), and 0 exception(s). View
2.2 KB

i'm not sure what i'm doing wrong, but i can't get the visual thing to come up, at all. i wonder if it's some misapplication of the binary data in the patch...

can we put this in a branch somewhere, maybe in the scotch repo? it'll make it easier for me to collab.

the reason why blockInfo wasn't saving is because of this:

class ConfigStorageController {
  protected function getProperties(EntityInterface $entity) {
    // Configuration objects do not have a schema. Extract all key names from
    // class properties.
    $class_info = new \ReflectionClass($entity);
    $properties = array();
    foreach ($class_info->getProperties(\ReflectionProperty::IS_PUBLIC) as $property) {
      $name = $property->getName();
      $properties[$name] = $entity->$name;
    }
    return $properties;
  }
}

this doesn't work for displays, as a number of the properties that need to be written out are protected. the solution here is our own storage controller...which i want to do anyway in order to reduce some of the dense logic on the default ConfigStorageController (holy...crap).

i'm opening a separate issue to fix this, as i think it's better kept separate and get in quickly and easily: #1849480: Allow ConfigEntity classes to control how they are saved.

with that addressed, though, we still aren't writing out blockInfo properly, because it's not being set up quite right. here's the issue:

+++ b/core/modules/display/lib/Drupal/display/DisplayFormController.php
@@ -0,0 +1,157 @@
+  public function form(array $form, array &$form_state, EntityInterface $display) {
(...)
+    $regions = $display->get('blockInfo'); // <-- here's the issue
+    $form_state['values'] += array(
+      'layout' => isset($display->layout) ? $display->layout : reset($layout_keys),
+      'regions' => empty($regions) ? array() : $regions,
+    );

blockInfo is not a region-sorted three-level array. it's a two-level array; the first level has keys that are block config names, each of which contain an array with that block's config info. what you want here is $display->getAllSortedBlocks()...sorta. in the current code, that just returns the block config names as an indexed array, ordered correctly for output in the region. but it doesn't include the info; the expectation is that client code will assemble that together. the motivation for this was that block info ordered by region is neither the final storage format, nor what's most useful at render-time (since we do not necessarily render each block in sequence by region).

however, it's clearly complicating client code unnecessarily here, so i've made that method return the full set of data you want here :)

now, some further review points:

+++ b/core/modules/display/lib/Drupal/display/DisplayFormController.php
@@ -0,0 +1,157 @@
+  public function form(array $form, array &$form_state, EntityInterface $display) {

we have to use EntityInterface instead of DisplayInterface to avoid a strict warning on compat with EntityFormController's signature, but we should should add an instanceof check immediately inside to ensure that $display is a DisplayInterface.

...assuming we stick with using these classes at all, per the other line of discussion.

+++ b/core/modules/display/lib/Drupal/display/DisplayFormController.php
@@ -0,0 +1,157 @@
+    $display->remapToLayout($layout);

this means we remap on initial form build. if we have a built, bound display, then there's no reason to remap right up front.

remapping to a different layout should be a user-mediated process that happens out of band with the normal form. i wrote a big email about this to Kevin about 10 days ago, did it make the rounds with everyone?

+++ b/core/modules/display/lib/Drupal/display/DisplayListController.php
@@ -0,0 +1,63 @@
+    $provider_info = system_get_info($layout['provider']['type'], $layout['provider']['provider']);

this 'provider' value isn't set by the layout plugin itself, and it's tossing out a bunch of errors on the list interface.

are we expecting it to be added by plugin processing in the layout manager? b/c it's not, currently.

Status: Needs review » Needs work

The last submitted patch, display-ui-1841584-22.patch, failed testing.

Gábor Hojtsy’s picture

@sdboyer:

- the provider information is added in #1839278: Add layout template demonstration which has been RTBC for 10 days but got no committer attention yet

- what is wrong with the "visual thing"? can you provide a screenshot? there is no such URL is just errors, or?

- as for the use of remapToLayout() the form has a layout template selection dropdown, which when changed will rebuild the form with AJAX; I've been following the AJAX form pattern in that you'd need to build the whole form with the latest data at all times instead of relying on external data (given that the data might change the form); one such change here is when a different layout template is selected in the form; I did not expect for it to be a problem for a display to be remapped to a layout that is it's existing layout already; should people need to check outside calling this method if the layout they want to remap to is different to what it is already or is it better to improve this method to skip doing stuff in that case? :)

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
3.44 KB
107.6 KB
FAILED: [[SimpleTest]]: [MySQL] 48,789 pass(es), 5 fail(s), and 0 exception(s). View

Took a crack at fixing the test failures. Now that more complete arrays are returned, we need to check for more complete arrays. But then assertIdentical just uses === which does not do recursive array comparison, so although the error message shows the arrays compared are exactly equal in content, this patch will still fail the same way.

I did not want to make up a recursive array identical assert myself for this one, so decided to post this instead and ask for feedback as for the best way to check this. It is also likely that we are testing too much if we test the identical array structure.

Status: Needs review » Needs work

The last submitted patch, displays-25.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
3.99 KB
110.95 KB
FAILED: [[SimpleTest]]: [MySQL] 48,978 pass(es), 6 fail(s), and 0 exception(s). View

@sdboyer extracted some code to make the display saving work from #1849480: Allow ConfigEntity classes to control how they are saved which turned out to needing more discussion. Including that and rerolling now that #1839278: Add layout template demonstration is committed. I'll work on making the tests pass and integrating the code with layout module now.

Gábor Hojtsy’s picture

FileSize
54.21 KB
52.4 KB
114.87 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/layout/layout.module. View

Given that #1839278: Add layout template demonstration got committed, I rolled this code into layout module (instead of display module). This cleaned up some naming things. Also now there are no patches other than this to make this work. @sdboyer's code ensured that saving now works well. I put in some more sample data (which shows off some of the problems of the UI), so now you can go and edit and save displays.

Yes, technically it does not use tempstore yet, the hotswapping of layouts works somewhat (existing blocks are remapped) but the reorganization will not work (due to AJAX/DOM/JS mixups when the form is regenerated). So more help / work is needed here even for the basics and we are not even there to remove blocks, etc.

(No interdiff because I moved around almost all the things).

Layout list:
Layouts.jpg

Layout editing:
EditLayout.jpg

Tests still not fixed, doing that next.

Status: Needs review » Needs work

The last submitted patch, display-ui-28.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
114.21 KB
FAILED: [[SimpleTest]]: [MySQL] 48,931 pass(es), 2 fail(s), and 0 exception(s). View
2.79 KB

Updated tests as per discussion with @sdboyer to test only the array keys of returned values. Using the region specific API for simplicity. Should pass tests now.

It would be great to have credible JS/CSS/AJAX expertise rolled on this patch to make the front end actually work better. I can continue dabble in some basic JS/CSS but that will probably not lead to widespread cheering.

(UI/capabilities unchanged from #28).

Status: Needs review » Needs work

The last submitted patch, display-ui-29.patch, failed testing.

MustangGB’s picture

#28: In the "Layout list" screenshot Two column is under the header Template, but in the "Layout editing" screenshot the label of the dropdown is Layout. I think for consistent UX these should both be the same, i.e. the "Layout editing" label needs to be changed to Template as well.

Also (this might already be the case, so please tell me to STFU if needs be), but on the region objects within a Template declaration is there (or can we add) a simple property to say whether the blocks added to this region will be displayed vertically or horizontally. We could then lever this information in the "Edit layout"/"Add blocks to regions" page to give a better representation to the user how their page will look.

Gábor Hojtsy’s picture

FileSize
660 bytes
114.21 KB
FAILED: [[SimpleTest]]: [MySQL] 49,298 pass(es), 2 fail(s), and 0 exception(s). View

@akamustang: good catch on that terminology problem. Fixed that.

As for the vertical/horizontal placement, placement of the blocks within the regions is a whole minefield. The actual block content in itself might dictate some width requirements / constraints that the display will need to take care of. I think the thinking is that "block styles" will solve this but not in core. Core will just tack them under each other exactly like currently in Drupal 7. We need to pick battles as to what we attempt to solve in core.

frega’s picture

I'll have a go at converting the involved JS to backbone - there seems to be an ample amount of client-side logic involved in this. A little mv*-structure would be beneficial, imho. Initial battleplan: http://goo.gl/expgJ - i'll try to post a patch later today.

frega’s picture

FileSize
20.62 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch frega-display-ui-35.patch. Unable to apply patch. See the log in the details link for more information. View

First stab attached. This is a very much WIP. Drag'n'drop, adding and deleting (random id-blocks) works but switching templates currently breaks things. I shoved all the backbone stuff into one file, grunt & build comes later.

Attached patch incorporates #29/#33.

Bojhan’s picture

Issue tags: +Usability, +layouts

Could you add screenies? Makes it easy for the non-devs to follow your progress :)

frega’s picture

FileSize
14.27 KB
122.55 KB
FAILED: [[SimpleTest]]: [MySQL] 49,285 pass(es), 3 fail(s), and 0 exception(s). View

Ok, tried again.

frega’s picture

FileSize
27.43 KB
134.65 KB
FAILED: [[SimpleTest]]: [MySQL] 49,282 pass(es), 2 fail(s), and 0 exception(s). View

Now slightly more complete (if not a lot prettier)
- Modal/Dialog integration - add new block instances, Remove/"Configure" existent block instances
- Drag and Drop, across regions
- Saving works
- Split up files & grunt included

Switching Templates doesn't work yet either.

Hope the patches are ok, trying the second approach to interdiffing now :)

Interdiff.txt is obviously #33.

Bojhan’s picture

Status: Needs work » Needs review

Looks like this is in "needs review" ? :)

Status: Needs review » Needs work

The last submitted patch, display-ui-1841584-38.patch, failed testing.

ry5n’s picture

Pitching in where I can by reviewing the CSS.

+++ b/core/modules/layout/js/theme.js
@@ -0,0 +1,39 @@
…
+          '<div class="region-actions">' +
+            '<div class="add-block"><span class="plus-icon add-block"></span></div>' +
+          '</div>' +
  • Should this span have the add-block class?
+++ b/core/modules/layout/layout.admin.css
@@ -15,3 +15,320 @@
…
+/**
+ * Begions.
+ */
  • Typo
+++ b/core/modules/layout/layout.admin.css
@@ -15,3 +15,320 @@
…
+#display-blocks .add-block-press {
  • (:1228) This looks like it might be styling an active state? If so, see if the :active pseudo-class might be used instead.
+++ b/core/modules/layout/layout.admin.css
@@ -15,3 +15,320 @@
…
+z#display-blocks .layout-block {
…
+z#display-blocks .page-block {
  • (:1188 and :1199) Accidental 'z' characters inserted

More generally:

  • We probably don't need to support the original -webkit-gradient(linear…) syntax.
  • We should support linear-gradients in Opera with the -o prefix.
  • Since the linear-gradient syntax has changed so many times, consider being explicit with the gradient direction: e.g. -{prefix}-linear-gradient(top, …) for prefixed and linear-gradient(to bottom, …) for official.
  • Double-check that styles can be re-used where applicable. For example, make sure that #display-blocks .layout-block will only ever appear inside #display-blocks. If it or something that looks similar might appear elsewhere, consider un-scoping it as a single re-usable class, something like .admin-layouts_layout-block (or something less verbose).
  • Also consider grouping repeated style blocks (like :1178–:1181, :1189–:1192) as a base class, e.g. .admin-layouts_block. Then add a second class and style rules to create a variant that 'extends' the base class: .admin-layouts_block-master extends .admin-layouts_block.

Kudos to everyone working so hard on this! :)

Gábor Hojtsy’s picture

@ry5n: the z chars are inserted to mark selectors which are not currently in use but were int @tkoleary's original HTML/CSS prototype. They are not accidental :D

ry5n’s picture

@Gábor Oh! I haven't seen that before. I guess it's a case of “You learn something new every day” :)

frega’s picture

FileSize
135.34 KB
FAILED: [[SimpleTest]]: [MySQL] 48,931 pass(es), 2 fail(s), and 0 exception(s). View

A small reroll of #38 to fix a reordering/rendering issue. There are still remaining issues (e.g. switching templates doesn't work, dragging onto an empty region doesn't work, etc.) and this there is no visual improvement/UX love in this, so far.

Before doing more substantial JS-work (or screenshots or in-depth reviews) on this, I think though, a little more time on the defining the architecture might be needed:

  1. a written spec that details what goes where (the screens/mockups provide a lot of good ideas and points but I am not sure what's within the scope of this initial issue or outside of it, tbh).
  2. as this is similar to placing "panels' content-placement" logic in core, we should co-ordinate with other aspects in the area (e.g. FieldUI & Entity Display ("panelizer in core") -> http://drupal.org/node/1852966)
  3. discuss what (and if at) we retrieve in a REST-ful way (e.g. is there a resouce /entity/block listing all available blocks etc.) and what needs to remain maximally alterable and go via the Form API (e.g. Configuration Forms?)
Gábor Hojtsy’s picture

@frega: I think we do need to define the scope of this issue ourselves :) I think at a minimum, we should be able to configure these two master layouts (no other master layouts allowed for now) the configuration meaning we should be able to place (add and/or move around) and remove blocks as needed. Locking blocks and other extra features might or might not go into this issue. Instead the core of this getting in core would be to figure out how to make it accessible (in reference to previous brainstorm, an idea is to have a "show weights and regions" link at the top and make dropdowns for both appear in all blocks, so those not being able to move around things can do block reorganization).

frega’s picture

FileSize
139.02 KB
FAILED: [[SimpleTest]]: [MySQL] 48,923 pass(es), 2 fail(s), and 0 exception(s). View
38.02 KB

Next iteration, even more work in progress. Signifcant new feature is persisting changes via RESTful webservice and TempStore.

- TempStore integrated (but not 100% complete, all very new to me incl. some "blind" copy and paste from views_ui) .
- New menu loader function %layout_master_cache
- (Temporary) Webservice on admin/structure/layouts/manage/webservice handles POSTs from the BB-App (non-FAPI-based communication)
- Changes are temporarily persisted in TempStore and then "saved" properly on clicking "Save" (no revert available atm :).

- Activated a RegionModal to prepare for region-based configuration Form.

Open issues remain (e.g. switching templates flaky, dragging onto an empty region doesn't work).

frega’s picture

FileSize
43.56 KB
144.14 KB
FAILED: [[SimpleTest]]: [MySQL] 48,925 pass(es), 2 fail(s), and 0 exception(s). View

This should now mimic views_ui's usage of temp store. Includes cancelling unpersisted changes and locking as well as breaking of other users locks.

Open issues remain (e.g. switching templates flaky, dragging onto an empty region doesn't work).

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Reviewed the functionality of the patch first:

- The tempstore function seems to be working really well, it feels effortlessly quick and if I navigate away I can come back and see the same unsaved changes. Cool.
- I see you moved the big "+" away from the right side of the region. I believe it was specifically designed to be that big and prominent and loosing that seems to be unfortunate.
- I've also seen you added a configuration gear on the region, which is not necessary at all. I don't know what you'd configure there, the current functionality specs for Drupal 8 definitely do not have anything to configure there. The region type which is the only property of a region besides its machine name and label is predefined and cannot/should not be changed I believe.

Next up is a code review, although I'm definitely not the best person to review involved JS code :)

yoroy’s picture

Issue tags: +JavaScript

Lets summon nod_ for the javascript then :)

Gábor Hojtsy’s picture

Issue tags: -JavaScript

Some hopefully useful comments on the code:

+++ b/core/modules/layout/grunt.jsundefined
@@ -0,0 +1,59 @@
+module.exports = function(grunt) {
+  // Project configuration.
+  grunt.initConfig({

Do we have doc standards for grunt.js files? A header comment would be great to summarise what this is about. There is a general lack of helper comments on other files as well :)

+++ b/core/modules/layout/js/models/block-model.jsundefined
@@ -0,0 +1,30 @@
+        // in real life this would need to come from somewhere else ..
+        // maybe time for UUID.js
+        'id': this.get('id'),
+        'label': this.get('id'),

What do you mean?

+++ b/core/modules/layout/js/theme.jsundefined
@@ -0,0 +1,48 @@
+/**
+ * @file
+ */
+(function ($) {
+  /**
+   * Theme function for a region.
+   * @param id
+   * @param label
+   * @return {String}

Way better than hardcoding HTML stuff :D Cool. Are we using this phpdoc standard though in JS or was just autogenerated in a wrong format?

+++ b/core/modules/layout/js/views/app-view.jsundefined
@@ -0,0 +1,49 @@
+  // sad panda.
+  Backbone.emulateJSON = true;
+  Backbone.emulateHTTP = true;

Why? :)

+++ b/core/modules/layout/js/views/app-view.jsundefined
@@ -0,0 +1,49 @@
+      // @todo: let's do this in a better way.
+      if (this.settings.locked) {
+        return false;
+      }

What does this locked property mean?

+++ b/core/modules/layout/js/views/region-view.jsundefined
@@ -0,0 +1,97 @@
+      // make sure we gather a bunch (300ms) of change events before we push the
+      // last to the server. @todo: make sure that the submit button is disabled
+      // so that we don't have crazy race condition thingies.
+      blockInstances.on('change', _.debounce(function() {
+        Drupal.layout.appModel.save();
+      }, 100), this);

Looks like this is not actually 300ms or I'm misreading the code? Is 300ms (or 100ms) enough to avoid overruning HTTP requests? How is that avoided if the request time is longer?

+++ b/core/modules/layout/js/views/regionmodal-view.jsundefined
@@ -0,0 +1,23 @@
+/**
+ * @file
+ */
+(function ($, _, Backbone, Drupal) {
+
+  "use strict";
+
+  Drupal.layout = Drupal.layout || {};
+  Drupal.layout.RegionModalView = Drupal.layout.ModalView.extend({
+    events: {
+    },
+    render: function() {
+      this.$el.html(
+        '<div>Configure region type? Load some FAPI form here.</div>' +
+        '<button class="dialog-cancel">' + Drupal.t('Close') + '</button>'
+      );
+      this.show();
+      return this;
+    }
+  });
+
+})(jQuery, _, Backbone, Drupal);
+

As said above, I don't think we have any use for this one.

+++ b/core/modules/layout/layout.admin.incundefined
@@ -73,3 +75,102 @@ function layout_page_view($key) {
+/**
+ * Page callback: temporary webservice.
+ *
+ * @param Drupal\layout\Plugin\Core\Entity\Display $display
+ * @param null $a
+ * @param null $b
+ * @param null $c
+ * @return int
+ */
+function layout_master_webservice(Display $display, $a=NULL, $b=NULL, $c=NULL) {

The documentation is not very helpful :)

Is this protected in any way from scripted attacks like forms with one time tokens? Would that kill the navigate-away possibility for the form? Any other way to solve this not being an easy attack point to screw up your site layout?

+++ b/core/modules/layout/lib/Drupal/layout/DisplayFormController.phpundefined
@@ -0,0 +1,228 @@
+            '!user' => theme('username', array('account' => user_load($display->locked->owner))),
+            '!age' => format_interval(REQUEST_TIME - $display->locked->updated),
+            '!break' => url('admin/structure/layouts/manage/' . $display->get('id') . '/break-lock')

!age and !break should be @age and @break.

+++ b/core/modules/layout/lib/Drupal/layout/DisplayFormController.phpundefined
@@ -0,0 +1,228 @@
+    // @todo: properly handle storing template / other form values
+    // this is a all bohemian villages to me ...

ha, what? :)

+++ b/core/modules/layout/lib/Drupal/layout/Plugin/Core/Entity/Display.phpundefined
@@ -183,4 +183,14 @@ public function getLayoutInstance() {
+  /**
+   * Submit handler to break_lock on a display.
+   * @todo: is this the best place? copied from ViewsUI.
+   */
+  public function submitBreakLock(&$form, &$form_state) {
+    drupal_container()->get('user.tempstore')->get('layout')->delete($this->get('id'));
+    $form_state['redirect'] = 'admin/structure/layouts/manage/' . $this->get('id') . '/edit';
+    drupal_set_message(t('The lock has been broken and you may now edit this display.'));
+  }

It does not seem to be related to the display itself, since it does not know / handle locks(?)

Gábor Hojtsy’s picture

Issue tags: +JavaScript

Cross-post.

frega’s picture

Thanks for the review!

- The tempstore function seems to be working really well.

There are still some superfluous XHR-requests on reordering going over the wire, but that should be easily fixed.

- I see you moved the big "+" away from the right side of the region. I believe it was specifically designed to be that big and prominent and loosing that seems to be unfortunate.

I spoke to Bojhan about this in IRC. We can switch back to that behaviour/design later on, but it would require changes to the structure of HTML and the CSS and I wanted to avoid that in the initial stab.

- I've also seen you added a configuration gear on the region, which is not necessary at all. I don't know what you'd configure there, the current functionality specs for Drupal 8 definitely do not have anything to configure there. The region type which is the only property of a region besides its machine name and label is predefined and cannot/should not be changed I believe.

Easily removed. It might make sense to keep this, though, if e.g. we need to provide a (more accessible, more mobile etc.) traditional form-based "reordering interface" for the block instances.

Status: Needs review » Needs work

The last submitted patch, display-ui-1841584-47.patch, failed testing.

frega’s picture

FileSize
16.09 KB
146.12 KB
FAILED: [[SimpleTest]]: [MySQL] 48,912 pass(es), 2 fail(s), and 0 exception(s). View

Ok, reroll. I hope i addressed everything in the review.

  1. grunt.js Add a @file block to the grunt-file added drupalSettings to the globals. Not aware of any guidelines. Can also be just removed, helps w/ development though.
  2. js/models/block-model.js - improved documentation - we need unique ids for all BlockInstances these need to be generated server-side (or via a js-based UUID implementation?)
  3. js/views/app-view.js - added documentation why we need emulateJSON and emulateHTTP (REST-verb based routing and application/json handling)
  4. js/views/app-view.js - added documentation re: TempStore "locked" state
  5. js/views/region-view.js - fixed the debounce-timeframe (but still too eagerly POSTing/PUTing)
  6. js/views/regionmodal-view.js - i left this in because a) we should think about whether we need an alternative and more a11y-friendly (perhaps also mobile friendly) approach instead of d'n'd (plus: keeping in mind other usecase of content-placement within containers in d8 => fieldui, layout manager etc.)
  7. layout.admin.inc - added more documentation. CSRF is inevitably going to be a concern w/ any RESTful stuff - because a "per request token" is not really "restful"; we should at least bring a per-session "CSRF"-token (e.g. a -tag like Rails does afaik) into core.
  8. lib/Drupal/layout/DisplayFormController.php - fixed and removed flippant comment.
  9. lib/Drupal/layout/Plugin/Core/Entity/Display.php - moved to a "traditional" submit-handler (the forms will be OOPified anyway soon)

Additionally: made sure that there are @file blocks in all JS and fixed a few small issues/typos etc. Bugs/lacking features mentioned in #47 still apply.

frega’s picture

Status: Needs work » Needs review

Setting to "needs review" (although it needs work as well ...)

Status: Needs review » Needs work

The last submitted patch, display-ui-1841584-55.patch, failed testing.

Bojhan’s picture

I will work on some designs for the issue raised by Gábor,

nod_’s picture

*appears in a puff of smoke*

I had a quick look at the code, there is still hardcoded HTML that isn't in Drupal.theme.something.

More generally, that's a lot of JS for something that is essentially similar to dashboard with modals and stuff added to it. Lots of boilerplate code too. I could use a walk-through to understand the real value of using backbone here.

jessebeach’s picture

FileSize
157.23 KB
PASSED: [[SimpleTest]]: [MySQL] 53,046 pass(es). View

Amazing work so far frega.

nod_, my understanding of this work is that we're moving towards a UI that can configure static layouts and then eventually responsive layouts. My intent was to move the responsive layout builder to Backbone eventually. This work will dovetail nicely with it.

I went through and refactored the CSS to match our .base and .theme standards. This involved changing some class names and therefore some of the templates. I tried really really hard not to break any functionality. I'm sorry if I did.

One other thought, in region-view.js, the render function

render:function () {
  this.$el.html(Drupal.theme.layoutRegion(this.model.get('id'), this.model.get('label')));
  this._collectionView.setElement(this.$el.find('.row', '.blocks'));
  this._collectionView.render();
  return this;
}

I don't understand why the collectionView element is being set in this region's render function. If collectionView is a container for multiple regions, would it not be better to create the instances of region views in the collectionView?

frega’s picture

@nod_: there are HTML-chunks in the modal-views but those should and probably will be replaced very soon w/ FAPI-forms loaded from the server - so i didn't bother with Drupal.theme-ing those. Please also note: we currently parse the HTML generated on the server to generate Models and Collections; I'm going to replace that as well - but I am not sure how comfortable people are with pages that render entirely on the client-side - what's your take on that (raises all kinds of "extensibility issues")?

Regarding the overall meaningfulness of using Backbone:

First, it's sort of a matter a principle: I don't think that we should store state in the DOM, ever. Serializing and deserializing from and into the DOM does not "scale". jQueryUI-widgets IMHO are not fun to work with (ask jesse or wim). Backbone can be verbose and definitely entails more "boilerplate", but it provides a saner and more decoupled approach than a DOM/jQueryUI based one.

Secondly, framing this as a "simple dashboard" underestimates the objective and implicit complexity both in terms of user interactions and of state/models. The objective, to me at least, is to start with a working prototype that can solve the (simple) use-case of "Block" placement for "Master Layouts", and in doing so lays meaningful groundwork for similar "GUI-challenges" down the road (FieldUI's "manage field" and the responsive layout builder jesse mentions).

I'd be happy to walk you through the code - if you have categorical concerns (there are plenty imaginable) or if you don't see how it "jives" with the overall strategy in D8, please tell me: I would not want to pursue this approach (or much spend time on it) otherwise.

@jessebeach: thanks for the adjustments, couldn't find any regressions so far.

Regarding the repeated setting of the nested _collectionViews' DOM-element - this is simply needed because at initialize()-time the containing region hasn't rendered yet and thus "

" hasn't been painted. Or do you mean something else?

jessebeach’s picture

I, personally, am OK with a UI generated completely by JavaScript for this component of the admin experience, especially because everything we're doing can be done through code if one really doesn't want to have JS enabled in their client. Sure, it wouldn't be convenient, but neither is building a complex UI that falls back to a non-JS functional state for a marginal minority of folks. We can also address aural and keyboard access in a complex GUI. There's no need to think about a non-JS version as the accessible version. Accessibility is what we make of it.

Regarding the repeated setting of the nested _collectionViews' DOM-element - this is simply needed because at initialize()-time the containing region
hasn't rendered yet and thus "" hasn't been painted. Or do you mean something else?

I'm not 100% solid on what I'd recommend here yet. My gut feeling is that children of a container should not necessary know anything about what contains them. The container should mediate communication to the objects it contains and be involved in their CRUD, rather than the other way around. The contained objects should only be concerned with their very focused tasks. In the case of a Region, that would be the task of managing blocks. Does that make sense? Maybe I just need to propose some code and see if what I'm babbling about makes sense here.

frega’s picture

FileSize
15.12 KB
152.13 KB
FAILED: [[SimpleTest]]: [MySQL] 53,077 pass(es), 1 fail(s), and 0 exception(s). View

Had a little idle time so tried to improve on this.
- Switch from DOM parsing to passing JSON to instantiate Models and Collections from drupalSettings.
- Fixed minor regression in #60 (region-view.js configure click event caused page reload)
- Replaced RegionsView with a UpdatingCollectionView (currently RegionsView has no distinctive logic)
- Improved UpdatingCollectionView by adding the nestedViewContainerSelector-option which allows to specify into which sub-element of a view the UpdatingCollectionView should "render"
- Added a exportGroupedByRegion method to Display to allow stripping DisplayFormController from assembling that data.
- Cleanup code in places

Switching between Templates and dragging to empty regions still doen't work.

Big questions for me are:
a) decide on whether this makes sense to pursue
b) figure out how to best load FAPI forms into dialogs
c) Leverage Drupal's new router to be more restful, refactor Webservice and introduce some form of CSRF protection

Bojhan’s picture

FileSize
33.5 KB

Just trowing an idea in there, this removes the large plus, to a small one. I changed some of the visual styling so it still stands out. As I used the initial design, I found the large plus a little uncomfortable it takes up a lot of space when you have a more crowded layout and gives the illusion, there is another zone.

frega’s picture

The current prototype looks sort of similar to what Bojhan outlined. I enabled overlay temporarily :) and took a few snaps.

Initial:

On clicking + an unstyled dialog with random blocks pops up.

If you modify the layout by moving, adding or removing block instances, changes are store in TempStore immediately and a views-like change notification is shown (yes buttons should change colors probably, too)

Please not the "gear" icon on the region is temporarily placed there and unstyled. In the "Layout Master" it doesn't have a purpose, in other related GUIs a configuration on a "container" level would probably make sense.

tim.plunkett’s picture

+++ b/core/modules/layout/lib/Drupal/layout/Config/DisplayBase.phpundefined
@@ -135,4 +135,22 @@ public function getAllRegionTypes() {
+    // Start by exporting the public properties.
+    $class_info = new \ReflectionClass($this);
+    $properties = array();
+    foreach ($class_info->getProperties(\ReflectionProperty::IS_PUBLIC) as $property) {
+      $name = $property->getName();
+      $properties[$name] = $this->$name;

+++ b/core/modules/layout/lib/Drupal/layout/Config/DisplayStorageController.phpundefined
@@ -0,0 +1,25 @@
+  public function getProperties(EntityInterface $entity) {
+    if (!$entity instanceof DisplayInterface) {
+      throw new \RuntimeException(sprintf('DisplayStorageController::getProperties can only work with objects implementing Drupal\\layout\\Config\\DisplayInterface, object of type "%s" was given.', get_class($entity)));
+    }
+
+    return $entity->export();

Why can't this just be:

public function getProperties(EntityInterface $entity) {
  $properties = parent::getProperties($entity);
  $properties['blockInfo'] = $entity->getAllBlockInfo();
  return $properties;
}

keeping the exception and all of course, but this is a bizarre workaround

frega’s picture

FileSize
15.5 KB
155.17 KB
PASSED: [[SimpleTest]]: [MySQL] 53,145 pass(es). View

Another iteration, now switching templates and dragging to empty regions works, removed some duplicate requests to the webservice.

I had to hack my way around some limitations (or design?) of FAPI/Drupal.ajax related stuff (see DisplayFormController.php:88-89 and layout.admin.js:72-73 - could also be due to the fact that i don't fully grok either).

Still missing loading FAPI-forms into the dialog.

I have left the issue raised in #66 unaddressed because I know nothing about that part of D8.

nod_’s picture

So I reviewed the code this week-end.

  1. There are still duplicate Ajax calls for the layout thing.
  2. As it it also poses the question of the first non no-js fallback functionality.
  3. More generally I don't see the benefit for backbone here.
  4. Code is somewhat hard to understand. I had to look at the AJAX request to figure things out. The format for those is pretty simple thankfully.
  5. Code seems kind of convoluted in some places.

To me having the jquery sortable properly configured and some ajax calls here and there would do the trick. sortable has code to serialize DOM elements to a sorted list and publish enough events to react to most/all things. Also storing the state/config of the blocks might end up necessary. It's possible we'll want to add the block thing to wysiwyg and the only way to make copy/paste work properly is having all the data on the DOM element.

I'd favor a more direct approach, it's currently more complicated than it needs to be.

frega’s picture

Hi nod, thanks for the review.

Re: 1. do you refer to the same request going over the wire twice (#67 removed only "some duplicate requests") - that can obviously be rectified but seems a somewhat pointless exercise right now.

Re: 2. there is no "no-js fallback" as the whole thing doesn't work w/o Javascript at all (#64) :)

Re: 3. and the general gist of your review - it is certainly possible to do this w/o Backbone using e.g. jqueryui widgets, drupal.ajax, FAPI and some events. I am not unaware of the boilerplate, complexity and the "impedance mismatch" between REST and FAPI, but I wouldn't want to pursue a "some ajax calls here and there"-approach. Especially if reuse is intended.

The patch is still far from that a properly decoupled approach using backbone and webservices on top of a proper API, would make sense imho - whether that's "core" stuff and what implications that has (maintaining extensibility via modules/PHP and JS) is though above my paygrade :)

As to 4.-5. that's hard to respond to without concrete examples, but please bear with me a little bit as this is only my second stab at D8 and grokking D8 can be quite a mouthful :)

frega’s picture

FileSize
165.84 KB
PASSED: [[SimpleTest]]: [MySQL] 53,170 pass(es). View
18.62 KB

One last iteration - now with backbone and Drupal.ajax :) It now includes a FAPI-form loaded in to a dialog. The prototype - if very ugly in appearance and parts of the code - can now do CRUD: add new blocks, move them around, update the block instance "config" (for want of meaningful things to configure just the block label) and delete block instances. Layouts can be switched on the fly (even if the logic for re-sorting them isn't very helpful).

The patch now *deliberately* combines Drupal.ajax and Backbone. Drupal.ajax is used for the block instance configuration modal (click on the gear that appears when you hover a block instance). Backbone handles adding, deleting (click the ugly "-" button), moving the block instances as well as the rendering. Please note how the two intertwine in js/layout.admin.js:7ff - I find that considerably more compelling than Drupal's traditional AHAH/AJAX stuff, but that might/will be contested :)

I don't think it makes a lot of sense for me to continue (and finetune) beyond this point without knowing whether to continue w/ Backbone or rip it out and replace it. I still think it is worthwhile to think what a "client-side driven approach" (based on a js-client backbone) could give us in a variety of use-cases, but am happy to roll either way.

sdboyer’s picture

sorry i've been absent from this for a while.

first, a general note - in theory, i'm excited about backbone, because the DnD block placement interface is one that i expect we'll want to see lots of different versions of. we certainly could do something simpler using what jQuery UI provides us - indeed, i did do just such a thing in contrib, albeit quite crappily. but if using backbone means creating an API that will be easier to repurpose both because:

a) people know backbone, and it'll be easier for them to grok.
b) backbone provides API layers that make it easier to reuse and repurpose what we've put in core.

i don't know backbone for shit, so these are completely generic statements about pros typically associated with using a framework.

my expectation is that this UI will be re-done not on a site-by-site basis, but by a few modules in contrib and especially in distros. which means that our API target should be more skilled developers, and creating a well-structured environment that eases the process of figuring out what differentiates a given implementation...and maybe even some kind of interoperability.

@tim.plunkett #66 - since #1849480: Allow ConfigEntity classes to control how they are saved went in, it seems the separation of concerns question has been decided. this patch can now be refactored to remove the custom workaround, since it's now in the base config storage controller.

Bojhan’s picture

Title: Add page to configure master displays (called master layouts on the UI) » Add and configure master displays

So what is this blocked on?

nod_’s picture

  1. We need a basic no-js fallback, meaning integrating most of this things through Form API.
  2. Then we can add fancy JS things on top of it. Backbone or not doesn't matter at this point since we don't have the fallback yet.

1. is definitely in this issue, 2. can be split of for later probably, it'll take some elbow grease to make 1.

Bojhan’s picture

Awesome, sounds like a solid to do. I have created #1871746: Add modal block browser

larowlan’s picture

will try and look at 1 next week

svettes’s picture

Hia! Just wondering if there was any update on this? :)

Bojhan’s picture

Version: 8.x-dev » 9.x-dev

Looks like this won't happen for D8 either, its quite a massive task still.

sdboyer’s picture

Version: 9.x-dev » 8.x-dev
Category: feature » task

was just talking about this today with Shannon and Kris. not doing this means is pretty much saying that we have to throw all of SCOTCH out, and my impression elsewhere is that we don't want to do that - given that we're now at the point of being able to build and render a display out (at least in patch form - #1812720: Implement the new panels-ish controller [it's a good purple]).

basically, this is a required conversion if we want any of SCOTCH to be viable, which is required if we want the ESI/structured page model/promise of WSCCI. so, i'm switching this back to 8.x and calling it a task. let the argument begin :)

Bojhan’s picture

@sdboyer Basically you have to take this with the maintainers, it is my impression that you guys have and still are focused on API fundamentals. There is not enough man power and or vision, to provide a good UI for this. Even if we do manage to hash out something, it will still not match an MVP that is desirable for users - which as we already researched goes beyond the simplistic approach taken here. I appreciate your sturdiness, but its been my impression for the past few months that Spark already threw in the towel and with that the largest resource available to making this happen.

sdboyer’s picture

yep - a conversation to be had with the maintainers, indeed.

the reality is that we're going to be tweaking and improving the API for some time to come. there's no denying that. what's different now is that, once we can get controllers in, anyway, the pieces of the API that this UI would be reliant on are taken care of. there's a much simpler conversation that could be had about this than the one we've had in the past.

effulgentsia’s picture

I think it's appropriate to keep this as an 8.x task issue for now, but my hunch is that the scope needs to be significantly reduced to something like:

- Keep layout.module (or the concept of layouts as plugins, even if that concepts moves to a non-module location).
- Once #1812720: Implement the new panels-ish controller [it's a good purple] is complete, ensure that the entire page.tpl.php rendering pipeline is using layout plugins + display configuration entities instead (this is an implementation task, not a UI affecting one).
- In core, provide a preset 1:1 mapping between theme and layout (i.e., each core theme provides 1 layout plugin and Display entity) (this is an implementation task, not a UI affecting one).
- Ensure there are no critical UX regressions of admin/structure/block relative to D7: #1875252: [META] Make the block plugin UI shippable.

What this issue originally set out to do, however, in terms of a UX for adding displays, or configuring them with a radically better UI than D7's admin/structure/block, seems out of reach for core. Though with the API support reflected in the above, D8 Panels can shed a lot of its former baggage.

Gábor Hojtsy’s picture

Yes, that sounds like abou the scope possible to achieve still. Definitely nowhere close to what the issue summary currently depicts.

EclipseGc’s picture

sdboyer and I have discussed the fact that we need to get a summary of what we think the way forward looks like, but given the limited feedback we've gotten on this issue already I feel pretty positive about where everyone is at in relation to where Blocks & Layouts looks like it's going right now. We'll get a blog post up detailing what we think is left and how that all jives with a post-18th world in the next few days. Thanks for this feedback thus far, it's greatly appreciated.

Eclipse

podarok’s picture

Status: Needs work » Needs review

any update here?

xjm’s picture

andypost’s picture

There's a much simpler implementation proposed in #1875252-47: [META] Make the block plugin UI shippable

xjm’s picture

temporarily tagging. :)

webchick’s picture

Version: 8.x-dev » 9.x-dev

This seems to be D9 at this point.

webchick’s picture

Issue summary: View changes

Updated issue summary.

catch’s picture

Version: 9.x-dev » 8.1.x-dev
Issue summary: View changes

Not impossible to add in a minor version.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andypost’s picture

Related issues: +#2296423: Implement layout plugin type in core

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tkoleary’s picture

Status: Needs review » Needs work

Patch needs re-roll

andypost’s picture

Issue tags: +Needs reroll

call for contrib

sidharthap’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
179 KB

Reroll patch as per #94

Status: Needs review » Needs work

The last submitted patch, 95: 1841584-master_displays-95.patch, failed testing.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.