Skinr needs to have a way to add additional links to the links array (drop-downs for Display settings, Region settings, Pane settings), most likely through a drupal_alter (hook_panels_links_alter). This would make it so Skinr could apply to any display, region, or pane regardless of its Style (which is what users expect).
I'm willing to write the patch, if this has a chance of going into Panels.
From what I can tell, all of this would happen in panels/includes/display-edit.inc
| Comment | File | Size | Author |
|---|---|---|---|
| #17 | panels-d6_727396_16.patch | 10.1 KB | moonray |
| #16 | panels_727396_16.patch | 10.81 KB | moonray |
| #13 | panels_editor_links_alter.patch | 11.67 KB | moonray |
| #12 | panels_editor_links_alter.patch | 11.5 KB | moonray |
| #2 | panels_drupal_alter.patch | 5.93 KB | moonray |
Comments
Comment #1
jacineThis would be huge for us. Skinr isn't really a panels style. Currently, when you decide to use Skinr with panels, you don't get to use panel styles at all, which isn't really ideal.
Comment #2
moonray commentedOK, so I went ahead and created that patch anyway; it was fairly straight-forward. Please review.
Comment #3
ChrisBryant commentedSubscribing...
Comment #4
mike stewart commentedHas there been conversation in IRC or another thread or something? bump
Comment #5
ChrisBryant commentedHi Mike, I know moonray and merlinofchaos discussed things initially in IRC some time ago but I'm not aware of any updates.
Comment #6
ChrisBryant commentedActually I've been corrected that the discussion I mentioned was a separate topic so no updates on this yet.
Comment #7
sdboyer commentedHmm...this is interesting. Making that links array more explicit (keying being a start, and possibility sufficient) is something that will need to happen eventually anyway with the IPE and the new pluggable render pipeline, so I'm +1 in principle, at least. However, the first question that jumps to mind here based on that patch is, how would Skinr reinclude the data from the added forms back onto the pane objects? Or the display, if it's being applied there? And I take it the expectation would be that other modules implementing that alter would simply need to provide all their own callbacks, do all the tricksy modal integration, etc.?
There's no doubt that we want to figure out how to make skinr interoperation work a lot better, but I want to talk it through a bit more before deciding on an approach. So please, deluge me with your thinking more than additional patches, at least for the moment :)
Comment #8
sdboyer commentedComment #9
sarvab commentedHi Sam, just to add my few cents, I feel that at the very least, even if it includes a lot of brunt work to make it appear integrated, a module should be able to add things in there if it'd like to. Skinr doesn't need to save anything into the pane object as it already has its own storage and really is separate from panels and the pane styles. It just needs an obvious place to present its UI as well as a pane-delete hook so it knows when to delete skins.
A solution that would allow for that as well as be easy to deal with all the modal integration would likely be ideal, although I'm not sure how best that would happen :)
Comment #10
moonray commentedIdeally CTools would make it easy to present a dialog (the modal integration and callbacks), but in general I feel modules that would take advantage of hooking into panels this way should implement their own code; it's not panels' responsibility.
At least when it comes to skinr, the modal is achieved through Dialog module (which is based on CTools), so that would already be taken care of.
As sarvab pointed out above, hooks to know when panes or panels are deleted would be a very good thing. Perhaps also a hook is needed to know that the number of panels (on a panel page) have changed, when changing layouts so other modules can respond to that.
Also, as I pointed out in #760554: AJAX refreshable content framework, a unique ID per pane would be necessary as well to ensure that things match up when panes are moved around or re-imported later.
In the end, as sarvab pointed out, skinr enhances panels' layout but isn't part of panels. It does its own data storage, it's own import/export, etc.
Comment #11
jacineHey, just wondering what the status of this is? Since the IPE was implemented and this wasn't, is this something you guys are still considering? Thanks. :)
Comment #12
moonray commentedHere's an updated patch that applies to panels_renderer_editor.
It adds named keys to all links so a drupal_alter can be run on them. The hook to implement to be able to alter those links is
hook_panels_links_alter(&$links, $op, $context).$op is display | region | pane | style
$context is a keyed array with the following keys:
if $op is display: array('panels_renderer_editor' => $this)
if $op is region: array('panels_renderer_editor' => $this, 'region_id' => $region_id)
if $op is pane: array('panels_renderer_editor' => $this, 'pane' => $pane, 'content_type' => $content_type)
if $op is style: array('panels_renderer_editor' => $this, 'type' => $type, 'id' => $id)
Comment #13
moonray commentedLet's try that again with a error-less patch.
Comment #14
moonray commentedAnd of course, this is for D7.
Comment #15
merlinofchaos commentedLet's do this for both D6 and D7.
I think I'd rather have 3 different alters; D7 is moving away from using the $op at all, and I agree with that. I'd rather see:
Because working with hooks that take different arguments depending upon an $op is bad DX.
Also if we go this route, we may need to use a more hierarchical link rendering system than we're using now so that the alter can work a bit better. Perhaps we need to modify this to use drupal_render -- in D7 that should be straightforward, as there is already a #type => 'links'. I don't remember if this was in D6. If it was it should be easy to use the same code. If not we may have to fake it a bit in D6.
Comment #16
moonray commentedI updated the patch to use individual alters.
hook_panels_links_pane_alter(&$links, $op, $context)
hook_panels_links_region_alter(&$links, $op, $context)
hook_panels_links_display_alter(&$links, $op, $context)
hook_panels_links_style_alter(&$links, $op, $context)
As for using drupal_render... I left that since it looked like it might involve updating ctools, or veering away from ctools entirely. I'm not sure how you would want this handled.
This patch is for D7. A D6 patch coming right up.
Comment #17
moonray commentedAnd the D6 version of the patch.
Comment #18
gmclelland commentedsubscribing
Comment #19
damienmckennaComment #20
vijaycs85closely related to #993990: Add option to provide links to a view pane from panels pane edit page. Don't think we can get both issues in. IMHO #993990: Add option to provide links to a view pane from panels pane edit page is much simpler than adding another set of hooks. however I'm OK to close #993990: Add option to provide links to a view pane from panels pane edit page and update this one with testing.
Comment #21
gmclelland commentedSee my comment at #993990-12: Add option to provide links to a view pane from panels pane edit page for an overview of the functionality that we already have in Panels.
Comment #22
japerryPeople still use skinr? this looks like a Not going to fix issue...
Comment #23
damienmckennaThis is a feature request, so is won't happen for D6 at this point.