Currently, Layouts can provide their own icon image which can be used in user interfaces. These icons could be in any size, color, dimensions, or style. As a result, user interfaces suffer as consistency is lost between different Layouts. The best example of this are the bright-pink icons provided by Radix, which look nothing like the default Panels or Display Suite Layouts (this isn't Radix's fault, we can't expect everyone to have the same design opinions).

The provided patch is a proof of concept for dynamically generated icons, which can not only help with consistency but could also be used in user interfaces to directly interact with regions. This also reduces developer strain as filling out config is far easier than creating an icon (which is a skill not everyone has).

Instead of making something fully featured, I wanted to put this forward and see if anyone sees value in what I've already done. Simply apply the provided patch, enable the layout_plugin_example module, and visit the path /admin/layout_plugin_example/layout_map/layout_example_stacked.

What you see on that page is the result of a new configuration value, "icon_map", which is a two-dimensional array mapping region names to their visual placement on a page. Widths of regions are determined dynamically based on the number of columns per row in the array. You can also hover over any region to see a tooltip containing the machine name of the region. See layout_plugin_example.layouts.yml for the exact structure used.

Thoughts?

CommentFileSizeAuthor
#116 2660124-layout-116.patch23.94 KBtim.plunkett
#116 2660124-layout-116-interdiff.txt4.45 KBtim.plunkett
#106 icons_in_tray.png20.35 KBxjm
#105 layout-icons-2.gif1.33 MByoroy
#103 2660124-layout-103.patch23.96 KBtim.plunkett
#103 2660124-layout-103-interdiff.txt5.23 KBtim.plunkett
#102 layout-icons.gif2.43 MByoroy
#98 2660124-layout-98.patch23.31 KBtim.plunkett
#98 2660124-layout-98-interdiff.txt743 bytestim.plunkett
#96 2660124-layout-96.patch23.31 KBtim.plunkett
#96 2660124-layout-96-interdiff.txt11.86 KBtim.plunkett
#94 2660124-layout-93.patch24.46 KBtim.plunkett
#93 2660124-layout-93-interdiff.txt7.08 KBtim.plunkett
#85 2660124-layout-85.patch23.38 KBtim.plunkett
#85 2660124-layout-85-interdiff.txt1.82 KBtim.plunkett
#83 2660124-layout-83.patch22.78 KBtim.plunkett
#83 2660124-layout-83-interdiff.txt12.48 KBtim.plunkett
#81 2660124-layout-81.patch20.79 KBtim.plunkett
#81 2660124-layout-81-interdiff.txt5.2 KBtim.plunkett
#79 2660124-layout-79-interdiff.txt13.25 KBtim.plunkett
#79 2660124-layout-79.patch19.93 KBtim.plunkett
#74 interdiff-2660124-70-74.txt1.33 KBphenaproxima
#74 2660124-74.patch19.97 KBphenaproxima
#70 2660124-layout-70.patch19.81 KBtim.plunkett
#70 2660124-layout-70-interdiff.txt6.66 KBtim.plunkett
#69 2660124-layout-69.patch19.83 KBtim.plunkett
#69 2660124-layout-69-interdiff.txt8.4 KBtim.plunkett
#56 2660124-layout-56.patch19.26 KBtim.plunkett
#56 2660124-layout-56-interdiff.txt8.49 KBtim.plunkett
#52 2660124-layout-52.patch18.91 KBtim.plunkett
#52 2660124-layout-52-interdiff.txt7.75 KBtim.plunkett
#49 2660124-layout-49.patch18.2 KBtim.plunkett
#49 2660124-layout-49-interdiff.txt1.72 KBtim.plunkett
#45 2660124-layout-45-interdiff.txt4.02 KBtim.plunkett
#45 2660124-layout-45.patch18.09 KBtim.plunkett
#43 2660124-layout-43-interdiff.txt14.03 KBtim.plunkett
#43 2660124-layout-43.patch18.2 KBtim.plunkett
#41 layout-icon-6.png6.55 KByoroy
#41 layout-icon-5.png10.7 KByoroy
#41 layout-icon-4.png161.84 KByoroy
#41 layout-icon-3.png160.7 KByoroy
#38 layout-icon-2.png16.87 KByoroy
#38 layout-icon.png18.99 KByoroy
#37 2660124-layout-37-interdiff.txt2.35 KBtim.plunkett
#37 2660124-layout-37.patch14.43 KBtim.plunkett
#35 2660124-layout-35.patch12.9 KBtim.plunkett
#35 2660124-layout-35-interdiff.txt2.22 KBtim.plunkett
#33 2660124-layout-33-interdiff.txt9.54 KBtim.plunkett
#33 2660124-layout-33.patch12.91 KBtim.plunkett
#27 Screen Shot 2017-06-29 at 10.45.27 PM.png513.04 KBWidgetsBurritos
#27 Screen Shot 2017-06-29 at 10.42.19 PM.png423.17 KBWidgetsBurritos
#26 2660124-layout_icons-26.patch12.63 KBsamuel.mortenson
#21 2660124-layout_icons-21.patch19.58 KBtim.plunkett
#20 panopoly-layouts.png28.97 KBdead_arm
#20 panels3-choose-layout.png2.21 KBdead_arm
#15 2660124-layout_icons-15.patch83.72 KBtim.plunkett
#15 2660124-layout_icons-15-interdiff.txt2.96 KBtim.plunkett
#15 2660124-layout_icons-15-do-not-test.patch19.7 KBtim.plunkett
#14 svg-render--do-not-test.patch5.47 KBtstoeckler
#12 2660124-layout_icons-12.patch73.67 KBtim.plunkett
#12 2660124-layout_icons-12-interdiff.txt520 bytestim.plunkett
#10 2660124-layout_icons-9.patch8.31 KBtim.plunkett
#3 Screen Shot 2016-02-01 at 7.42.11 AM.png71.79 KBsamuel.mortenson
#2 interdiff-2660124-1-2.txt3.31 KBsamuel.mortenson
#2 layout-plugin-dynamic-icons-2660124-2.patch9.27 KBsamuel.mortenson
layout-plugin-dynamic-icons.patch8.9 KBsamuel.mortenson
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

samuel.mortenson created an issue. See original summary.

samuel.mortenson’s picture

Minor code cleanup that I missed last night.

samuel.mortenson’s picture

Icon Map Screenshot

Added a tl;dr screenshot of this working on a temporary stacked layout.

dsnopek’s picture

For the record, this is really cool! And making those layout icons has always been super annoying. :-) I'd love to get this in eventually.

As discussed on IRC, the thing that worries me is this slowing down our attempt to get layout_plugin into core. Maybe we can do this in a contrib module for the time being and merge upstream later (whether upstream means layout_plugin in contrib or core)? Or if we completely miss the core 8.1.0 window, then we'll have 6 months before we can try again for 8.2.0, and then I'd be less wary of merging this in..

Gábor Hojtsy’s picture

