Currently, the only way to edit any Panels display used by Panelizer is via the Panels IPE. While this works, it's super awkward for working with Panels defaults, especially in non-default view modes (basically, you have to create a View that displays one piece of content of the given content type in the given view mode - then view it, and use the IPE to "Save as default" - crazy, but it works!).

Instead, it'd be great to have an admin UI at admin/structure/types/manage/%/display/% (or appropriate location for the given entity type) which allows placing blocks and changing layout (currently, there is no context or relationship support at all - but once there is, it should be included here too. See #2664686: Implement support for adding arbitrary static contexts to Panels displays in Panelizer and #2664688: Implement support for adding relationships to Panels displays in Panelizer).

This issue isn't about adding support for "Panel choice" (ie. multiple defaults for a single entity type, bundle and view mode) because that'll need support in the Panelizer field, so we'll do that in a separate issue: #2664698: Allow multiple defaults and "Panel choice"

CommentFileSizeAuthor
#103 2664682-103.patch117.26 KBphenaproxima
#101 2664682-101.patch117.26 KBphenaproxima
#99 interdiff-2664682-93-99.txt10.05 KBphenaproxima
#99 2664682-99.patch116.22 KBphenaproxima
#93 interdiff-panelizer-2664682-90-93.txt265 bytesDane Powell
#93 panelizer-2664682-93.patch116.34 KBDane Powell
#4 Selection_007.png67.53 KBjuampynr
#4 Selection_008.png38.22 KBjuampynr
#4 Selection_009.png42.81 KBjuampynr
#4 implement_admin_ui_for-2664682-4.patch0 bytesjuampynr
#5 implement_admin_ui_for-2664682-4.patch10.86 KBjuampynr
#6 interdiff.txt4.49 KBjuampynr
#6 implement_admin_ui_for-2664682-6.patch13.17 KBjuampynr
#6 Selection_001.png25.71 KBjuampynr
#8 Selection_002.png17.48 KBjuampynr
#8 Selection_003.png27.87 KBjuampynr
#8 implement_admin_ui_for-2664682-7.patch15.07 KBjuampynr
#8 interdiff.txt3.95 KBjuampynr
#9 Selection_001.png50.07 KBjuampynr
#9 interdiff.txt32.08 KBjuampynr
#9 implement_admin_ui_for-2664682-9.patch43.53 KBjuampynr
#10 interdiff.txt2.33 KBjuampynr
#10 implement_admin_ui_for-2664682-10.patch45.44 KBjuampynr
#13 Selection_003.png45.79 KBjuampynr
#13 implement_admin_ui_for-2664682-13.patch61.92 KBjuampynr
#13 interdiff.txt22.56 KBjuampynr
#15 Selection_001.png58.97 KBjuampynr
#15 interdiff.txt8.77 KBjuampynr
#15 implement_admin_ui_for-2664682-15.patch62.78 KBjuampynr
#18 implement_admin_ui_for-2664682-18.patch66.46 KBjuampynr
#18 interdiff.txt5.1 KBjuampynr
#19 interdiff.txt3.38 KBjuampynr
#19 implement_admin_ui_for-2664682-19.patch69.84 KBjuampynr
#21 Selection_009.png59.46 KBjuampynr
#21 Selection_010.png45.98 KBjuampynr
#21 Selection_011.png26.9 KBjuampynr
#21 interdiff.txt7.28 KBjuampynr
#21 implement_admin_ui_for-2664682-21.patch73.68 KBjuampynr
#23 interdiff.txt20.62 KBjuampynr
#23 implement_admin_ui_for-2664682-23.patch70.02 KBjuampynr
#25 2664682-25.patch80.18 KBEclipseGc
#25 2664682.interdiff.txt45.85 KBEclipseGc
#27 2664682-27.patch95.92 KBEclipseGc
#28 2664682-28.patch70.54 KBEclipseGc
#28 2664682-interdiff.txt30.07 KBEclipseGc
#30 2664682-30.patch71.01 KBEclipseGc
#30 2664682-interdiff.txt1.09 KBEclipseGc
#31 2664682-31.patch74.5 KBEclipseGc
#31 2664682-interdiff.txt5.24 KBEclipseGc
#32 2664682-32.patch80.83 KBEclipseGc
#32 2664682-interdiff.txt3.26 KBEclipseGc
#33 2664682-33.patch86.39 KBphenaproxima
#35 2664682-35.patch91.47 KBphenaproxima
#35 interdiff-2664682-33-35.txt30.99 KBphenaproxima
#37 2664682-37.patch93.66 KBphenaproxima
#37 interdiff-2664682-35-37.txt6.25 KBphenaproxima
#39 2664682-39.patch93.63 KBphenaproxima
#39 interdiff-2664682-37-39.txt1000 bytesphenaproxima
#41 2664682-41.patch93.53 KBEclipseGc
#41 2664682-interdiff.txt4.49 KBEclipseGc
#45 2664682-45.patch94.27 KBEclipseGc
#45 2664682-interdiff.txt3.31 KBEclipseGc
#48 2664682-48.patch105.49 KBEclipseGc
#48 2664682-interdiff.txt19.24 KBEclipseGc
#49 2664682-49.patch109 KBEclipseGc
#49 2664682-interdiff.txt8.88 KBEclipseGc
#52 2664682-52.patch109.42 KBsamuel.mortenson
#52 interdiff-2600634-49-52.txt3.13 KBsamuel.mortenson
#54 2664682-54.patch109.42 KBbalsama
#54 2664682-interdiff--52-54.txt849 bytesbalsama
#55 2664682-55.patch108.93 KBbalsama
#55 2664682-interdiff--52-55.txt849 bytesbalsama
#90 2664682-90.patch111.74 KBhctom
#90 interdiff-2664682-89-90.txt1.2 KBhctom
#89 2664682-89.patch112.23 KBphenaproxima
#89 interdiff-2664682-87-89.txt5.95 KBphenaproxima
#87 2664682-87.patch110.51 KBEclipseGc
#78 2664682-78.patch114.76 KBjaperry
#78 2664682-interdiff-55-78.txt8.22 KBjaperry
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dsnopek created an issue. See original summary.

dsnopek’s picture

dsnopek’s picture

Issue summary: View changes

Added link to the "Panel choice" issue: #2664698: Allow multiple defaults and "Panel choice"

juampynr’s picture

