Comments

Status:Active» Needs review
StatusFileSize
new4.93 KB

This is an example of a panel pane for service links. It allows choice of icon type, and display style.

StatusFileSize
new4.92 KB

and again with fewer whitespace errors.

StatusFileSize
new4.88 KB

And one more time without duplicate divs.

Cool, 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 after array).
-- +            'value' => 'service_links_fisheye' technically this wants a trailing comma.

D) content_type plugins don't use a trailing period on the admin title:

+function service_links_service_links_content_type_admin_title($subtype, $conf, $context) {
+  return t('Service links.');
+}

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

StatusFileSize
new4.64 KB

Thanks 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 :)

still applies to latest version.

StatusFileSize
new8.43 KB

I've added few more configurations to panel settings form, so that admin can choose which services required per pane.

StatusFileSize
new8.12 KB

updated working patch..

StatusFileSize
new7.75 KB

I'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.

StatusFileSize
new7.61 KB

Fixed undefined index issue.

Here is the last patch, rerolled from the modules root directory, and not drupal core root.

Can 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...)

Status:Needs review» Reviewed & tested by the community

Looks like @aendrew forgot to change the status to 'Reviewed & tested by the community' :)

@jenlampton Indeed I did (Or I was waiting for one other person to confirm it worked; I forget). Thanks!

This depends from panel and ctools, could you provide it as external plugin as i did for service_links_share, service_links_displays... ?

It 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

Status:Reviewed & tested by the community» Needs work

As far i know there is no language which accept an undefined constant, indeed i had:

Notice: Use of undefined constant PANELS_REQUIRED_CTOOLS_API - assumed 'PANELS_REQUIRED_CTOOLS_API' in service_links_ctools_plugin_directory() (line 1096 of .../all/modules/service_links/service_links.module).

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:

<?php
function service_links_ctools_plugin_directory($module, $plugin) {
 
// Safety: go away if CTools is not at an appropriate version.
  // the check below is the key
 
if (defined('PANELS_REQUIRED_CTOOLS_API')) {
    if (!
module_invoke('ctools', 'api_version', PANELS_REQUIRED_CTOOLS_API)) {
      return;
    }
    return
'plugins/' . $plugin;
  }
}
?>

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

Status:Needs work» Needs review
StatusFileSize
new8.09 KB

Sounds 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

nice clean patch. Also includes the new SERVICE_LINKS_STYLE_EMPTY option for panels config.

Great 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 :)

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new7.72 KB

ok fixed #21 and reviewed! Please commit since it's really usefull :)

Status:Reviewed & tested by the community» Fixed

Since 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)!

Status:Fixed» Closed (fixed)

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

Issue summary:View changes

alter request for simplicity. patch coming soon.

I get an error when trying to insert the pane:

An AJAX HTTP error occurred.
HTTP Result Code: 200
Debugging information follows.
Path: /panels/ajax/editor/add-pane/panel_context%3Anode_view%3A/middle/service_links/service_links
StatusText: OK
ResponseText:
Fatal error:  Unsupported operand types in /home/s00e7441bec4588d/www/sites/default/modules/service_links/plugins/content_types/service_links.inc on line 183

I experience the problem on my site, but tested this also on a clean simplytest.me installation.

Regarding #24: my fault, I did not actually have any services selected. Still was kind of confused to get the AJAX error for that.

Issue summary:View changes

Patch in #21 works for me!