This looks super cool and indeed the 8.1 release went by... Not 100% sure this needs to be in the initial solution but this is an important usability problem we struggled in Spark years ago when doing dynamic layout editor. Its great you can dynamically edit your layout but then there was no dynamic icon. So super-cool once again :)

webchick’s picture

Just wanted to chime in and say this is fricking awesome.

phenaproxima’s picture

+1 for this feature. It is radical, tubular, and righteous. Love it.

johnkennedy’s picture

+1

tim.plunkett’s picture

Project: Layout Plugin (obsolete, use core's Layout Discovery) » Drupal core
Version: 8.x-1.x-dev » 8.3.x-dev
Component: Code » layout.module
Issue tags: +Blocks-Layouts

Stealing this now that Layout is in core.
The global function and render() calls were bugging me, so I made it a service? Idk.
Also wrote some tests.
Since we don't have examples, didn't port that part.
Also have yet to add it to the layout plugin at all.

tim.plunkett’s picture

Status: Active » Needs review
FileSize
8.31 KB

Status: Needs review » Needs work

The last submitted patch, 10: 2660124-layout_icons-9.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
520 bytes
73.67 KB

...

samuel.mortenson’s picture

Awesome! It's worth noting that the patch from #12 includes the field_layout patch from #2796173: Add experimental Field Layout module to allow entity view/form modes to switch between layouts. I'll take some time to review my logic for rendering the icon since I haven't worked on this in a year and want to make sure there's nothing (major) missing.

Off the bat I would recommend that core only uses dynamically generated icons if this goes in, to set an example for contrib developers. In addition to all the benefits of being configurable in YML and styled via the API, these icons also have hover tooltips on regions so you actually know what goes where.

tstoeckler’s picture

Played around with this a bit. Because of #2544318: Remove #type => html_tag usages I don't think the current render mechanism is going to fly at least in the long run. I think we should add proper theme functions/render elements for generating SVGs. Cooked up an initial patch at #2694535-5: Support rect property and nested render arrays in html_tag for dynamic SVGs. Attached patch shows how that could work. It also adds all layout icons to the "Manage display" of entities with the field_layout module on for testing purposes. (It is meant to be applied on top of #12)

tim.plunkett’s picture

Rolling this all together a bit more. The do-not-test branch shows without the initial field_layout patch, but contains #2694535: Support rect property and nested render arrays in html_tag for dynamic SVGs

Status: Needs review » Needs work

The last submitted patch, 15: 2660124-layout_icons-15.patch, failed testing.

tim.plunkett’s picture

samuel.mortenson’s picture

+++ b/core/lib/Drupal/Core/Layout/LayoutDefinition.php
@@ -428,6 +428,30 @@ public function setIconPath($icon) {
+    if ($icon_path = $this->getIconPath()) {
+      $icon = [
+        '#theme' => 'image',
+        '#uri' => $icon_path,
+      ];
+    }
+    elseif ($icon_map = $this->get('icon_map')) {

Should we switch this logic to prefer the dynamically generated icon?

In my opinion the icon path is a fallback in case there isn't a map, although you may have done this to prevent confusion with existing layout_plugin users.

Otherwise this patch is looking good! Thank you @timplunkett for all the work so far. I also added some review to #2694535: Support rect property and nested render arrays in html_tag for dynamic SVGs to move that issue along, as this depends on it.

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.

dead_arm’s picture

At DrupalCon Baltimore @pixelite and I were discussing that the layout preview icons that Panopoly provides are more legible (black rectangles without borders) than the layout preview icons that Panels provides. Our suggestion is that the generated svg layout icons should be solid color rectangles instead of rectangles with borders.

Standard Panels layout preview:
Panels layout icons

Panopoly layout preview:
Panopoly layout icons

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
19.58 KB

This is a straight reroll of the last patch. It does not take into account changes made in the other issue, or the review in #20!

Status: Needs review » Needs work

The last submitted patch, 21: 2660124-layout_icons-21.patch, failed testing.

Manuel Garcia’s picture

Came across this while working on #2786541: Add page listing the available layouts on the site, and this is awesome.

Just a small request:

+++ b/core/lib/Drupal/Core/Layout/LayoutDefinition.php
@@ -377,6 +377,30 @@ public function setIconPath($icon) {
+  public function getIcon() {
+    $icon = [];
+    if ($icon_path = $this->getIconPath()) {
+      $icon = [
+        '#theme' => 'image',
+        '#uri' => $icon_path,
+      ];
+    }
+    elseif ($icon_map = $this->get('icon_map')) {
+      $icon = $this->getIconGenerator()->generateSvgFromIconMap($icon_map);
+    }
+    return $icon;
+  }

It'd be great if we can get the alt attribute on the icon as well:
'#alt' => $this->getLabel()

Manuel Garcia’s picture

Re: #20 that looks better, but as it is it would not work if the theme had a dark background... unless we set the background of the layout selection to white, or add a white background to each icon itself.

Manuel Garcia’s picture

Just a heads up that #2694535: Support rect property and nested render arrays in html_tag for dynamic SVGs is RTBC, with a new approach which allows nesting html_tag elements, see CR https://www.drupal.org/node/2887146

samuel.mortenson’s picture

Status: Needs work » Needs review
FileSize
12.63 KB

This should fix tests, I updated \Drupal\Core\Layout\IconGenerator to support the recent html_tag changes and things are looking good! #20/#24 are not addressed.

Two things we need to think about:

  1. How are these icons going to be used? Is there a UI in contrib where we can see how they look with this patch, or are we waiting to build a UI that shows layout icons in core?
  2. Can we make the default layout icon style configurable? It's nice to have the icon generation abstracted as it is, but how can users change the style to match their preferences? Would this be done by code providing a Layout-picker-UI?
WidgetsBurritos’s picture

@samuel.mortenson,

Great work on this. While testing #26 locally using the field_layout module, I noticed a strange issue. Every time I switch back and forth between different layouts, it seems to nest the layout preview in another div.

So initially you have something like this:
Initial layout screenshot

But then after switching several times you get a bunch of nested divs like this:

Keeps adding <div> tags

I'm not entirely sure if that's related to this code specifically or a sign of a separate underlying issue with the field_layout module, but thought I'd point it out here. Since I'm not entirely sure the culprit, I won't change the status of this issue just yet, but wanted to point that out.

tim.plunkett’s picture

WidgetsBurritos’s picture

@tim.plunkett ahh okay great. Disregard my comment then.

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.

tim.plunkett’s picture

  1. +++ b/core/lib/Drupal/Core/Layout/IconGenerator.php
    @@ -0,0 +1,112 @@
    + * @internal
    + *   The layout system is currently experimental and should only be leveraged by
    + *   experimental modules and development releases of contributed modules.
    + *   See https://www.drupal.org/core/experimental for more information.
    

    The Layout system is no longer experimental! Yay.

  2. +++ b/core/lib/Drupal/Core/Layout/IconGenerator.php
    @@ -0,0 +1,112 @@
    +class IconGenerator {
    

    Since this is a service, it should have an interface.

  3. +++ b/core/modules/layout_discovery/layout_discovery.services.yml
    --- /dev/null
    +++ b/core/modules/layout_discovery/layouts/twocol/twocol.png
    

    There should be a layout in layout_test that has a PNG. We shouldn't add one to just one core layout.

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Layout/LayoutDefinition.php
@@ -377,6 +377,30 @@ public function setIconPath($icon) {
+  public function getIcon() {
...
+        '#theme' => 'image',
+        '#uri' => $icon_path,
...
+      $icon = $this->getIconGenerator()->generateSvgFromIconMap($icon_map);

For better usage in different situations, I think getIcon() should have a $width and $height parameter, which can be passed to the #theme=>image array and the generate call

tim.plunkett’s picture

I made the adjustments of #31 and #32, and also finished defining the config for each layout, and added some more docs.

Status: Needs review » Needs work

The last submitted patch, 33: 2660124-layout-33.patch, failed testing. View results

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
2.22 KB
12.9 KB

Accidentally changed the default values when I was testing it out. Set them back.

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Layout/LayoutDefinition.php
@@ -372,6 +372,52 @@ public function setIconPath($icon) {
+    elseif ($icon_map = $this->get('icon_map')) {

getIconMap should be made a real method, and $icon_map should be added as \Drupal\Core\Layout\Annotation\Layout::$icon_map

tim.plunkett’s picture

Finally got back to this.

yoroy’s picture

Status: Needs review » Needs work
FileSize
18.99 KB
16.87 KB

Super cool to see this back alive!

  1. It's quite big. How was this size arrived at?
  2. Can the colors be overridden by the theme?
  3. I think we can come up with a nicer looking default. Maybe we don't need a different stroke color at all. The gutter itself could be enough to outline the different segments. Looking at it in Seven theme, might want to reuse the #ccc color that's used for the details wrapper
  4. The outermost top and left line widths are 1px instead of the defined 2 px. This happens when x or y have a "0" starting position. Tweaking it to x=1 fixed it for me. Likely because the line width is applied centered on it's box outline (not inside or outside it).

Keeping it at the same size and changing the colors only already makes it visually less intrusive:

Manuel Garcia’s picture

RE: #38.3 - While I agree that removing the stroke makes it much easier on the eyes, if the theme has the same background color of gray, you would not be able see the gutters...

SKAUGHT’s picture

#39
simply enough: add a 10px white border (padding) around the entire image.

yoroy’s picture

Good point @Manuel Garcia. Discussed this with @timplunkett in IRC:

- Size is based on the icons being used in settings tray, like so: https://www.drupal.org/files/issues/layout-builder-add-section.png
- Colors can't be overridden

I experimented a bit more. Screenshots include the widhts, heights, colors I changed through the inspector. You can see that with these values the whole of the 250*300 canvas is used for the drawing, the blue area indicates the whole of the svg box:

using 2px strokes:

using 1px strokes (This still needs a x=1 and y=1 starting position or else we see a blurry attempt at 0,5px rendering)

A 1px stroke looks better. The 2px stroke interferes with the 2px(?) gutter. Having the stroke be less wide than the gutter is easier on the eye.
I used #333 for the stroke color (same as for body text) and #ccc for the fill.

A more Seven compatible palette: fill #f5f5f2, stroke #666 (because double #333 and well, evil :)

The same drawing on the dark settings tray background:

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

Working on this.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
18.2 KB
14.03 KB

Redid the generation logic to account for the stroke and padding. SVGs draw their stroke centered over the line. For a 4px stroke, you'd need to position the top left at 2,2 in order for it to be visible.

This uses the colors specified by @yoroy.

Additionally, I removed all of the default values for color and widths from the service.
Since they cannot be overridden, I believe they should always be specified by the calling code that is putting it into the UI.

This also prevents us from putting Seven theming into \Drupal\Core subsystems.

tedbow’s picture

Status: Needs review » Needs work

UPDATE: forgot to say how awesome this feature is!!! 🙌

  1. +++ b/core/lib/Drupal/Core/Layout/IconGeneratorInterface.php
    @@ -0,0 +1,73 @@
    +   *   A two dimensional array representing the visual output of the layout.
    +   *   For the following shape:
    +   *   |------------------------------|
    +   *   |                              |
    +   *   |             100%             |
    +   *   |                              |
    +   *   |-------|--------------|-------|
    +   *   |       |              |       |
    +   *   |  25%  |      50%     |  25%  |
    +   *   |       |              |       |
    +   *   |-------|--------------|-------|
    +   *   |                              |
    +   *   |             100%             |
    +   *   |                              |
    +   *   |------------------------------|
    +   *   The corresponding array would be:
    +   *   - [top]
    +   *   - [first, second, second, third]
    +   *   - [bottom].
    

    Is the only place someone could look in the code to determine what should go an icon_map? Or or at least to visually what an icon would look like next to the map values?

    If so it seems like it would be good idea to show another example with region spanning multiple rows.

  2. +++ b/core/lib/Drupal/Core/Layout/IconGeneratorInterface.php
    @@ -0,0 +1,73 @@
    +  public function generateSvg(array $regions, $width, $height, $stroke_width, $fill, $stroke);
    

    Why is a public method on the interface? From what I can tell it is only called from \Drupal\Core\Layout\IconGenerator::generateSvgFromIconMap Do well really want to expose this to our API if we don't have to?

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
18.09 KB
4.02 KB

Great feedback on both points!
Fixed both.

neclimdul’s picture

Looks really awesome. Truly the dream for layout icons.

I had some concerns about the find-ability of of the icon_map format documentation. I think its probably fine with the @see but it does bury it a bit. I'm told its because we want to provide logic that can be used outside the core layout plugin and that makes sense.

Additionally render arrays... gross. I guess this sort of kills #2544318: Remove #type => html_tag usages(noted in #14) but without a SVG library or something much more complicated that seems like the only solution so carry on.

tedbow’s picture

Status: Needs review » Needs work
Issue tags: +Needs change record, +needs documentation updates
  1. Doesn't this need a change record since it is new key that should be added to layout.yml?
  2. Needs doc update: https://www.drupal.org/docs/8/api/layout-api/how-to-register-layouts
  3. +++ b/core/lib/Drupal/Core/Layout/Annotation/Layout.php
    @@ -112,6 +112,13 @@ class Layout extends Plugin {
    +   * The icon map.
    +   *
    +   * @var array optional
    +   */
    +  public $icon_map;
    

    Could probably use the same @see to \Drupal\Core\Layout\IconGeneratorInterface::generateSvgFromIconMap
    So it would show up here https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Layout%21...

Otherwise very close.

dawehner’s picture

I just came by to say how amazing this feature is!

Here are just nitpicks

  1. +++ b/core/lib/Drupal/Core/Layout/IconGenerator.php
    @@ -0,0 +1,178 @@
    +    if (!$rows) {
    +      return $region_rects;
    +    }
    

    Given the rest of the function this if could be avoided. The rest doesn't do anything

  2. +++ b/core/lib/Drupal/Core/Layout/IconGeneratorInterface.php
    @@ -0,0 +1,51 @@
    +interface IconGeneratorInterface {
    
    +++ b/core/lib/Drupal/Core/Layout/LayoutDefinition.php
    @@ -372,6 +381,77 @@ public function setIconPath($icon) {
    +  /**
    +   * @return \Drupal\Core\Layout\IconGenerator
    +   */
    

    Nitpick: Let's typehint against the interface

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Status: Needs work » Needs review
FileSize
1.72 KB
18.2 KB

Fixing all the code parts.
Docs and CR still needed.

Status: Needs review » Needs work

The last submitted patch, 49: 2660124-layout-49.patch, failed testing. View results

Manuel Garcia’s picture

Any chance we can get the alt attribute as mentioned on #23? It would be great for accessibility.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
7.75 KB
18.91 KB

SVG elements don't support alt, but a top-level <title> tag
Added support for that, as well as adding #alt for the image.

Also the check I removed in #49 *was* needed, but fixed it a different way.

Manuel Garcia’s picture

Brilliant, thanks @tim.plunkett!

samuel.mortenson’s picture

Issue tags: -Needs change record

Created change notice: Layout icons can be dynamically generated

We can add more information, but I wanted to keep things simple as the documentation will likely go into more depth.

tstoeckler’s picture

  1. +++ b/core/lib/Drupal/Core/Layout/IconGenerator.php
    @@ -0,0 +1,186 @@
    +  protected function generateSvg(array $regions, $label, $width, $height, $stroke_width, $fill, $stroke) {
    ...
    +          'x' => $attributes['x'],
    +          'y' => $attributes['y'],
    +          'width' => $attributes['width'],
    +          'height' => $attributes['height'],
    +          'fill' => $fill,
    +          'stroke' => $stroke,
    +          'stroke-width' => $stroke_width,
    

    So I stumbled a bit on the number of arguments here, which makes usage of this function a bit confusing IMO, and then I noticed that with $regions we already prepare a special data structure to assemble the rectangles from. So I thought whether it would make sense to add the fill, stroke and stroke width to the $regions structure as well. That would have the upside of requiring fewer arguments to this function and also allowing a bit more flexibility if someone needs it. The downside is that it would make $regions a fair bit more magical. So I guess it's arguable whether it's good or bad, but I thought I'd put it out there.

  2. +++ b/core/lib/Drupal/Core/Layout/IconGenerator.php
    @@ -0,0 +1,186 @@
    +      // Group our regions allows for metadata, nested elements, and tooltips.
    

    This sentence is not quite proper English.

  3. +++ b/core/lib/Drupal/Core/Layout/IconGenerator.php
    @@ -0,0 +1,186 @@
    +      $vertical_offset = $this->getOffset($row, $row_height, $stroke_width, $padding);
    ...
    +            'x' => $this->getOffset($col, $column_width, $stroke_width, $padding),
    +            'y' => $vertical_offset,
    ...
    +          $region_rects[$region]['width'] = $this->getOffset($col, $column_width, $stroke_width, $padding) - $region_rects[$region]['x'] + $region_rects[$region]['original_width'];
    +          $region_rects[$region]['height'] = $this->getOffset($row, $row_height, $stroke_width, $padding) - $region_rects[$region]['y'] + $region_rects[$region]['original_height'];
    

    Am I missing something or are the getOffset() calls not identical between the if and the else branch? Would be nice to consolidate. Especially the 'x' and the 'y' part in the if-branch look inconsistent even though it seems they should not need to.

  4. +++ b/core/lib/Drupal/Core/Layout/IconGenerator.php
    @@ -0,0 +1,186 @@
    +            // Store the original values.
    +            'original_width' => $column_width,
    +            'original_height' => $row_height,
    

    So it seems these are only used for the internal calculation but they are never cleaned up afterwards. They don't really hurt, but since nowadays people are just as likely to just look at the resulting object in a debugger rather than (or in addition to) reading its docs, I think it would be nice to make it a tad less confusing to those people, given that that this is just a magic array.

tim.plunkett’s picture

#55 Thanks for the review!

1) I just looked into making $regions more magic, and also into making an $options array containing the more optional parameters.
Except that both made the code much harder to understand.
But it made me look into what happens when you pass NULL for any of those.
Thankfully it prevents the attribute from printing at all (which is good), and that SVGs natively have default values for these.

So as a compromise for the long list of parameters, we can make the truly optional ones optional. I think this is an improvement.

2) Reworded

3) Well spotted! I guess I didn't clean up after refactoring this.

4) I'm eyerolling super hard at the idea of managing our internal data differently in defense of people violating the API. But it's an easy change to make, and a fun usage of array_walk :)

zaporylie’s picture

+++ b/core/lib/Drupal/Core/Layout/Annotation/Layout.php
@@ -112,6 +112,15 @@ class Layout extends Plugin {
+   * @var array optional

Nitpic: @var takes data type specification only. "optional" should be moved to the description (2 lines above) or removed. See https://www.drupal.org/docs/develop/coding-standards/api-documentation-a... and #2909370: Fix 'Drupal.Commenting.VariableComment.IncorrectVarType' coding standard

tstoeckler’s picture

Re #56: Thanks, the improvements look great. Regarding 1: Thanks for thinking about it, I absolutely trust your judgement that it would make the code less readable. And thanks for 4., I will applaud your patience with me every time I see one of those thingies in the Debugger ;-)

tim.plunkett’s picture

@zaporylie, this was added to our standard specifically for documenting annotation classes, fixing that standard (and all annotation classes) is out of scope here.

@tstoeckler ❤️

Manuel Garcia’s picture

Status: Needs review » Needs work

Brilliant, thanks all!

I had a look at the CR, it looks good to me. So I suppose the only thing left to do here would be updating the documentation.

DyanneNova’s picture

Status: Needs work » Needs review

Here's a draft for the documentation changes:

<h3 id="simplest-case">The simplest case</h3>

<p>Here's an example of a super simple *.layouts.yml:</p>

<pre>
one_column:
  label: 'One column'
  category: 'My Layouts'
  template: templates/one-column
  default_region: main
  icon_map:
    -  [main]
  regions:
    main:
      label: Main content
two_column:
  label: 'Two column'
  category: 'My Layouts'
  template: templates/two-column
  default_region: main
  icon_map:
    -  [main, sidebar] 
  regions:
    main:
      label: Main content
    sidebar:
      label: Sidebar
</pre>

<p>This registers two layouts: "One column" and "Two column". The 'label' and 'category' keys are required!</p>

[ ... ]

<h2 id="full-annotation-reference">Full annotation reference</h2>

<p>Each layout definition <strong>must</strong> have the following keys:</p>

<dl>
     <dt>label</dt>
     <dd>The human-readable name of the layout.</dd>
     <dt>category</dt>
     <dd>The human-readable category to which the layout belongs.</dd>
     <dt>regions</dt>
     <dd>Array of regions in the layout. The keys are the regions' machine names and the<br />
     values are sub-arrays containing the following elements:
     <ul>
          <li>label (required): The human-readable name of the region.</li>
     </ul>
     </dd>
     <dt>icon_map</dt>
     <dd>A map of regions for the icon generator to use in creating a layout icon.
     <figure class="image"><img alt="A more complicated example icon map." height="434" src="https://www.drupal.org/files/issues/Screen%20Shot%202016-02-01%20at%207.42.11%20AM.png" width="613" />
     <figcaption>A more complicated example icon map.</figcaption>
    </figure></dd>
     <dt>theme</dt>
     <dd>If specified, the theme hook which will be used to render this layout. It's<br />
     expected that the module or theme which registers the layout will also register this<br />
     theme hook. If you use this key, you cannot use template.</dd>
     <dt>template</dt>
     <dd>If specified, the template to use to render the layout, relative to the given<br />
     path, without the .html.twig extension. If given, the template will be<br />
     automagically registered with the theme system. If you use this key, you cannot use<br />
     theme.</dd>
</dl>

<p>Each layout definition can also have the following <strong>optional</strong> keys:</p>

( I removed current code tags to keep formatting readable here, but we should keep them in the updates.)

The last submitted patch, 2: layout-plugin-dynamic-icons-2660124-2.patch, failed testing. View results

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Looks great, thanks @DyanneNova!
Leaving the tag, since we still have to make the changes once it is committed.

amateescu’s picture

Was this RTBC'd before by someone who didn't work so much on the patch?

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs review

It was not! My mistake, I thought it had gone RTBC->NW in #60. Thanks @amateescu :)

Manuel Garcia’s picture

Looks to me like all code reviews have been addressed, we have a CR and Documentation edits ready for when this is committed. RTBC++
Only one thing I see that could be improved perhaps.

+++ b/core/lib/Drupal/Core/Layout/IconGenerator.php
@@ -0,0 +1,193 @@
+   *   See \Drupal\Core\Layout\IconGeneratorInterface::generateSvgFromIconMap().

@see would be better =)

Manuel Garcia’s picture

Status: Needs review » Reviewed & tested by the community

My last nitpick should be fixable on commit :)

xjm’s picture

Status: Reviewed & tested by the community » Needs work

This is interesting!

Miscellaneous questions and small things to fix, mostly docs clarifications:

  1. +++ b/core/lib/Drupal/Core/Layout/IconGenerator.php
    @@ -0,0 +1,193 @@
    +   * Generates an SVG.
    ...
    +  protected function generateSvg(array $regions, $label, $width, $height, $stroke_width = NULL, $fill = NULL, $stroke = NULL) {
    

    I was initially confused by the difference between generateSvgFromIconMap() and this, until I noticed that this is protected and not on the interface. Would this method be better names something like generateSvgRenderArray() or something? Because it doesn't actually generate an SVG per se.

  2. +++ b/core/lib/Drupal/Core/Layout/IconGenerator.php
    @@ -0,0 +1,193 @@
    +   * @param mixed[] $regions
    +   *   An array keyed by region name, with each element containing the 'height',
    +   *   'width', and 'x' and 'y' offsets of each region.
    ...
    +   * @return mixed[]
    +   *   An array keyed by region name, with each element containing the 'height',
    +   *   'width', and 'x' and 'y' offsets of each region.
    

    Discussed with @tim.plunkett; this is a mixed[][].

  3. +++ b/core/lib/Drupal/Core/Layout/IconGenerator.php
    @@ -0,0 +1,193 @@
    +   * @param array $rows
    +   *   A two dimensional array representing the visual output of the layout.
    +   *   See \Drupal\Core\Layout\IconGeneratorInterface::generateSvgFromIconMap().
    
    +++ b/core/lib/Drupal/Core/Layout/IconGeneratorInterface.php
    @@ -0,0 +1,53 @@
    +   * @param array $icon_map
    +   *   A two dimensional array representing the visual output of the layout.
    +   *   For the following shape:
    

    See which bit of `generateSvgFromIconMap()`? I'd have assumed its return value but it looks like maybe it actually means the `$icon_map` parameter? Let's say that explicitly?

    Also I guess they are `string[][]`?

    Nit (here and elsewhere): "two-dimensional".

    The suggestion above can be addressed by additionally appending the @see at the end of the docblock I guess; I do think we should refer to it here in the parameter docs for clarity (where we can't have an @see).

  4. +++ b/core/lib/Drupal/Core/Layout/IconGenerator.php
    @@ -0,0 +1,193 @@
    +   * @param int $padding
    +   *   The padding between regions. Any value above 0 is valid.
    

    What if I wanted 0 padding, like a stained glass window? (Or border-collapse: collapse; for tables.) I would think 0 is a fine value then?

    By the same note, could it be a fraction value? (Applies to the others as well I guess.) I think it's fine to choose to constrain it to integer pixel counts by choice; just wanted to raise the question.

  5. +++ b/core/lib/Drupal/Core/Layout/IconGenerator.php
    @@ -0,0 +1,193 @@
    +        // A repeated region should expand the dimensions of the original.
    

    Er wha?

  6. +++ b/core/lib/Drupal/Core/Layout/IconGenerator.php
    @@ -0,0 +1,193 @@
    +    $total_stroke = $number_of_regions * $stroke_width;
    

    Wouldn't this be twice the stroke width (a border on either side)?

    ...Never mind, I see above that only half the stroke is outside the region. Having inline comments here could be helpful in case we have to update the math for some reason in the future.

  7. +++ b/core/lib/Drupal/Core/Layout/IconGeneratorInterface.php
    @@ -0,0 +1,53 @@
    +   *   - [top]
    +   *   - [first, second, second, third]
    +   *   - [first, bottom, bottom, bottom].
    

    Are the region names strings here? If so could we single-quote them for clarity in the example, and declare the parameter as string[][]?

  8. +++ b/core/lib/Drupal/Core/Layout/LayoutDefinition.php
    @@ -372,6 +381,81 @@ public function setIconPath($icon) {
    +   * @param string $fill
    +   *   (optional) If a generated SVG is used, the fill color of regions.
    +   *   Defaults to 'lightgray'.
    +   * @param string $stroke
    +   *   (optional) If a generated SVG is used, the color of region borders.
    +   *   Defaults to 'black'.
    

    Above in #20, someone proposes making the layouts solid rectangles for easier visibility. Per @tim.plunkett, the colors in the current patch have been checked for contrast ratios and were recommended by @yoroy (as a Seven-compatible palette?)

    Where do I specify the background color? Or would I just do that in CSS?

    Do we need a followup issue to add CSS to themes for this (postponed on it being stable), or do we want it to always be specified in the API?

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
8.4 KB
19.83 KB

#68

1) Agreed, the naming here is off. I went to rename the methods away from "generate", but stopped since it is also the class, interface, and service ID. Let's discuss that naming before changing it all.

2) Fixed! It is technically (int|float)[][] but that is not valid doxygen :)

3) Fixed

4) There's no reason to say this, even negative values are valid (and useful, in the stained glass example).
Fixed the docs and added test coverage.
Technically there is nothing preventing a fractional value, and it would render just fine.

5) While attempting to improve these docs, realized we shouldn't ever need to store the original values. Hopefully this is more understandable

6) Added docs

7) This was copied from the YAML, where quotes are not included for machine names. Adding them here though.

8) There is currently no provision for background color. Yes you could do it in CSS, or we could add an additional parameter.
Themes can override the colors provided with CSS, but I agree we should ship with reasonable defaults.

@TODO Decide on naming (generate, build, etc)

tim.plunkett’s picture

Discussed the naming in the #layouts Slack channel with @EclipseGc and @samuel.mortenson.
Decided that just removing "Svg" from the method names made it clearer.
Yes, it generates a render array, which is documented. And now the method names don't imply something different.

That covers the last of the feedback in #68

larowlan’s picture

Reviewed this, my only question is about the precision of the fractional numbers in the test-case.

Code looks good, is nicely isolated and well tested.

+++ b/core/tests/Drupal/KernelTests/Core/Layout/IconGeneratorTest.php
@@ -0,0 +1,131 @@
+<rect x="1" y="1" width="78.666666666667" height="176.4" fill="lightgray" stroke="black" stroke-width="2" />
...
+<rect x="85.666666666667" y="1" width="163.33333333333" height="54.8" fill="lightgray" stroke="black" stroke-width="2" />
...
+<rect x="85.666666666667" y="61.8" width="78.666666666667" height="54.8" fill="lightgray" stroke="black" stroke-width="2" />
...
+<rect x="170.33333333333" y="61.8" width="78.666666666667" height="54.8" fill="lightgray" stroke="black" stroke-width="2" />
...
+<rect x="85.666666666667" y="122.6" width="163.33333333333" height="54.8" fill="lightgray" stroke="black" stroke-width="2" />

Is there a risk that these might result in a fragile test? i.e floats are always painful w.r.t precision

EclipseGc’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me!

Eclipse

markhalliwell’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Layout/LayoutDefinition.php
@@ -371,6 +380,81 @@ public function setIconPath($icon) {
+   *   (optional) The width of the icon. Defaults to 250.
...
+   *   (optional) The height of the icon. Defaults to 300.
...
+   *   Defaults to 2.
...
+   *   value above 0 is valid. Defaults to 5.
...
+   *   Defaults to 'lightgray'.
...
+   *   Defaults to 'black'.
...
+  public function getIcon($width = 125, $height = 150, $stroke_width = 1, $padding = 4, $fill = '#f5f5f2', $stroke = '#666') {

Minor, but important documentation nit: each "Defaults to" does not match the actual default values.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
19.97 KB
1.33 KB

Addressed #73.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Well spotted @markcarver, got out of sync during the feedback cycle.
Thanks for fixing @phenaproxima!

xjm’s picture

Status: Reviewed & tested by the community » Needs review

The naming changes still don't really address differentiating generateFromIconMap() from generate(). They don't return the same thing, but they are named as if they do. Can we discuss the naming a little more?

Also not sure the bit about background color got addressed. From @tim.plunkett in #69:

There is currently no provision for background color. Yes you could do it in CSS, or we could add an additional parameter.
Themes can override the colors provided with CSS, but I agree we should ship with reasonable defaults.

Thanks!

tim.plunkett’s picture

But they do return the same thing? The only difference is the parameters

+  /**
+   * {@inheritdoc}
+   */
+  public function generateFromIconMap(array $icon_map, $label, $width, $height, $padding, $stroke_width = NULL, $fill = NULL, $stroke = NULL) {
+    $regions = $this->calculateSvgValues($icon_map, $width, $height, $stroke_width, $padding);
+    return $this->generate($regions, $label, $width, $height, $stroke_width, $fill, $stroke);
+  }

One literally returns the other after translating the icon_map into the x/y/height/width array

xjm’s picture

Status: Needs review » Needs work

Discussed with @tim.plunkett. My suggestion is that the method currently named generateFromIconMap() (which is the only public API) just be called generate(). And then the internal helper be called something like buildRenderArray(), since in this case you don't actually have a choice between two different generation methods. (Tim explained that both methods used to be public, so I can see how we ended up with the current pattern.)

We also discussed whether a parameter for the background color should be added, or whether the stroke and fill colors should be specified in the PHP API at all. @lauriii said he did not think that the colors should be specified via the API. So let's remove those colors as parameters and figure out how to best supply default styling for them in the theme layer.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
19.93 KB
13.25 KB

1395f35749 Rename methods.
e98e8348e2 Remove support for specifying colors.
8a4c1163b5 Add back colors via CSS.

Status: Needs review » Needs work

The last submitted patch, 79: 2660124-layout-79.patch, failed testing. View results

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
5.2 KB
20.79 KB

Forgot to update the test for the attribute additions.

DyanneNova’s picture

Status: Needs review » Needs work

I think it would be helpful to include a class on the svg for the layout label, so, for example, in addition to layout-icon we would also have layout-icon--two-column on the two column layout icon. That will make the region classes more useful and give layout themers an easy way to categorically color layouts for site builders.

Other than that, the CSS looks good to me!

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
12.48 KB
22.78 KB

Good idea! Fixed.


As I made this change, I realized why this class has been bugging me: The parameters to generate() are getting out of hand.
Every time we want to pass slightly more data to the generator, the entire method signature has to change.

So I rewrote the whole thing using a form of the Builder pattern.
Take this usage from LayoutDefinition:

    elseif ($icon_map = $this->getIconMap()) {
      $icon_generator = $this->getIconGenerator()
        ->setId($this->id())
        ->setLabel($this->getLabel())
        ->setWidth($width)
        ->setHeight($height)
        ->setPadding($padding)
        ->setStrokeWidth($stroke_width);
      $icon = $icon_generator->generate($icon_map);
    }

All that is strictly required is the icon map.
The rest are overrides or enhancements.

Status: Needs review » Needs work

The last submitted patch, 83: 2660124-layout-83.patch, failed testing. View results

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.82 KB
23.38 KB

Made a mistake with the reroll for #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it
Adapted the location of the CSS files to fit with the new structure.

EclipseGc’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Layout/IconGeneratorInterface.php
@@ -0,0 +1,99 @@
+   * @param int|null $stroke_width
+   *   The width of region borders.
+   *
+   * @return $this
+   */
+  public function setStrokeWidth($stroke_width);

$stroke_width isn't default null anymore. Is this an error?

Otherwise looking good

Eclipse

tim.plunkett’s picture

Status: Needs work » Needs review

I should have mentioned that, sorry. I did this purposefully. This should have been done when the stroke color moved to CSS.

EclipseGc’s picture

Status: Needs review » Reviewed & tested by the community

Ok then, let's commit this, it looks good!

Eclipse

xjm’s picture

  1. +++ b/core/lib/Drupal/Core/Layout/LayoutDefinition.php
    @@ -409,26 +409,20 @@ public function setIconMap($icon_map) {
    -   *   (optional) The width of the icon. Defaults to 250.
    +   *   (optional) The width of the icon. Defaults to 125.
        * @param int $height
    -   *   (optional) The height of the icon. Defaults to 300.
    +   *   (optional) The height of the icon. Defaults to 150.
        * @param int $stroke_width
        *   (optional) If a generated SVG is used, the width of region borders.
    -   *   Defaults to 2.
    +   *   Defaults to 1.
    

    Ha nice catch. Missed that.

  2. +++ b/core/modules/settings_tray/css/off-canvas.layout.css
    @@ -0,0 +1,11 @@
    +/**
    + * @file
    + * Visual styling for layouts in the off canvas tray.
    + *
    + * See seven/css/layout/layout.css
    + */
    +
    +.layout-icon__region {
    +  fill: #f5f5f2;
    +  stroke: #666;
    +}
    
    +++ b/core/modules/settings_tray/settings_tray.libraries.yml
    @@ -35,6 +35,7 @@ drupal.off_canvas:
    +      css/off-canvas.layout.css: {}
    
    +++ b/core/themes/seven/css/layout/layout.css
    @@ -4,3 +4,11 @@
    +
    +/**
    + * Add color to layout icons.
    + */
    +.layout-icon__region {
    +  fill: #f5f5f2;
    +  stroke: #666;
    +}
    

    Probably we should have a followup to add these to Classy once the module is stable?

xjm’s picture

Disregard #89; I apparently somehow opened an older patch. And of course this is being added as a stable library so it doesn't need a followup; it just goes in this patch. :)

xjm’s picture

Status: Reviewed & tested by the community » Needs work

I like the switch to using class members for the width, height, etc.; I had thought about proposing that when I reviewed it previously but eventually didn't mention it. But I like the change. I think we need to go a little further with it though.

+++ b/core/lib/Drupal/Core/Layout/IconGenerator.php
@@ -0,0 +1,290 @@
+  protected function buildRenderArray(array $regions, $width, $height, $stroke_width) {

+++ b/core/lib/Drupal/Core/Layout/LayoutDefinition.php
@@ -371,6 +380,82 @@ public function setIconPath($icon) {
+  public function getIcon($width = 125, $height = 150, $stroke_width = 1, $padding = 4) {

+++ b/core/modules/layout_discovery/layout_discovery.services.yml
@@ -2,3 +2,5 @@ services:
+  layout.icon_generator:
+    class: Drupal\Core\Layout\IconGenerator

Discussed with @tim.plunkett. I was wondering if buildRenderArray() should either be static (and take more things as parameters) or be purely "self-aware" (cr. @tim.plunkett) and just use the members rather than taking them as parameters. And then I started thinking about state and Tim remembered that the icon generator is still a service. So it should have an icon generator factory service (icon factory service?) that's separate from the icon things that are things. (Y'all with the CS degrees supply the correct terminology here.)

So NW for that; Tim has a refactor that he's probably already coding to address that. :P

xjm’s picture

Also, let's test this in an RTL language to see if it supports RTL currently.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
7.08 KB

Neither <svg> nor <rect> support direction, only the text content within them.
Any file-based icons would similarly not respect RTL.
Furthermore, our layouts themselves apparently do not respect RTL!
Opened #2919372: Layouts do not respect RTL as a follow-up.

This switches to a factory in order for the service to remain stateless.

tim.plunkett’s picture

That was just the interdiff. Here's the patch too :)

EclipseGc’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Layout/Icon/IconFactoryInterface.php
    @@ -0,0 +1,18 @@
    +interface IconFactoryInterface {
    

    IconGeneratorFactoryInterface?

  2. +++ b/core/lib/Drupal/Core/Layout/Icon/SvgIcon.php
    @@ -1,13 +1,13 @@
    +class SvgIcon implements IconGeneratorInterface {
    

    SvgIconGenerator?? seems weird to not include that word. If not, that's cool too.

  3. +++ b/core/lib/Drupal/Core/Layout/Icon/SvgIconFactory.php
    @@ -0,0 +1,17 @@
    +class SvgIconFactory implements IconFactoryInterface {
    

    SvgIconGeneratorFactory

  4. +++ b/core/lib/Drupal/Core/Layout/LayoutDefinition.php
    @@ -449,11 +449,11 @@ public function getIcon($width = 125, $height = 150, $stroke_width = 1, $padding
    +    return \Drupal::service('layout.icon_generator')->get();
    

    I'm sure I'll say this on the services.yml but the service id feels like it should be "layout.icon_generator_factory" now, and even as I read the interface above, I was thinking "getGenerator()" which would make this line pretty clear. I'm being pedantic here probably but if it resonates for you, then consider it.

  5. +++ b/core/modules/layout_discovery/layout_discovery.services.yml
    @@ -3,4 +3,4 @@ services:
       layout.icon_generator:
    -    class: Drupal\Core\Layout\IconGenerator
    +    class: Drupal\Core\Layout\Icon\SvgIconFactory
    

    Yeah, as I mentioned, feels like this should be "layout.icon_generator_factory"

Code looks pretty good otherwise, my complaints are all "hard problem" territory. I think I'd like to see "generator" in the interface and class names as appropriate. If you feel like I made a point worth considering on the service, then do that too, but I'm not married to it.

Eclipse

tim.plunkett’s picture

Title: Dynamically generate layout icons based on well formed config » Dynamically build layout icons based on well formed config
Status: Needs work » Needs review
FileSize
11.86 KB
23.31 KB

All the way back in #68, @xjm pointed out that we don't "generate" anything.
@EclipseGc also said in chat something along the lines of "build is shorthand for 'i want a render array'".

So this once again switches the names.

Additionally, @EclipseGc pointed out there was a feature in Symfony that ensured we wouldn't get the same instance of a service. This lets us skip the factory part.

EclipseGc’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Layout/Icon/SvgIconBuilder.php
@@ -142,11 +142,11 @@ protected function buildRenderArray(array $regions, $width, $height, $stroke_wid
+   *   \Drupal\Core\Layout\IconBuilderInterface::build().

\Drupal\Core\Layout\Icon\IconBuilderInterface

Otherwise this looks great.

Eclipse

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
743 bytes
23.31 KB

Well spotted!

EclipseGc’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Layout/Icon/SvgIconBuilder.php
    @@ -0,0 +1,290 @@
    +/**
    + * Builds SVG layout icons.
    + */
    

    Any reason to not be more specific here and say something like "Builds render arrays representing SVG layout icons" or something like that? We're not actually building icons from this thing.

  2. +++ b/core/lib/Drupal/Core/Layout/Icon/SvgIconBuilder.php
    @@ -0,0 +1,290 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function setWidth($width) {
    +    $this->width = $width;
    +    return $this;
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function setHeight($height) {
    +    $this->height = $height;
    +    return $this;
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function setPadding($padding) {
    +    $this->padding = $padding;
    +    return $this;
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function setStrokeWidth($stroke_width) {
    +    $this->strokeWidth = $stroke_width;
    +    return $this;
    +  }
    

    These are all suppose to be integers and the rest of the code completely depends on that. I think it's probably worth doing a check to make sure that we got what we were expecting since we don't have scalar type hints in PHP 5.5.9.

  3. +++ b/core/modules/layout_discovery/layout_discovery.services.yml
    @@ -2,3 +2,6 @@ services:
    +    shared: false
    

    Super happy this worked.

This looks really really close.

Eclipse

tim.plunkett’s picture

Status: Needs work » Needs review

I'm going to push back on both of these points.

1) \Drupal\Core\Layout\Icon\IconBuilderInterface::build() is the one that cares about render arrays. But there could eventually be non-render array public methods. And \Drupal\Core\Layout\Icon\SvgIconBuilder is just "whatever the interface says, but with SVGs", hence the shortened comment

2) We don't actually enforce int, it could also be a float in theory?
If assert() weren't currently so fraught with PHP 5/7 issues, I'd consider using that.
But there's nowhere else we do this sort of handholding.

EclipseGc’s picture

Status: Needs review » Reviewed & tested by the community

Ok, I'm convinced. This patch looks super solid and reads pretty nicely. I say we get this done.

RTBC pending tests

Eclipse

yoroy’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.43 MB

Works like a charm, really nice. But why the full-on super dark fills? It's not the best looking default and in #38/#39 we discussed that a border-less design will make the icons invisible on a background of the same color.

tim.plunkett’s picture

I made a mistake in my reroll in #85 which caused #102, fixed that.

Also per discussion with @xjm, tried to reduce the number of places we hardcode these numbers.
Couldn't remove them all, as we need a height/width for the file-based portion of LayoutDefinition::getIcon()

xjm’s picture

Thanks @tim.plunkett; that set of defaults seems fine to me.

yoroy’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.33 MB

Much better!

xjm’s picture

Issue summary: View changes
FileSize
20.35 KB

I manually tested it with Field Layout and it works great, as in the screenshot in #105.

I also used #2905922: Implementation issue for Layout Builder to test the icons in the Settings Tray. They resize nicely with the overridden sizes from that patch. The contrast ratio in the Settings Tray is a little intense:

But since the layout icons aren't exposed in the Settings Tray in this issue or in HEAD, I don't think that needs to block anything here. (It might even be okay anyway.)

xjm’s picture

I checked with @yoroy and he said the colors in #106 are fine, so no blocker there.

xjm’s picture

I like where this has ended up; probably will commit this later today.

  1. +++ b/core/lib/Drupal/Core/Layout/Icon/SvgIconBuilder.php
    @@ -0,0 +1,290 @@
    +  public function build(array $icon_map) {
    ...
    +  protected function buildRenderArray(array $regions, $width, $height, $stroke_width) {
    

    This combo of names makes a lot more sense, thanks! And it's nice that only one thing is required to create it.

  2. +++ b/core/lib/Drupal/Core/Layout/LayoutDefinition.php
    @@ -371,6 +380,85 @@ public function setIconPath($icon) {
    +  protected function getIconBuilder() {
    +    return \Drupal::service('layout.icon_builder');
    +  }
    

    I would have expected this to be injected, but @tim.plunkett explained that it's not injectable, and pointed me at the entity_type service which is used in the same way. It still seems a little weird to me, but not worth blocking this on.

xjm’s picture

Adding #2919372: Layouts do not respect RTL to the related issues, which I think blocks both Field Layout and Layout Builder from being stable. But since this and Layout Discovery only provide APIs that are not yet exposed in stable code, it's not blocking here.

larowlan’s picture

+++ b/core/tests/Drupal/KernelTests/Core/Layout/IconBuilderTest.php
@@ -0,0 +1,132 @@
+<rect x="85.666666666667" y="1" width="163.33333333333" height="54.8" stroke-width="2" class="layout-icon__region layout-icon__region--top" />
...
+<rect x="85.666666666667" y="61.8" width="78.666666666667" height="54.8" stroke-width="2" class="layout-icon__region layout-icon__region--left" />
...
+<rect x="170.33333333333" y="61.8" width="78.666666666667" height="54.8" stroke-width="2" class="layout-icon__region layout-icon__region--right" />
...
+<rect x="85.666666666667" y="122.6" width="163.33333333333" height="54.8" stroke-width="2" class="layout-icon__region layout-icon__region--middle" />

Did anyone answer my question in #71 about the level of precision here? Is it needed? Could it lead to fragility because of floating point arithmetic? Pity we can't use calc

tim.plunkett’s picture

I could pick more round numbers for the test maybe? But browsers support the subpixel arithmetic, and I'm not sure of a better way to flub the math to get integers always.

larowlan’s picture

According to https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/width width/height can be specified in % - would that help - or would we still end up with decimals?

tim.plunkett’s picture

We'd still end up with 33.333333% 33.333333% 33.333333% and the like

larowlan’s picture

Okay - thanks - was worth asking.

So the only other option is some maths to make it integers, but that might yield issues. We could explore that in a follow up, but its probably not worth the effort.

Thanks again

markhalliwell’s picture

The level of precision does seem unnecessarily verbose. I think, at most, 3 levels would be more than sufficient.

There is the possibility that these tests may fail in the future if this current level of precision ever changes depending on the software/architecture, however that seems highly unlikely... at least for a while anyway.

On a side note, I found this relic of an issue and think this issue provides a perfect use case for adding it to core, but I don't think it's a blocker for this issue.

For now, if anyone cares enough to do this, simply adding some @todos for the relevant methods/tests that reference said issue would likely be enough.

tim.plunkett’s picture

Switched the dimensions around so that we still test that floats are supported, but by using halves instead of thirds.

Leaving at RTBC.

xjm’s picture

I don't think #115 is really relevant because this isn't a field; it's an HTML attribute. I do think we should test float values to ensure they work, because that's important to make the icons render correctly and it's going to be the case more often than not.

#116 seems a good compromise to avoid any risk of testing implementation details of the testbot while still testing the fractional values. Thanks @tim.plunkett.

I think this is ready, but waiting for tests.

xjm’s picture

Saving issue credit.

  • xjm committed 1ebb2e1 on 8.5.x
    Issue #2660124 by tim.plunkett, samuel.mortenson, phenaproxima,...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

Alright, I read through the whole patch one more time, including executing the code and the test scenarios in my head with mental math. :P I also manually tested with both field_layout and the Layout Builder patch (but not both at the same time since field_layout breaks the Layout Builder).

Committed and pushed to 8.5.x, and published the change record. Thanks!

DamienMcKenna’s picture

Woohoo, thanks everyone!

ipwa’s picture

This is so amazing, thanks guys!

Status: Fixed » Closed (fixed)

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

tim.plunkett’s picture

Component: layout.module » layout_discovery.module