Issue summary: View changes
Status: Active » Needs work
FileSize
67.53 KB
38.22 KB
42.81 KB
0 bytes

Here is my first patch. Still a work in progress. Here is what I did so far:

1. I changed the flag that activates Panelizer at Manage Display so when you click on the following link for a view mode, you start the wizard that will guide you to set General settings, Contexts, Layouts and Content:

Panelizer wizard

2. Clicking there opens the wizard's first step:

Panelizer wizard general

3. The next step is the contexts form, which I am still working on:

Panelizer wizard contexts

Next steps are:

[ ] Add Layout step.
[ ] Add Content step.
[ ] Create the Wizard trail interface (so you can edit each of the steps once the wizard is complete). This will replace core's Manage Display functionality.
[ ] Add tests.

@dsnopek, @eclipsegc and myself agreed this morning to skip the Access step as we don't know if it is needed.

juampynr’s picture

Doh! Submitted an empty patch. Here it is.

juampynr’s picture

Status: Needs work » Needs review
FileSize
4.49 KB
13.17 KB
25.71 KB

Here I am adding the remaining forms to manage contexts within the wizard. It is based on the Ctools and Page Manager ones. This reminds me that there is a bug in Ctools where some contexts cannot be added: #2696819: Cannot add contexts whose entity ids are not numeric.

Context step

I am now moving on to the Layout step. Here is the updated task list:

[X] General step.
[X] Contexts step.
[ ] Access step.
[ ] Layout step.
[ ] Content step.
[ ] Extend the Panelizer schema to save the Wizard data on Finish.
[ ] Implement logic to take over Manage Display when the wizard has completed.
[ ] Wizard trail.

Status: Needs review » Needs work

The last submitted patch, 6: implement_admin_ui_for-2664682-6.patch, failed testing.

juampynr’s picture

This patch implements the Layout selection step and part of the Content step. Since there is only one variant I have hardcoded a few things which I am not sure if they are correct or not.

This is how the Layout step looks like:

Layout selection step

And this is the Content step:

Content step

I still need to define routes for managing blocks within regions. I will take Page Manager as a guide for this. Here is the updated task list:

[X] General step.
[X] Contexts step.
[ ] Access step.
[X] Layout step.
[ ] Content step.
[ ] Extend the Panelizer schema to save the Wizard data on Finish.
[ ] Implement logic to take over Manage Display when the wizard has completed.
[ ] Wizard trail.

juampynr’s picture

Here I have added the Content step. Blocks can now be added, edited and deleted from a region. Here is a screenshot:

Content step

@EclipseGC: there are many of the classes of the interdiff which are almost identical to the Page Manager ones. Should we extend them and override what's different in Panelizer? Please have a look at the interdiff to see what I mean.

Here is the updated task list. I will now work on the finish() method so it saves the wizard data into core.entity_view_display.*.*.*.third_party.panelizer.

[X] General step.
[X] Contexts step.
[ ] Access step.
[X] Layout step.
[X] Content step.
[ ] Extend the Panelizer schema to save the Wizard data on Finish.
[ ] Implement logic to take over Manage Display when the wizard has completed.
[ ] Add the wizard trail (the UI to access to each of the wizard steps once it has been completed).

juampynr’s picture

Status: Needs work » Needs review
FileSize
2.33 KB
45.44 KB

Here I have adjusted the finish() method so it saves the Panels Display settings stored throughout the wizard steps. I am not sure if I am doing it right so I did not bother in refactoring much. I did not have to extend the schema since we are simply changing the UI.

Here is the updated task list:

[X] General step.
[X] Contexts step.
[ ] Access step.
[X] Layout step.
[X] Content step.
[ ] Extend the Panelizer schema to save the Wizard data on Finish.
[X] Save the Panelizer settings once the wizard finishes.
[ ] Implement the routing logic to take over Manage Display when the wizard has completed.
[ ] Create and render the wizard trail.

Status: Needs review » Needs work

The last submitted patch, 10: implement_admin_ui_for-2664682-10.patch, failed testing.

juampynr’s picture

I discussed the tasklist with @eclipsegc yesterday. We agreed on skipping the item to take over Manage Display and instead focus in creating the wizard trail separately. @eclipsegc will take my progress on Monday to complete and polish what is left to complete this patch.

Here is the updated task list:

[X] General step.
[X] Contexts step.
[ ] Access step.
[X] Layout step.
[X] Content step.
[ ] Extend the Panelizer schema to save the Wizard data on Finish.
[X] Save the Panelizer settings once the wizard finishes.
[ ] Implement the routing logic to take over Manage Display when the wizard has completed.
[ ] Create and render the wizard trail.
[ ] Add basic test coverage.

juampynr’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
45.79 KB
61.92 KB
22.56 KB

Added the wizard trail. Once you complete the wizard steps, you are redirected there. Here is an screenshot of how it looks like:

Wizard trail

There are a few things to fix here so it can work properly. I am adjusting the task list below:

[X] General step.
[X] Contexts step.
[ ] Access step.
[X] Layout step.
[X] Content step.
[ ] Extend the Panelizer schema to save the Wizard data on Finish.
[X] Save the Panelizer settings once the wizard finishes.
[ ] Implement the routing logic to take over Manage Display when the wizard has completed.
[X] Create and render the wizard trail.
[ ] Automatically load the current entity type as a context. For example, load the entity:node context for articles.
[ ] Fix a bug when loading the Content step at the Wizard trail.
[ ] When a bundle is panelized, change the link "Panelize this node" to "Go to Panelizer settings", which links to the wizard trail.
[ ] Add a submit button to the wizard trail to Delete Panelizer settings for the bundle.
[ ] Add basic test coverage.

Status: Needs review » Needs work

The last submitted patch, 13: implement_admin_ui_for-2664682-13.patch, failed testing.

juampynr’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
58.97 KB
8.77 KB
62.78 KB

Here I have fixed the issue with the contexts and content steps not showing data at the Wizard Trail. I am saving contexts in a panels variant. I order to be able to save it, I had to create a dummy page (from page manager) as PageVariant::save() calls Page::getDependendencies().

I have also added the current entity's context. I could not use the Panelizer's identifier for this (@panelizer.entity_context:entity) because this is not a valid machine name. I therefore used "panelizer_context_entity".

Here is an screenshot of the contexts step after completing the wizard:

Wizard trail contexts step

Here is the updated task list:

