See parent issue. There's a patch under consideration for ctools that adds a renderer for an icon+text link, in addition to the existing icon-only and text-only links. This patch for Panels IPE will check if the new function is available - in case the site has updated Panels without updating ctools. If available, it will use the combined button. This way it will not create duplicate links by using both the image and the text buttons for the same Add Content function.

CommentFileSizeAuthor
#8 panels-icon-text-renderer-2283719-8.patch1.97 KBcboyden
#7 panels-icon-text-renderer-2283719-7.patch1.94 KBcboyden
#1 panels-icon-text-renderer-2283719-1.patch1.97 KBcboyden

Comments

cboyden’s picture

cboyden commented
Status: Active » Needs review
StatusFileSize
new panels-icon-text-renderer-2283719-1.patch1.97 KB

Patch attached.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Status: Needs review » Reviewed & tested by the community

I tested manually and it works great! We've just committed this to Panopoly as well. Marking as RTBC, but I have one super minor code review comment:

+++ b/templates/panels-add-content-link.tpl.php
@@ -5,6 +5,10 @@
+  <?php if (isset($icon_text_button)) {
+    print $icon_text_button;
+  } else {
+    print $image_button; ?>
+    <div><?php print $text_button; ?></div>
+  <?php  } ?>
 </div>

While this works, this is a relatively difficult template to read. Personally, I think it'd read better like:

<?php if (isset($icon_text_button)): ?>
  <?php print $icon_text_button ?>
<?php else: ?>
  <div><?php print $text_button; ?></div>
<?php endif; ?>

Although, this is really up to the maintainer.

cboyden’s picture

cboyden commented

If you don't output the image button along with the text button, the UI and the DOM will change, which may cause problems for people who update Panels without updating ctools.

There may be a more readable way to code that in the template though.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented

Yeah, my comment wasn't about the functionality - that looks fine! - just the readability of the template. Using curly brackets in templates is a little harder to read than the colon/endif syntax.

japerry’s picture

japerry
Location KOMK
commented
Status: Reviewed & tested by the community » Needs work

Getting there! Committed the ctools patch. As for this one, there are two things I have questions/concerns about:

  1. +++ b/includes/add-content.inc
@@ -78,6 +78,9 @@ function template_preprocess_panels_add_content_link(&$vars) {
+  if (function_exists('ctools_ajax_icon_text_button')) {
+    $vars['icon_text_button'] = ctools_ajax_icon_text_button($vars['title'], $vars['icon'], $vars['url'], $vars['description'], 'panels-modal-add-config');
+  }
   $vars['image_button'] = ctools_ajax_image_button($vars['icon'], $vars['url'], $vars['description'], 'panels-modal-add-config');
   $vars['text_button'] = ctools_ajax_text_button($vars['title'], $vars['url'], $vars['description'], 'panels-modal-add-config');

    You say the goal is to have it duplicated, but I guess I don't understand how this would be a good thing. Perhaps throw a else in here? 

  2. +++ b/templates/panels-add-content-link.tpl.php
@@ -5,6 +5,10 @@
-  <?php print $image_button; ?>
-  <div><?php print $text_button; ?></div>
+  <?php if (isset($icon_text_button)) {
+    print $icon_text_button;
+  } else {
+    print $image_button; ?>
+    <div><?php print $text_button; ?></div>
+  <?php  } ?>

    I agree with dsnopek here, templates should not be using curly braces. See https://drupal.org/coding-standards#controlstruct for more info.

cboyden’s picture

cboyden commented

For #1: This covers the case where someone is overriding the template panels-add-content-link.tpl.php. They may be depending on the $image_button and $text_button variables being set. There's a bit of complexity here to cover these alternatives:

  1. ctools is updated, but not Panels. No problem, there's just a function available in ctools which doesn't get called.
  2. Panels is updated, but not ctools. No problem, the function_exists check prevents an error in add-content.inc. $image_button and $text_button are set and available for the template.
  3. Both Panels and ctools are updated, but the site overrides panels-add-content-link.tpl.php and does something different with $image_button and $text_button. No problem, $image_button and $text_button are set, even though $icon_text_button is available.

I don't know why anyone would override that template, as it's internal to Panels. But if they did, and setting the $image_button and $text_button happened inside an else block, that would cause a PHP notice and would break their template. If you think that's an extreme enough edge case I can make the change to an else block.

For #2: I will reroll the template patch using the alternate control statement syntax.

cboyden’s picture

cboyden commented
Status: Needs work » Needs review
StatusFileSize
new panels-icon-text-renderer-2283719-7.patch1.94 KB

Alternate control structure for template file. Also reroll to apply to latest dev of add-content.inc.

cboyden’s picture

cboyden commented
StatusFileSize
new panels-icon-text-renderer-2283719-8.patch1.97 KB

Updated patch to add subtype_class - see earlier commit.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Status: Needs review » Reviewed & tested by the community

Patch looks great to me! Just did some manual testing and it worked fine.

  • japerry committed 8640b5a on 7.x-3.x authored by cboyden 
    Issue #2283719 by cboyden: Duplicate links for text and icons in Panels...
japerry’s picture

japerry
Location KOMK
commented
Status: Reviewed & tested by the community » Fixed

Tested good for me as well, Fixed! Thanks dsnopek for the ping :)

Status: Fixed » Closed (fixed)

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