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"
Comment | File | Size | Author |
---|---|---|---|
#103 | 2664682-103.patch | 117.26 KB | phenaproxima |
| |||
#101 | 2664682-101.patch | 117.26 KB | phenaproxima |
#99 | interdiff-2664682-93-99.txt | 10.05 KB | phenaproxima |
#99 | 2664682-99.patch | 116.22 KB | phenaproxima |
#93 | interdiff-panelizer-2664682-90-93.txt | 265 bytes | Dane Powell |
Comments
Comment #2
dsnopekAdded issues about static context and relationships.
Comment #3
dsnopekAdded link to the "Panel choice" issue: #2664698: Allow multiple defaults and "Panel choice"
Comment #4
juampynr CreditAttribution: juampynr at Lullabot commentedHere 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:
2. Clicking there opens the wizard's first step:
3. The next step is the contexts form, which I am still working on:
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.
Comment #5
juampynr CreditAttribution: juampynr at Lullabot commentedDoh! Submitted an empty patch. Here it is.
Comment #6
juampynr CreditAttribution: juampynr at Lullabot commentedHere 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.
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.
Comment #8
juampynr CreditAttribution: juampynr at Lullabot commentedThis 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:
And this is the 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.
Comment #9
juampynr CreditAttribution: juampynr at Lullabot commentedHere I have added the Content step. Blocks can now be added, edited and deleted from a region. Here is a screenshot:
@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 intocore.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).
Comment #10
juampynr CreditAttribution: juampynr at Lullabot commentedHere 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.
Comment #12
juampynr CreditAttribution: juampynr at Lullabot commentedI 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.
Comment #13
juampynr CreditAttribution: juampynr at Lullabot commentedAdded the wizard trail. Once you complete the wizard steps, you are redirected there. Here is an screenshot of how it looks like:
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.
Comment #15
juampynr CreditAttribution: juampynr at Lullabot commentedHere 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:
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.
Comment #17
dsnopekThis 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?
Comment #18
juampynr CreditAttribution: juampynr at Lullabot commented@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.
Comment #19
juampynr CreditAttribution: juampynr at Lullabot commentedHere 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.
Comment #21
juampynr CreditAttribution: juampynr at Lullabot commentedThis 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:
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:
3. I have moved the flag "Panelize this display mode" to the General step. This makes it possible to switch on/off Panelizer settings:
I have completed the task list. I will now take the chance to do some code refactoring to the patch.
Comment #23
juampynr CreditAttribution: juampynr at Lullabot commentedHere I have refactored the Add/Edit wizard classes by moving common code to a base class.
Comment #25
EclipseGc CreditAttribution: EclipseGc commentedOk, 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
Comment #26
juampynr CreditAttribution: juampynr at Lullabot commentedI think that page_manager uses a different tempstore for block selection. Shall we change it there as well?
@andrewbelcher asked about this a week ago as he stumbled upon it with Minipanels.
Comment #27
EclipseGc CreditAttribution: EclipseGc commentedok, 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
Comment #28
EclipseGc CreditAttribution: EclipseGc commentedRerolled 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
Comment #29
juampynr CreditAttribution: juampynr at Lullabot commentedGreat stuff! That last patch removes a lot of code. Love it when that happens.
Comment #30
EclipseGc CreditAttribution: EclipseGc commentedUpdated the general form step to call the buildConfigurationForm method of the variant plugin.
Eclipse
Comment #31
EclipseGc CreditAttribution: EclipseGc commentedNewest 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
Comment #32
EclipseGc CreditAttribution: EclipseGc commentedOk, 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
Comment #33
phenaproximaAdded a (relatively shallow) functional test of this awesome patch.
Comment #35
phenaproximaMiscellaneous cleanup. There are still some dependency injection holes that should be fixed, if possible.
Comment #37
phenaproximaAdded another test to cover things changed in #35.
Comment #39
phenaproximaMinor fix in a test.
Comment #41
EclipseGc CreditAttribution: EclipseGc commentedOk, this iteration of the patch is, I think, a big win.
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
Comment #42
phenaproximaI'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.
Nit: I think the class names should be quoted...?
list(,,, $default)
is slightly confusing syntax. Can there be a comment explaining it?I'd like to think of a better (less jargon-heavy) description of this field.
Nit: $form should be type hinted.
What's the point of the $rebuild flag here? Why not simply put all the heavy rebuilding logic inside this if block?
What will happen if $machine_name is malformed for empty? I feel like this method should be throwing InvalidArgumentException.
$cached_values needs a type hint.
Needs a type hint.
$cached_values should be type hinted.
There should be type hints here.
And here.
$cached_values needs a type hint, in every method that accepts $cached_values.
Should be machineName.
The isset() should be empty(), in case $this->machine_name somehow becomes FALSE or an empty string (or other non-null empty value).
For readability, can
$route_match->getCurrentRouteMatch()
be called once at the top of this method?I think it would be slightly less confusing to move this check out of the first !$display check.
Why not simply
if (!empty($config[$name]['static_context']))
?Nit: No need for the inner set of parentheses.
We must appease the git gods here :)
Both of these methods need doc comments.
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?
It looks like this might need a description in the doc comment.
Needs a doc comment.
Needs a doc comment.
Doc comment!
Maybe this should throw an exception if any of those parameters are empty...
Should these four variables be sanity-checked, just in case $this->getMachineName() returns garbage? It might make debugging easier.
Both of these need descriptions.
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.
Comment #43
dsnopekOk, 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:
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?
Could we reuse this from Panels as well?
And this too
Super tiny code standards thing - no space after the "=" sign
Seems unrelated?
!empty(TRUE) == TRUE
!empty(FALSE) == FALSE
So this change is unnecessary
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?
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.
Does this need to go through translation? Or is that handled elsewhere?
Translation? Also, this ends with both a question mark and a period :-)
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 :-)
I prefer "Gets" for the one line description (as opposed to "Returns") - probably doesn't actually matter, though :-)
Comment #44
EclipseGc CreditAttribution: EclipseGc commentedComment #45
EclipseGc CreditAttribution: EclipseGc commentedImplementing the access class.
Comment #46
dsnopekFor 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.
Comment #47
EclipseGc CreditAttribution: EclipseGc commentedI agree, that could be awkward. I think I'm ok with adopting those perms if they're nuanced properly per entity/bundle.
Eclipse
Comment #48
EclipseGc CreditAttribution: EclipseGc commentedOk, 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
Comment #49
EclipseGc CreditAttribution: EclipseGc commentedIn this patch:
Next priorities:
Eclipse
Comment #50
samuel.mortenson$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.
Comment #51
japerryIs 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...
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.
Comment #52
samuel.mortensonHere'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.
Comment #53
Dane Powell CreditAttribution: Dane Powell at Acquia commentedPatches 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 :
You can see these and the steps to reproduce in this Lightning Travis test: https://travis-ci.org/acquia/lightning/jobs/153005688#L990
Comment #54
balsamaThis 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.
Comment #55
balsamaWhoops. Last patch was identical to #52. Try that again.
Comment #56
DamienMcKennaComment #75
Dane Powell CreditAttribution: Dane Powell at Acquia commentedMy comments in #53 still apply--still seeing those errors in Lightning after applying patch in #55.
Comment #77
j4 CreditAttribution: j4 commentedHi Any update on this? Am having the same errors. Thank you!
Comment #78
japerrySo 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.
Comment #79
dsnopekPlease 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.Comment #80
EclipseGc CreditAttribution: EclipseGc commentedDavid,
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
Comment #81
japerrySoo 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? ;)
Comment #83
samuel.mortensonWhat about the other comments reviewing the patch or reporting bugs? Have they been addressed yet? Looking at #42, #51, and #53 specifically.
Comment #84
japerry51 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.
Comment #85
dsnopekHere'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.
Comment #86
japerryErrors #3 and #4 in comment #53 are addressed in the patch here: #2820966: Context missing from cached values inside wizard
Comment #87
EclipseGc CreditAttribution: EclipseGc commentedRerolled 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)
Comment #89
phenaproximaI 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:
Comment #90
hctomHere is a little update to the patch from #89
What I did:
PanelizerEntityBase
to have it set for all entities by defaultComment #91
hctomReorder files...
Comment #92
Dane Powell CreditAttribution: Dane Powell at Acquia commentedThese 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:
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
Comment #93
Dane Powell CreditAttribution: Dane Powell at Acquia commentedThis adds the dependency on field_ui.
Comment #94
japerryRe-triggering tests.
Comment #95
japerryComment #99
phenaproximaThis should fix the test failures.
Comment #101
phenaproximaRerolled. Interdiff from #99 still applies.
Comment #103
phenaproximaahferchrissake
Comment #104
DamienMcKenna@phenaproxima: Who's Chris? ;-) The tests are green, which is awesome. I'll try to review it in the morning.
Comment #105
japerryI'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.
Comment #106
DamienMcKennaThis is excellent, great work everyone!
Committed!
Comment #108
DamienMcKennaComment #109
DamienMcKennaFYI I've created two follow-on issues for this:
There are probably lots more small things that need improving.