[X] General step.
[X] Contexts step.
[ ] Access step.
[X] Layout step.
[X] Content step.
[ ] Extend the Panelizer schema to save the Wizard data on Finish.
[X] Save the Panelizer settings once the wizard finishes.
[ ] Implement the routing logic to take over Manage Display when the wizard has completed.
[X] Create and render the wizard trail.
[X] Automatically load the current entity type as a context. For example, load the entity:node context for articles.
[X] Fix a bug when loading the Content step at the Wizard trail.
[ ] Save panelizer defaults at the Wizard Trail on "Update and Save" or "Save".
[ ] When a bundle is panelized, change the link "Panelize this node" to "Go to Panelizer settings", which links to the wizard trail.
[ ] Add a submit button to the wizard trail to Delete Panelizer settings for the bundle.
[ ] Add basic test coverage.

Status: Needs review » Needs work

The last submitted patch, 15: implement_admin_ui_for-2664682-15.patch, failed testing.

dsnopek’s picture

I could not use the Panelizer's identifier for this (@panelizer.entity_context:entity) because this is not a valid machine name. I therefore used "panelizer_context_entity".

This is a little worrisome! Core contexts (which we would like to use in some form eventually) use service identifier type names like "@panelizer.entity_context:entity". So, we might have something wrong in the storage and/or form?

juampynr’s picture

@dsnopek, I see how can overcome the identifier issue that you mentioned above.

Here I have fixed the Update/Update and Save/Save/Cancel buttons of the Wizard Trail. I am about to fix a bug with the Context step and then add testing coverage.

Here is the updated tasklist:

[X] General step.
[X] Contexts step.
[ ] Access step.
[X] Layout step.
[X] Content step.
[ ] Extend the Panelizer schema to save the Wizard data on Finish.
[X] Save the Panelizer settings once the wizard finishes.
[ ] Implement the routing logic to take over Manage Display when the wizard has completed.
[X] Create and render the wizard trail.
[X] Automatically load the current entity type as a context. For example, load the entity:node context for articles.
[] Fix a bug when loading the Content step at the Wizard trail.
[X] Save panelizer defaults at the Wizard Trail on "Update and Save" or "Save".
[ ] When a bundle is panelized, change the link "Panelize this node" to "Go to Panelizer settings", which links to the wizard trail.
[ ] Add a submit button to the wizard trail to Delete Panelizer settings for the bundle.
[ ] Add basic test coverage.

juampynr’s picture

Status: Needs work » Needs review
FileSize
3.38 KB
69.84 KB

Here I am adding basic testing coverage for the wizard.

Here is the updated task list. I will work on Monday on the two remaining items:

[X] General step.
[X] Contexts step.
[ ] Access step.
[X] Layout step.
[X] Content step.
[ ] Extend the Panelizer schema to save the Wizard data on Finish.
[X] Save the Panelizer settings once the wizard finishes.
[ ] Implement the routing logic to take over Manage Display when the wizard has completed.
[X] Create and render the wizard trail.
[X] Automatically load the current entity type as a context. For example, load the entity:node context for articles.
[] Fix a bug when loading the Content step at the Wizard trail.
[X] Save panelizer defaults at the Wizard Trail on "Update and Save" or "Save".
[ ] When a bundle is panelized, change the link "Panelize this node" to "Go to Panelizer settings", which links to the wizard trail.
[ ] Add a submit button to the wizard trail to Delete Panelizer settings for the bundle.
[X] Add basic test coverage.

Status: Needs review » Needs work

The last submitted patch, 19: implement_admin_ui_for-2664682-19.patch, failed testing.

juampynr’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
59.46 KB
45.98 KB
26.9 KB
7.28 KB
73.68 KB

This patch alters the Manage Display tab so if the display mode is managed by Panelizer, it displays a link. Here are the different statuses:

1. If the display is not Panelized, it shows a link at the bottom:

Default state
2. When you click it, it goes through the wizard steps. Once you complete it, if you go again to the Manage Display tab, the field overview table is hidden and there is a link to the Panelizer settings:

Panelized view mode

3. I have moved the flag "Panelize this display mode" to the General step. This makes it possible to switch on/off Panelizer settings:

Main flag

I have completed the task list. I will now take the chance to do some code refactoring to the patch.

Status: Needs review » Needs work

The last submitted patch, 21: implement_admin_ui_for-2664682-21.patch, failed testing.

juampynr’s picture

Status: Needs work » Needs review
FileSize
20.62 KB
70.02 KB

Here I have refactored the Add/Edit wizard classes by moving common code to a base class.

Status: Needs review » Needs work

The last submitted patch, 23: implement_admin_ui_for-2664682-23.patch, failed testing.

EclipseGc’s picture

Ok, the defaults are actually working at this point and I spent a lot of effort removing the page_variant related stuff. This patch SHOULD no longer be reliant on PageManager code in any way. In addition to this I updated the block placement and configuration screens to use the common tempstore the rest of the wizard is using. This removes a lot of WTF going on.

