Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
It would be great if we could have a Service Links content pane for panels.
Comments
Comment #1
jenlamptonThis is an example of a panel pane for service links. It allows choice of icon type, and display style.
Comment #2
jenlamptonand again with fewer whitespace errors.
Comment #3
jenlamptonAnd one more time without duplicate divs.
Comment #4
dwwCool, this looks really useful, thanks!
Nit-picky review:
A) Do you really want an empty
<div class="service-links"></div>
as the payload of the rendered pane if the user doesn't have 'access service links' permission? Looks like the code would throw a PHP notice, too, since $content would be undefined in that case.B) Why bother testing
if (module_exists('service_links')) {
?C) Still some whitespace errors. ;)
-- In
test ? a : b
you want spaces around the:
.--
+ 'invisible' => array (
(has an extra space afterarray
).--
+ 'value' => 'service_links_fisheye'
technically this wants a trailing comma.D) content_type plugins don't use a trailing period on the admin title:
You just want:
return t('Service links');
E) Is this really a good idea?
+ if ($module == 'page_manager' || $module == 'panels' || $module == 'ctools' || $module == 'stylizer') {
Why not just say that we define content_types in plugins/content_types and not care which module might be asking about them. Then, if other modules start using content_type plugins, we won't have to update this code.
Otherwise, looks good on visual inspection. I haven't tried testing it, but it looks like it'd work. ;)
Thanks,
-Derek
Comment #5
jenlamptonThanks for the review! :)
A) I'm not sure I see how the empty div tag is getting in there, but I've set the content to be NULL if that permission is not had. That should help with the notice.
B) Haha! Good catch, that's leftover from when this lived in a custom module of mine. Remvoed :)
c) Whitespace removed.
D) Period removed.
E) Okie, removed.
It's still working on my site with all these changes, but could use another test :)
Comment #6
jenlamptonstill applies to latest version.
Comment #7
crabsoul CreditAttribution: crabsoul commentedI've added few more configurations to panel settings form, so that admin can choose which services required per pane.
Comment #8
crabsoul CreditAttribution: crabsoul commentedupdated working patch..
Comment #9
crabsoul CreditAttribution: crabsoul commentedI've added few more configurations to panel settings form, so that admin can choose which services required per pane. this is the latest working patch.
Comment #10
crabsoul CreditAttribution: crabsoul commentedFixed undefined index issue.
Comment #11
tom friedhof CreditAttribution: tom friedhof commentedHere is the last patch, rerolled from the modules root directory, and not drupal core root.
Comment #12
aendra CreditAttribution: aendra commentedCan confirm #11 patches cleanly against latest stable (7.x-2.1).
Dear folks who contributed this patch: you are awesome and also just saved me some time. Thank you and have a nice day!
Edit: Also patches cleanly against 7.x-dev (Just realized I need that for the Pinterest widget...)
Comment #13
jenlamptonLooks like @aendrew forgot to change the status to 'Reviewed & tested by the community' :)
Comment #14
aendra CreditAttribution: aendra commented@jenlampton Indeed I did (Or I was waiting for one other person to confirm it worked; I forget). Thanks!
Comment #15
TheCrow CreditAttribution: TheCrow commentedThis depends from panel and ctools, could you provide it as external plugin as i did for service_links_share, service_links_displays... ?
Comment #16
dwwIt doesn't create a dependency. Only if those modules are enabled does this code do anything. Most modules that support ctools and panels in this way just provide it in the main module. There's practically no cost of that, and it saves all the additional cost (and development overhead) of a whole separate module.
Thanks,
-Derek
Comment #17
TheCrow CreditAttribution: TheCrow commentedAs far i know there is no language which accept an undefined constant, indeed i had:
obviously i am not using Panels where i got it...
Just because everybody does like that, doesn't mean that's right at all ;)), something like that is better:
here some reference:
http://www.php.net/manual/en/language.constants.php
http://www.php.net/manual/en/function.defined.php
http://www.php.net/manual/en/function.define.php
I wanted it on a separated module because last time i tested it i had to look the documentation to enable a second module on Ctools module set, (having no other Panel fields installed it was not loaded with Panels automatically maybe...), otherwise it was not displaying at all, maybe things changed but have to give a try again.
Btw thanks to Jen and Rashr i appreciated your work just i am caring to avoid dozens of duplicated support requests about how to enable this field on Panel :)
Thanks,
-Fabio
Comment #18
jenlamptonSounds fair. :)
This patch has been rerolled with the check from #17 and also some minor code cleanup and new docblocks, maybe left over from #11
Comment #19
jenlamptonnice clean patch. Also includes the new SERVICE_LINKS_STYLE_EMPTY option for panels config.
Comment #20
funana CreditAttribution: funana commentedGreat patch, exactly what I needed! Thank you very much.
Running it in a Panopoly install and works like a charm.
One minor thing: In the panels category tabs it created a new menu "widget" instead of using the already existing "widgets".
Again: I can confirm that it works very well :)
Comment #21
muschpusch CreditAttribution: muschpusch commentedok fixed #21 and reviewed! Please commit since it's really usefull :)
Comment #22
Simon Georges CreditAttribution: Simon Georges commentedSince a lot of people seem to be using it, I've committed it to 7.x-2.x-dev version. Let's see how it goes. Everybody, thanks for everything (your patches, your tests, your reviews, and your patience)!
Comment #23.0
(not verified) CreditAttribution: commentedalter request for simplicity. patch coming soon.
Comment #24
drupov CreditAttribution: drupov commentedI get an error when trying to insert the pane:
I experience the problem on my site, but tested this also on a clean simplytest.me installation.
Comment #25
drupov CreditAttribution: drupov commentedRegarding #24: my fault, I did not actually have any services selected. Still was kind of confused to get the AJAX error for that.
Comment #26
sjancich CreditAttribution: sjancich commentedPatch in #21 works for me!
Comment #27
Jacqs CreditAttribution: Jacqs commentedI've just installed 7.x-2.3 and the AJAX error commented in #25 still shows.