FacetAPI Collapsible completely takes control over FacetAPI theming hooks in facetapi_collapsible_theme_registry_alter(). It doesn't allow for other modules or the theme to alter existing hooks. Will attach a patch shortly.

CommentFileSizeAuthor
#1 1836748-facetapi_collapsible-theme_hooks-1.patch3.83 KBmarkhalliwell

Comments

markhalliwell’s picture

markhalliwell
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new 1836748-facetapi_collapsible-theme_hooks-1.patch3.83 KB

Patch attached.

markhalliwell’s picture

markhalliwell
Primary language English
commented

Just to clarify, this patch is taking #1812994: Theme overrides do not work a step further and allows theming to remain completely hook-able (for other modules and themes). The only thing this module really should be doing is wrapping facet links with a custom div and appending a toggle span in the title of the facet.

Themes should still be able to implement their own hooks for facetapi theme functions (to construct their own titles, links, counts, etc).

attiks’s picture

attiks commented
Status: Needs review » Needs work

Nice work, one question:

+++ b/facetapi_collapsible.moduleundefined
@@ -19,60 +19,34 @@ function facetapi_collapsible_facetapi_widgets() {
-  foreach (array('facetapi_title', 'facetapi_link_inactive', 'facetapi_link_active', 'facetapi_count') as $theme_func) {
...
+  foreach (array('facetapi_title', 'facetapi_link_inactive', 'facetapi_link_active') as $theme_function) {

why is facetapi_count removed?

markhalliwell’s picture

markhalliwell
Primary language English
commented
Status: Needs work » Needs review

Because this module really shouldn't be theming the facets. The only other reason I left the others in is because markup needs to be injected or wrapped around those elements. I'm guessing the count theme hook got thrown in the code at some point to "pretty it up" by injecting the <span/> wrapper so it could be aligned to the right; it also took out the parenthesis. In reality though, it's the theme that should be implementing THEME_facetapi_count to inject markup or change the output however it's needed.

attiks’s picture

attiks commented
Priority: Critical » Major
Status: Needs review » Fixed

#4 True, thanks for the clarification.

Committed and removed the following as well

.facetapi-collapsible h2 {
  background: url(images/expand.png) no-repeat right center;
}
.facetapi-collapsible.expanded h2 {
  background: url(images/collapse.png) no-repeat right center;
}
markhalliwell’s picture

markhalliwell
Primary language English
commented

Good catch.

Status: Fixed » Closed (fixed)

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