Finally, I started adding a space for enumerating multiple defaults. This is the code in the .module file. It's test code at best right now, but I added a "label" to wizard. This means that a machine_name will be generated from that, and that's going to be wrong, so for the moment manually name your machine names {entity_type_id}__{bundle}__{normal_machine_name} (and those are double underscores. I'll get that machine name squared away first thing when I pick this patch back up next week.

Eclipse

juampynr’s picture

  1. +++ b/panelizer.routing.yml
    @@ -72,25 +72,25 @@ panelizer.wizard.step.context.delete:
    +    tempstore_id: 'panelizer.wizard'
    

    I think that page_manager uses a different tempstore for block selection. Shall we change it there as well?

  2. +++ b/src/Wizard/PanelizerEditWizard.php
    @@ -192,10 +185,24 @@ class PanelizerEditWizard extends PanelizerWizardBase {
    +      }
    ...
    +      }
    
    +++ b/src/Wizard/PanelizerWizardBase.php
    @@ -43,22 +83,19 @@ abstract class PanelizerWizardBase extends FormWizardBase {
    +      $operations['content']['form'] = PanelizerWizardContentForm::class;
    

    @andrewbelcher asked about this a week ago as he stumbled upon it with Minipanels.

EclipseGc’s picture

ok, working (mostly) multiple defaults including UI, with some fixes to the content step so that it doesn't cache the layout step's initial choice. A number of wizard fixes here, including the need to use CTools 8.x-3.xalpha26. Also be sure to use specifically: https://www.drupal.org/node/2697587#comment-11164355 as a patch to panels.

Plenty of work left to do, but this is beginning to take shape. I'm kind of irritated at the automatic default display that gets generated for stuff. At least I'd like to make this optional. Thoughts? No interdiff because I'm in an airport lounge and don't feel like it. I'll generate one later this week when I land somewhere solid.

Eclipse

EclipseGc’s picture

Rerolled against the new and improved 2697587 (specifically patch #22). Most of the complexity of this patch has moved into that patch in terms of content placement, block selection/adding/configuring/deleting. That's a huge improvement because it means that Panelizer (and mini-panels) won't need their own version of this UI and can benefit from any future UX improvements passively.

I've added the new PanelsPattern plugin implementation for Panelizer which is pretty simply and benefits from the default implementation. The routes involved are all still misconfigured with regard to access and that's the next big step. After that we will need to update tests and make sure this all looks good.

Eclipse

juampynr’s picture

Great stuff! That last patch removes a lot of code. Love it when that happens.

EclipseGc’s picture

Updated the general form step to call the buildConfigurationForm method of the variant plugin.

Eclipse

EclipseGc’s picture

Newest patches:

Takes care of the builder form element so that IPE can be chosen in it.
Beginning to work on selecting defaults during entity creation. Currently this is not working.

Eclipse

EclipseGc’s picture

Ok, selection of defaults is now working during content creation. There's a form element on the "manage display" tab that supports this. Due to core weirdness, any non-overridden view mode will all have the same panelizer defaults. So if "full", "search results" and "search index" are all not-customized view modes, then the panelizer default displays built for the default entity display will be shared across all of them. By contrast, teaser (as a seperate view mode that is "customized") will have completely independent control over whether to allow panelizer defaults choices. This is sort of weird and wonky, but I think it's by design.

Eclipse

phenaproxima’s picture

Added a (relatively shallow) functional test of this awesome patch.

Status: Needs review » Needs work

The last submitted patch, 33: 2664682-33.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
91.47 KB
30.99 KB

Miscellaneous cleanup. There are still some dependency injection holes that should be fixed, if possible.

Status: Needs review » Needs work

The last submitted patch, 35: 2664682-35.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
93.66 KB
6.25 KB

Added another test to cover things changed in #35.

Status: Needs review » Needs work

The last submitted patch, 37: 2664682-37.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
93.63 KB
1000 bytes

Minor fix in a test.

Status: Needs review » Needs work

The last submitted patch, 39: 2664682-39.patch, failed testing.

EclipseGc’s picture

Ok, this iteration of the patch is, I think, a big win.

  1. The panelizer field is now only deployed to bundles when "allow choice" or "customize" have been chosen
  2. Default choices are saved as a simple string in the field
  3. Customizations are saved in the field as normal
  4. Editing a default through IPE is 100% doable
  5. Customizing a node (even with "allow choice") is 100% doable
  6. Customized entities with "allow choice" turned on will display a "Custom Override" text on the view modes with a custom override and prevent a default from being changed.
  7. All of this "allow choice" stuff only works if you display the panelizer field in the form display

This feels pretty solid. We'll be writing more tests tomorrow to prove that it is as solid as I think it is.

Feedback VERY welcome.

Eclipse

phenaproxima’s picture

I'm not really qualified to point out major architectural or functional flaws in this patch (if there are any), so I did a relatively shallow review.

  1. +++ b/panelizer.links.action.yml
    @@ -0,0 +1,5 @@
    +  class: \Drupal\panelizer\Menu\AddDefaultLocalAction
    +  deriver: \Drupal\panelizer\Plugin\AddDefaultLinkDeriver
    

    Nit: I think the class names should be quoted...?

  2. +++ b/panelizer.module
    @@ -54,7 +107,7 @@ function panelizer_panels_build_alter(&$build, PanelsDisplayVariant $panels_disp
    +    list(,,, $default) = explode(':', $panels_display->getStorageId());
    

    list(,,, $default) is slightly confusing syntax. Can there be a comment explaining it?

  3. +++ b/panelizer.module
    @@ -161,45 +214,107 @@ function panelizer_form_entity_view_display_edit_form_alter(&$form, FormStateInt
    +      '#description' => t('When multiple defaults for a view mode are available it can be useful to allow content creators to choose which default to use for a given node.'),
    

    I'd like to think of a better (less jargon-heavy) description of this field.

  4. +++ b/panelizer.module
    @@ -161,45 +214,107 @@ function panelizer_form_entity_view_display_edit_form_alter(&$form, FormStateInt
    +function panelizer_form_entity_view_display_edit_submit(&$form, FormStateInterface $form_state) {
    

    Nit: $form should be type hinted.

  5. +++ b/panelizer.module
    @@ -161,45 +214,107 @@ function panelizer_form_entity_view_display_edit_form_alter(&$form, FormStateInt
    +    if ($settings['enable'] != $form_state->getValue(['panelizer', 'enable'])) {
    +      $rebuild = TRUE;
    +    }
    

    What's the point of the $rebuild flag here? Why not simply put all the heavy rebuilding logic inside this if block?

  6. +++ b/src/Form/PanelizerDefaultDelete.php
    @@ -0,0 +1,160 @@
    +  public function buildForm(array $form, FormStateInterface $form_state, $machine_name = NULL) {
    +    list (
    +      $this->entityTypeId,
    +      $this->bundle,
    +      $this->viewMode,
    +      $this->displayId
    +    ) = explode('__', $machine_name);
    +
    +    return parent::buildForm($form, $form_state);
    

    What will happen if $machine_name is malformed for empty? I feel like this method should be throwing InvalidArgumentException.

  7. +++ b/src/Form/PanelizerWizardContextConfigure.php
    @@ -0,0 +1,94 @@
    +  protected function getParentRouteInfo($cached_values) {
    

    $cached_values needs a type hint.

  8. +++ b/src/Form/PanelizerWizardContextConfigure.php
    @@ -0,0 +1,94 @@
    +  protected function getContexts($cached_values) {
    

    Needs a type hint.

  9. +++ b/src/Form/PanelizerWizardContextConfigure.php
    @@ -0,0 +1,94 @@
    +  protected function addContext($cached_values, $context_id, ContextInterface $context) {
    

    $cached_values should be type hinted.

  10. +++ b/src/Form/PanelizerWizardContextConfigure.php
    @@ -0,0 +1,94 @@
    +  public function contextExists($value, $element, $form_state) {
    

    There should be type hints here.

  11. +++ b/src/Form/PanelizerWizardContextConfigure.php
    @@ -0,0 +1,94 @@
    +  protected function disableMachineName($cached_values, $machine_name) {
    

    And here.

  12. +++ b/src/Form/PanelizerWizardContextForm.php
    @@ -0,0 +1,152 @@
    +  protected function getContextClass($cached_values) {
    

    $cached_values needs a type hint, in every method that accepts $cached_values.

  13. +++ b/src/Form/PanelizerWizardGeneralForm.php
    @@ -0,0 +1,67 @@
    +  protected $machine_name;
    

    Should be machineName.

  14. +++ b/src/Form/PanelizerWizardGeneralForm.php
    @@ -0,0 +1,67 @@
    +    if ($form_state->hasValue('id') && !isset($this->machine_name) && $form_state->has('machine_name_prefix')) {
    

    The isset() should be empty(), in case $this->machine_name somehow becomes FALSE or an empty string (or other non-null empty value).

  15. +++ b/src/Menu/AddDefaultLocalAction.php
    @@ -0,0 +1,25 @@
    +    $this->pluginDefinition['route_parameters']['entity_type_id'] = $route_match->getCurrentRouteMatch()->getParameter('entity_type_id');
    +    $this->pluginDefinition['route_parameters']['bundle'] = $route_match->getCurrentRouteMatch()->getParameter('bundle');
    +    $this->pluginDefinition['route_parameters']['view_mode_name'] = $route_match->getCurrentRouteMatch()->getParameter('view_mode_name');
    

    For readability, can $route_match->getCurrentRouteMatch() be called once at the top of this method?

  16. +++ b/src/Panelizer.php
    @@ -342,6 +355,43 @@ class Panelizer implements PanelizerInterface {
    +      // If we still don't find a display, then we won't find a Panelizer
    +      // default for sure.
    +      if (!$display) {
    +        return NULL;
    +      }
    

    I think it would be slightly less confusing to move this check out of the first !$display check.

  17. +++ b/src/Panelizer.php
    @@ -342,6 +355,43 @@ class Panelizer implements PanelizerInterface {
    +    if (!empty($config[$name]) && !empty($config[$name]['static_context'])) {
    

    Why not simply if (!empty($config[$name]['static_context']))?

  18. +++ b/src/Panelizer.php
    @@ -394,11 +448,12 @@ class Panelizer implements PanelizerInterface {
    +      if (($settings['custom'] || $settings['allow'])) {
    

    Nit: No need for the inner set of parentheses.

  19. +++ b/src/Panelizer.php
    @@ -564,4 +619,4 @@ class Panelizer implements PanelizerInterface {
     
    -}
    \ No newline at end of file
    

    We must appease the git gods here :)

  20. +++ b/src/PanelizerInterface.php
    @@ -115,6 +115,12 @@ interface PanelizerInterface {
    +  public function getDisplayStaticContexts($name, $entity_type_id, $bundle, $view_mode, EntityViewDisplayInterface $display = NULL);
    +
    +  public function setDisplayStaticContexts($name, $entity_type_id, $bundle, $view_mode, $contexts);
    

    Both of these methods need doc comments.

  21. +++ b/src/Plugin/AddDefaultLinkDeriver.php
    @@ -0,0 +1,56 @@
    +        "entity.entity_view_display.$plugin_id.default",
    +        "entity.entity_view_display.$plugin_id.view_mode"
    

    I'm not the biggest fan of the fact that PanelizerEntity plugins are effectively required to have the same plugin ID as the entity type they handle. Can we add an entity_type property to the PanelizerEntity annotation to make it official, and use that here? Just for the sake of keeping things clear and explicit?

  22. +++ b/src/Plugin/Field/FieldWidget/PanelizerWidget.php
    @@ -46,50 +47,21 @@ class PanelizerWidget extends WidgetBase {
    +  public function getEntityDisplayRepository() {
    

    It looks like this might need a description in the doc comment.

  23. +++ b/src/Tests/PanelizerAddDefaultLinkTest.php
    @@ -0,0 +1,51 @@
    +  public function test() {
    

    Needs a doc comment.

  24. +++ b/src/Tests/PanelizerDefaultsTest.php
    @@ -0,0 +1,78 @@
    +  public function test() {
    

    Needs a doc comment.

  25. +++ b/src/Tests/PanelizerTestTrait.php
    @@ -0,0 +1,147 @@
    +  protected function addPanelizerDefault($node_type, $display = 'default') {
    

    Doc comment!

  26. +++ b/src/Wizard/PanelizerAddWizard.php
    @@ -0,0 +1,72 @@
    +    if ($entity_type_id && $bundle && $view_mode_name) {
    +      $form_state->set('machine_name_prefix', "{$entity_type_id}__{$bundle}__{$view_mode_name}");
    +    }
    

    Maybe this should throw an exception if any of those parameters are empty...

  27. +++ b/src/Wizard/PanelizerEditWizard.php
    @@ -0,0 +1,226 @@
    +    list($entity_type, $bundle, $view_mode, $display_id) = explode('__', $this->getMachineName());
    

    Should these four variables be sanity-checked, just in case $this->getMachineName() returns garbage? It might make debugging easier.

  28. +++ b/src/Wizard/PanelizerEditWizard.php
    @@ -0,0 +1,226 @@
    +   * @param array $form
    +   * @param \Drupal\Core\Form\FormStateInterface $form_state
    

    Both of these need descriptions.

  29. +++ b/src/Wizard/PanelizerEditWizard.php
    @@ -0,0 +1,226 @@
    +    list($entity_type_id, $bundle, $view_mode) = explode('__', $this->getMachineName());
    

    Again -- I feel like these should be sanity-checked. Maybe we should have a parseMachineName() method which can explode the machine name, ensure all parts exist, and return the split array.

dsnopek’s picture

Ok, I went through as much of this patch as I could in 30 minutes. :-)

I didn't have a chance to actually test it, and since this was a top-to-bottom read-through, the stuff I noted is mostly just nit-picky. There's a couple things that would be nice to share from Panels, and one architectural change around adding more stuff to the Panelizer service, but the internal mechanics look right to me. I'd still like to do a more in-depth review before this is committed.

Here's my notes for this time:

  1. +++ b/css/panelizer.admin.css
    @@ -0,0 +1,113 @@
    +/**
    + * @file
    + * Styles for Panelizer wizard admin.
    + */
    

    Is there any way to reuse these styles from Panels? I could imagine a library that Panels defines that we could #attach when reusing it's UI components?

  2. +++ b/panelizer.module
    @@ -17,10 +18,62 @@ function panelizer_theme() {
    +    'panelizer_wizard_tree' => [
    +      'variables' => [
    +        'wizard' => NULL,
    +        'cached_values' => [],
    +        'tree' => [],
    +        'divider' => ' » ',
    +        'step' => NULL,
    +      ],
    +    ],
    

    Could we reuse this from Panels as well?

  3. +++ b/panelizer.module
    @@ -17,10 +18,62 @@ function panelizer_theme() {
     /**
    + * Preprocess function for panelizer-wizard-tree.html.twig.
    + */
    +function template_preprocess_panelizer_wizard_tree(&$variables) {
    

    And this too

  4. +++ b/panelizer.module
    @@ -161,45 +214,107 @@ function panelizer_form_entity_view_display_edit_form_alter(&$form, FormStateInt
    +        $machine_name ="{$display->getTargetEntityTypeId()}__{$display->getTargetBundle()}__{$display->getMode()}__$machine_name";
    

    Super tiny code standards thing - no space after the "=" sign

  5. +++ b/panelizer.module
    @@ -161,45 +214,107 @@ function panelizer_form_entity_view_display_edit_form_alter(&$form, FormStateInt
    -      '#default_value' => !empty($settings['enable']),
    +      '#default_value' => isset($settings['enable']) ? $settings['enable'] : FALSE,
    

    Seems unrelated?

    !empty(TRUE) == TRUE
    !empty(FALSE) == FALSE

    So this change is unnecessary

  6. +++ b/panelizer.module
    @@ -161,45 +214,107 @@ function panelizer_form_entity_view_display_edit_form_alter(&$form, FormStateInt
    +    $form['panelizer']['allow'] = [
    

    Let's not call this setting 'allow' - that's super ambiguous: 'allow' what? Can we call it 'allow_panel_choice' (for D7 classic flavor) or 'allow_default_choice' or 'allow_default_per_node' or something like that?

  7. +++ b/panelizer.module
    @@ -161,45 +214,107 @@ function panelizer_form_entity_view_display_edit_form_alter(&$form, FormStateInt
    -          ':input[name="panelizer[enable]"]' => ['checked' => TRUE],
    -        ]
    -      ]
    +          '#edit-panelizer-enable' => ['checked' => TRUE],
    

    Also, seems unrelated. IDs can't be depended on because Drupal rewrites them, so ':input' should be more stable, right? Beyond that this seems to be doing the same thing.

  8. +++ b/src/Form/PanelizerDefaultDelete.php
    @@ -0,0 +1,160 @@
    +    return 'Are you certain you want to delete this panelizer default?.';
    

    Does this need to go through translation? Or is that handled elsewhere?

  9. +++ b/src/Form/PanelizerDefaultSelect.php
    @@ -0,0 +1,132 @@
    +    return 'Are you certain you want to set this panelizer default as the default for this bundle?.';
    

    Translation? Also, this ends with both a question mark and a period :-)

  10. +++ b/src/PanelizerInterface.php
    @@ -115,6 +115,12 @@ interface PanelizerInterface {
    +  public function getDisplayStaticContexts($name, $entity_type_id, $bundle, $view_mode, EntityViewDisplayInterface $display = NULL);
    +
    +  public function setDisplayStaticContexts($name, $entity_type_id, $bundle, $view_mode, $contexts);
    +
    

    Some time ago, we discussed adding a PanelizerDisplay value object that would hold the PanelsDisplayVariant and whatever other data went along with a display (like static contexts). I'd really prefer to see us add that rather than continuing the proliferation of get/set methods on PanelizerInterface/Panelizer with this same set of arguments.

    Then we'd have a getPanelizerDisplay()/setPanelizerDisplay() only, and access the value object to deal with contexts, Panels display, etc.

    This is probably the only truly architectural comment I have :-)

  11. +++ b/src/Plugin/Field/FieldType/PanelizerFieldType.php
    @@ -86,6 +86,8 @@ class PanelizerFieldType extends FieldItemBase {
    +   * Returns the Panels display plugin manager.
    

    I prefer "Gets" for the one line description (as opposed to "Returns") - probably doesn't actually matter, though :-)

EclipseGc’s picture

EclipseGc’s picture

Implementing the access class.

dsnopek’s picture

For the permission for editing defaults: should we maybe consider using the same permission that core uses for allowing users to access the "Manage display" page for an entity? I'm not sure what permission that is, but this could create some odd situations if a user would have permission to "Manage display" but can't change the display of entity because it's Panelized.

EclipseGc’s picture

I agree, that could be awkward. I think I'm ok with adopting those perms if they're nuanced properly per entity/bundle.

Eclipse

EclipseGc’s picture

Ok, I introduced a wrapper for the field_ui access check that we can apply to other routes if we're ok with this approach. I had to utilize an adapter approach because the existing access check in core is REALLY bad and make dumb assumptions about how it will be getting data. While this is distasteful to me, the adapter is actually pretty simple and other than the fact that I end up manipulating the route that I'm passing on it's pretty clean (that was super useful here but sort of feels like it should be an immutable object, but that's a Symfony problem so *punt*).

In addition to this I've introduced another access check that can tell if a display is the default display. I went ahead and added cache tags to the access results on this because (if I understand this at all, and I may not) it seems like this should all be cache-able until such time as a change is made after which point, all the defaults related screens showing and manipulating defaults and their status should be invalidated. I tested this a bit and couldn't find anything breaking, however I'd love for someone to really check me out on this.

The field widget now supports explicitly choosing to follow the chosen default from the "manage display" UI. I've also added the functionality which will do proper reverts for our entities. If an entity chooses to follow the bundle's default, and then at some point customizes that default for it's own purposes, when it is reverted later, it will adopt whatever the current bundle default is, even if that's a completely different display. By the same token, if a default was specifically chosen when the entity was created, then reverting customizations would revert back to that originally selected default.

I think this is a really great set of improvements to the overall code base here, and it's feeling pretty solid. I'd love some more review.

Eclipse

EclipseGc’s picture

In this patch:

  1. When "allow panel choice" is on and an entity is using a default, if that default gets deleted the entity in question will fallback to the sitewide default instead of just blowing up.
  2. Improved cache tag support:
    • If a default is updated through the backend, all nodes using that default will have their render caches properly invalidated
    • If an entity has not picked a particular default and is using the site wide default for that bundle, if the site wide default changes to a completely different default, the entity's render cache will be properly invalidated

Next priorities:

  1. Look into render cache around customized entities and how this might relate to revision-based workflows like WBM
  2. Double check strings through out the patch and make sure they're consistent and sensible
  3. Context manipulation on "edit" redirects to "add" routes and needs to be fixed

Eclipse

samuel.mortenson’s picture

@@ -200,7 +201,7 @@ class Panelizer implements PanelizerInterface {
    */
   public function getPanelsDisplay(FieldableEntityInterface $entity, $view_mode, EntityViewDisplayInterface $display = NULL) {
...
+          $this->setCacheTags($panels_display, $entity, $display, $default, $settings);
+          return $panels_display;

$display may be NULL at this point in the execution (getPanelsDisplay implements a default value), so setCacheTags needs to be able to handle the case where $display is NULL.

japerry’s picture

Is the expected outcome with this patch to be able to do the following by default? It seems sorta like a bug to me.

1) Goto http://localhost/admin/structure/types/manage/page/display
2) Select 'Panelize this view mode' -- a default will appear that you can edit.
(http://localhost/admin/structure/panelizer/edit/node__page__default__def...)
3) Verify under general settings that 'In-Place Editor' is selected.
4) Edit the default, and look at content. It should be in this order...

  • Authored By
  • Authored On
  • Body

4) Goto a page node:
5) Click edit on that page node, and move the order around. Save.
6) Verify that the page node shows the fields in the new order.
7) Go back to the panelizer default. (http://localhost/admin/structure/panelizer/edit/node__page__default__def...)
8) You'll notice under content that the fields haven't changed. Click save.
9) Navigate back to the page node, you'll see that the fields have re-arranged themselves back to the default.

There is no UI like in D7 that tells the user that when saving IPE in the node that they are overriding the default. And indeed, it seems like the overridden default is temporary. Either way I don't think this is a desired outcome.

samuel.mortenson’s picture

Here's a new patch which addresses the bug I raised in #50. I also fixed spelling errors in PanelizerInterface.php since I was copying some @param blocks from there.

I went ahead and left the entity view mode empty if $display is not passed, but we could set this to something like "!none" in the cache tag as well.

Dane Powell’s picture

Patches 49 and 52 throw a number of errors when starting to create a new layout via the panelizer wizard at admin/structure/panelizer/add/node/landing_page/full :

Notice: Undefined index: exists in Drupal\Core\Render\Element\MachineName::validateMachineName() (line 245 of core/lib/Drupal/Core/Render/Element/MachineName.php).

Warning: call_user_func() expects parameter 1 to be a valid callback, no array or string given in Drupal\Core\Render\Element\MachineName::validateMachineName() (line 246 of core/lib/Drupal/Core/Render/Element/MachineName.php).

Notice: Undefined index: @panelizer.entity_context:entity in Drupal\panelizer\Form\PanelizerWizardContextForm->isEditableContext() (line 133 of modules/contrib/panelizer/src/Form/PanelizerWizardContextForm.php).

Notice: Undefined index: current_user in Drupal\panelizer\Form\PanelizerWizardContextForm->isEditableContext() (line 133 of modules/contrib/panelizer/src/Form/PanelizerWizardContextForm.php).

You can see these and the steps to reproduce in this Lightning Travis test: https://travis-ci.org/acquia/lightning/jobs/153005688#L990

balsama’s picture

This patch just removes the elseif added in #48 that emptied the panels_display if the view mode wasn't set to the current default.

This check was throwing out any custom block placements a user had made in the IPE when they tried to save the entity.

balsama’s picture

Whoops. Last patch was identical to #52. Try that again.

DamienMcKenna’s picture

Status: Needs work » Needs review

The last submitted patch, 4: implement_admin_ui_for-2664682-4.patch, failed testing.

The last submitted patch, 5: implement_admin_ui_for-2664682-4.patch, failed testing.

The last submitted patch, 8: implement_admin_ui_for-2664682-7.patch, failed testing.

The last submitted patch, 9: implement_admin_ui_for-2664682-9.patch, failed testing.

The last submitted patch, 18: implement_admin_ui_for-2664682-18.patch, failed testing.

The last submitted patch, 25: 2664682-25.patch, failed testing.

The last submitted patch, 27: 2664682-27.patch, failed testing.

The last submitted patch, 28: 2664682-28.patch, failed testing.

The last submitted patch, 30: 2664682-30.patch, failed testing.

The last submitted patch, 31: 2664682-31.patch, failed testing.

The last submitted patch, 32: 2664682-32.patch, failed testing.

The last submitted patch, 41: 2664682-41.patch, failed testing.

The last submitted patch, 45: 2664682-45.patch, failed testing.

The last submitted patch, 48: 2664682-48.patch, failed testing.

The last submitted patch, 49: 2664682-49.patch, failed testing.

The last submitted patch, 52: 2664682-52.patch, failed testing.

The last submitted patch, 54: 2664682-54.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 55: 2664682-55.patch, failed testing.

Dane Powell’s picture

My comments in #53 still apply--still seeing those errors in Lightning after applying patch in #55.

The last submitted patch, 55: 2664682-55.patch, failed testing.

j4’s picture

Hi Any update on this? Am having the same errors. Thank you!

japerry’s picture

So in #53, the first 2 errors is caused because the machine name needs its own callback. That is fixed in this patch. Working on the next errors to see if we can come to some resolution here.

With this size of this patch, and the alpha nature of panelizer, I'm really wanting us to get this in, even if its not perfect. Having to manually patch this is causing all types of issues downstream.

dsnopek’s picture

With this size of this patch, and the alpha nature of panelizer, I'm really wanting us to get this in, even if its not perfect. Having to manually patch this is causing all types of issues downstream.

Please don't rush this issue!

Most of my review from #43 is still unhandled, and I'd particularly like to see #43.10 before merging this. This was something discussed with EclipseGC quite awhile ago and we decided on an approach that involved adding a PanelizerDisplay value object, but this hasn't been implemented in the patch.

EclipseGc’s picture

David,

Yes, we totally agreed to do that up front, however given the state of the PanelizerInterface methods, I think it makes more sense to do that in a follow up and update ALL the methods there in. Otherwise, we'd have some pretty funny looking methods in the middle of that class and a really weird set of "gotcha"s. Thoughts?

Eclipse

japerry’s picture

Status: Needs work » Needs review

Soo putting on my 'sites are suffering' hat -- frankly, every day this issue is left in this state the harder it is for people to use and contribute back to panelizer.

And whether we like it or not, this patch is being used, in production, by a number of Drupal 8 sites. I'd like to get this patch in, once tests pass, and bump everything else as follow ups.

Specifically, regarding #43:
1 - 5, 7-9: We can cleanup/make efficient/refactor later.
6: I agree that it'd be nice to rename this, but again, read second paragraph, we will run into issues. Suggest we make this change later with an upgrade path. Yes yes, I know patches shouldn't have upgrade paths, but when a patch sits for this long and is this widely used, its bad to break a bunch of sites for something not absolutely necessary.

Regarding 10: Agree with EclipseGC -- this too can be refactored later, even though we agreed that we'd try to do it here... Lets focus on the change in a subsequent issue.

I know its not optimal, but with the patch as big as it is -- thats what alpha is good for, right? ;)

Status: Needs review » Needs work

The last submitted patch, 78: 2664682-78.patch, failed testing.

samuel.mortenson’s picture

What about the other comments reviewing the patch or reporting bugs? Have they been addressed yet? Looking at #42, #51, and #53 specifically.

japerry’s picture

Status: Needs work » Needs review

51 and 53 should be resolved in #78. #42 includes a bunch of nits and opinions, no bugs ;)

I'd like to make a code cleanup patch, which can be tagged novice and contains all the relevant points from #42 and #43.

dsnopek’s picture

Here's a new issue that breaks out just the API changes that I'd like to see from #43.10:

#2816725: Refactor PanelizerInterface for better DX

The current patch just demonstrates the changes - they still need to be finished.

japerry’s picture

Errors #3 and #4 in comment #53 are addressed in the patch here: #2820966: Context missing from cached values inside wizard

EclipseGc’s picture

Rerolled against head and removed the 'administer pages' permission from the PanelizerNodeFunctionalTest.php because we can't expect page_manager to be installed.

This will still fail on tests, just fails better.

Eclipse

(Sorry no interdiff)

Status: Needs review » Needs work

The last submitted patch, 87: 2664682-87.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
112.23 KB
5.95 KB

I made some UX changes to this patch. They are purely cosmetic changes, but I believe they will make the UI more understandable to people.

What I did:

  • The checkboxes to set up Panelizer are now displayed above the list of fields (or Panelizer layouts), not below it.
  • The custom and allow checkboxes are in their own collapsible element, which is only displayed when the enable checkbox is checked. This makes it clear to the user that the additional checkboxes are options appearing because the enable checkbox was checked.
  • I changed the terminology. A Panelizer "default" is now referred to as a "layout".
  • When the enable checkbox is checked, the fields table fades out to make it clear that the option has disabled the table. It looks broken if it just disappears abruptly.
  • The custom and allow checkboxes' labels are reworded and use the entity type's singular and plural labels for clarity.
  • I got rid of the very engineer-ey 'TRUE' and 'FALSE' values in the Default column. Instead, the default will display a checkmark glyph; the rest will be empty.
  • Renamed the "Label" column to the user-friendlier "Name", and "Default" to "Use as default". Ditto with the "Set default" operation.
hctom’s picture

FileSize
111.74 KB
1.2 KB

Here is a little update to the patch from #89

What I did:

  • Move setting of label for default to display to PanelizerEntityBase to have it set for all entities by default
hctom’s picture

Reorder files...

Dane Powell’s picture

Status: Needs review » Needs work

These patches seem to rely on Field UI. If you uninstall the Field UI module with this patch applied, you will be in a world of hurt, with errors like this one:

exception 'Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException' with message 'The service "access_check.panelizer.view_mode" has a dependency on a non-existent service "access_check.field_ui.view_mode".'

Because of how dangerous this bug is, I think the immediate solution should be to declare Field UI as a dependency. But long-term, it seems like we should break that dependency.

See also #2826720: Make Field UI optional

Dane Powell’s picture

Status: Needs work » Needs review
FileSize
116.34 KB
265 bytes

This adds the dependency on field_ui.

japerry’s picture

Status: Needs review » Active

Re-triggering tests.

japerry’s picture

Status: Active » Needs review

The last submitted patch, 89: 2664682-89.patch, failed testing.

The last submitted patch, 90: 2664682-90.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 93: panelizer-2664682-93.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
116.22 KB
10.05 KB

This should fix the test failures.

Status: Needs review » Needs work

The last submitted patch, 99: 2664682-99.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
117.26 KB

Rerolled. Interdiff from #99 still applies.

Status: Needs review » Needs work

The last submitted patch, 101: 2664682-101.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
117.26 KB

ahferchrissake

DamienMcKenna’s picture

@phenaproxima: Who's Chris? ;-) The tests are green, which is awesome. I'll try to review it in the morning.

japerry’s picture

Status: Needs review » Reviewed & tested by the community

I've made a follow up for #42, 6) #2833612: Change panelizer 'allow' key on edit form

It seems like EclipseGC is working on a new way to approach the architecture change in #2816725: Refactor PanelizerInterface for better DX.

However, since 701 of the 1100 or so panelizer users are also lightning D8 users, I think this patch is ready to go.

Marking RTBC, and baring any major issues, we should get this committed asap. Nits, small changes, etc, should be logged as followup issues.

DamienMcKenna’s picture

Status: Reviewed & tested by the community » Fixed

This is excellent, great work everyone!

Committed!

  • DamienMcKenna committed 5e8d0f2 on 8.x-3.x
    Issue #2664682 by juampynr, EclipseGc, phenaproxima, balsama, japerry,...
DamienMcKenna’s picture

Status: Reviewed & tested by the community » Fixed
DamienMcKenna’s picture

FYI I've created two follow-on issues for this:

There are probably lots more small things that need improving.

Status: Fixed » Closed (fixed)

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