Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jenlampton’s picture

Status: Active » Needs review
FileSize
4.93 KB

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

jenlampton’s picture

and again with fewer whitespace errors.

jenlampton’s picture

And one more time without duplicate divs.

dww’s picture

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

jenlampton’s picture

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

jenlampton’s picture

still applies to latest version.

crabsoul’s picture

FileSize
8.43 KB

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

crabsoul’s picture

FileSize
8.12 KB

updated working patch..

crabsoul’s picture

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.

crabsoul’s picture

Fixed undefined index issue.

tom friedhof’s picture

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

aendra’s picture

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

jenlampton’s picture

Status: Needs review » Reviewed & tested by the community

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

aendra’s picture

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

TheCrow’s picture

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

dww’s picture

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

TheCrow’s picture

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:

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

jenlampton’s picture

Status: Needs work » Needs review
FileSize
8.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

jenlampton’s picture

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

funana’s picture

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

muschpusch’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
7.72 KB

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

Simon Georges’s picture

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.

Anonymous’s picture

Issue summary: View changes

alter request for simplicity. patch coming soon.

drupov’s picture

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.

drupov’s picture

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

sjancich’s picture

Issue summary: View changes

Patch in #21 works for me!

Jacqs’s picture

I've just installed 7.x-2.3 and the AJAX error commented in #25 still shows.