Currently the Panopoly Accordion style says to input "-1" into the active textfield to have all panes start collapsed. That worked fine in jQuery UI 1.8, but 1.9 and later allow negative indexing for the accordion (http://api.jqueryui.com/accordion/#option-active), so it needs to be explicitly set to false.

CommentFileSizeAuthor
#9 panopoly_theme-accordion_active_selection-2530966-9.patch1.15 KBhart0554
#6 panopoly_theme-accordion_active_selection-2530966-6.patch1.72 KBhart0554
#5 panopoly_theme-accordion_active_selection-2530966-5.patch1.63 KBhart0554
#1 panopoly_theme-accordion_active_selection-2530966-1.patch1.45 KBhart0554

Comments

hart0554’s picture

hart0554 commented
StatusFileSize
new panopoly_theme-accordion_active_selection-2530966-1.patch1.45 KB

Patch attached

hart0554’s picture

hart0554 commented

Just realized this patch isn't quite going to work because it's going to read '0' as empty. I'll rework this and upload a fix.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Priority: Normal » Critical
Issue tags: +sprint

Thanks for the report and working the patch! :-)

This will probably also need a hook_update_N() to change the value for existing sites that have -1 already saved.

Marking as Critical because this is a regression.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Status: Active » Needs work
hart0554’s picture

hart0554 commented
StatusFileSize
new panopoly_theme-accordion_active_selection-2530966-5.patch1.63 KB

I'm not 100% sure I know how to go about updating that setting with a hook_update_N, I'd be interested in seeing how someone tackles that. What I've done with this updated patch is account for negative int values by assuming that the intention is actually to have no active accordion sections, since that's been the described behavior of the setting previously. It doesn't leverage the ability to use negative indexes that jQUI provides, but at least preserves existing '-1' behavior.

I can see the utility of potentially being able to say you want the active accordion section to be the last of n, but will have to leave the more sufficient solution to someone who understanding implementing update hooks better than I do.

hart0554’s picture

hart0554 commented
StatusFileSize
new panopoly_theme-accordion_active_selection-2530966-6.patch1.72 KB

Update again because the above version forgot to account for blank value.

Lowell’s picture

Lowell commented
Status: Needs work » Reviewed & tested by the community

Tested patch #6, works as expected.
Does require the "Can close all section" checkbox to be checked.

dsnopek’s picture

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

@Lowell: Thanks for testing!

@hart0554: I think your solution to not having a hook_update_N() function is fine! However, since we are now preserving the -1 behavior, maybe we should just continue saying that -1 is what you use to say that we want no pane active? Also, the patch appears to allow you to specific the string 'false' as an option, which doesn't seem necessary. Maybe we could just check specifically for a value of -1? So, instead of:

+++ b/plugins/styles/accordion/panopoly_accordion.inc
@@ -54,7 +54,14 @@ function theme_panopoly_theme_panopoly_accordion_style_render_region($vars) {
+    if ((int)$settings_js['accordion'][$region_id]['options']['active'] < 0 || strtolower($settings_js['accordion'][$region_id]['options']['active']) === 'false') {

... we could do:

if ((int)$settings_js['accordion'][$region_id]['options']['active'] === -1) {

Then we can keep the old description as well.

How does that sound?

hart0554’s picture

hart0554 commented
Status: Needs work » Needs review
StatusFileSize
new panopoly_theme-accordion_active_selection-2530966-9.patch1.15 KB

Okay, here's an updated patch with the suggested solution.

  • dsnopek committed dff7ec5 on 7.x-1.x 
    Update Panopoly Theme for Issue #2530966 by hart0554, Lowell: Accordion...
dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Status: Needs review » Fixed

Looks good, thanks! Committed. :-)

Status: Fixed » Closed (fixed)

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