facet theme function clean-up

pwolanin - May 15, 2009 - 18:15
Project:Apache Solr Search Integration
Version:6.x-1.x-dev
Component:Code
Category:task
Priority:normal
Assigned:Unassigned
Status:closed
Description

The facet and unclick link code is a bit ugly - I think we can separate things a bit to make the code easier to understand

#1

pwolanin - May 15, 2009 - 18:15
Status:active» needs review
AttachmentSize
facet-cleanup-463900-1.patch 9.85 KB

#2

David Lesieur - May 22, 2009 - 16:36

Patch seems to work well.
By moving the $count argument of theme_apachesolr_facet_item() after $path and $options, it would be more consistent with theme_apachesolr_unclick_link().

#3

pwolanin - June 18, 2009 - 01:58

revised patch per suggestion.

AttachmentSize
facet-cleanup-463900-3.patch 9.85 KB

#4

JacobSingh - June 18, 2009 - 07:50

While reviewing the patch, I uncovered this bug: http://drupal.org/node/495012

Which is not related to the patch itself.

The patch itself looks okay, I don't think it will be totally intuitive for most themer. For instance ,we have apachesolr_facet_item and apachesolr_unclick_link. Should it be apachesolr_add_facet_link and apachesolr_remove_facet_link?

Anyway, here is another one with a little bit of docs in apachesolr.module as a starting point.

AttachmentSize
facet_cleanup-463900-4.patch 11.05 KB

#5

pwolanin - June 18, 2009 - 14:16

for consistency, changing apachesolr_facet_item to apachesolr_facet_link, but I think longer names are unwieldy.

Also, making the signature of apachesolr_sort_link match.

AttachmentSize
facet_cleanup-463900-5.patch 12.69 KB

#6

JacobSingh - June 18, 2009 - 14:18

I didn't test this last patch, but if that's all you changed I give it a thumbs up.

#7

pwolanin - June 18, 2009 - 18:35
Status:needs review» fixed

committed to 6.x (note I added the change to the REAME manually - I mised it in the last patch)

#9

System Message - July 3, 2009 - 19:10
Status:fixed» closed

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

 
 

Drupal is a registered trademark of Dries Buytaert.