Problem/Motivation
The widget "Links with checkboxes" displays differently depending on whether it's inactive or active:
- Inactive facets are displayed as an unchecked checkbox followed by a linked label.
- Active facets are displayed as checked checkboxes followed by an unlinked label.
There should be an option to make active facets displayed as checked checkboxes followed by a linked label. This will make the two displays more consistent.
Proposed resolution
Add an option to the "Links with checkboxes" widget settings to display active facets as checked checkboxes followed by a linked label.
The default for this option should be to do what it did before (i.e.: display active facets as checked checkboxes followed by an unlinked label). This will prevent it from breaking existing settings.
Remaining tasks
Write patch- Module maintainer should approve small JavaScript API change (see below and comment #33) or move issue to 7.x-2.x.
- Review and RTBC
- Commit.
User interface changes
A checkbox with the title "Show the labels of active checkboxes" and the description "Displays an active item as a checkbox followed by a link." is added to the "Display settings" fieldset of the "Configure facet display" page when "Display widget" is set to "Links with checkboxes".
When this setting is set, active facets are displayed as checked checkboxes followed by a linked label.
API changes
Before this patch, Drupal.facetapi.makeCheckbox()
required the HTML element to manipulate (the facet link) to be the calling scope (this
).
After this patch, Drupal.facetapi.makeCheckbox()
requires the HTML element to manipulate to be passed as the first parameter. Existing code can be easily changed by calling Drupal.facetapi.makeCheckbox(this)
.
I don't anticipate this JavaScript-API change to be a problem, because, as far as I know, the only JavaScript that calls Drupal.facetapi.makeCheckbox()
is inside facetapi.js
, and I've fixed that. I also searched as many other modules as I could think of that depended on FacetAPI (current_search
, search_api_facetapi
, date_facets
, facetapi_bonus
, facetapi_i18n
, wetkit_search
, wetkit_apachesolr
, and facetapi_select
) — none of these modules call this function.
Original report by @Jon Pugh
A client has requested to make an active facet text a link, just like the inactive facets.
I am not a usability expert, but I think this makes sense. Whether it is checkboxes or the (-) link , it is pretty small and a pain to click, which is frustrating because you likely got there by clicking the word. My intuition tells me to click the word again to "uncheck".
I do think there should be a style change for that link (There is already a class), as making the facet text a link would make it harder to see.
Thoughts? We are going to start with a patch. We will post it here.
I do understand this will likely be responded to with "Make a FacetApiWidget", but I wanted to open the conversation to usability folks since this really seems like a good change to make.
Comment | File | Size | Author |
---|---|---|---|
#47 | facet-link-code-output.png | 165.67 KB | Internet |
#47 | without-show-labels-of-active-checkboxes.png | 32.74 KB | Internet |
#47 | with-show-labels-of-active-checkboxes.png | 30.16 KB | Internet |
#47 | show-labels-of-active-checkboxes.png | 60.67 KB | Internet |
#42 | when_a_facet_is_active-1526020-42.patch | 4.8 KB | mparker17 |
|
Comments
Comment #1
cpliakas CreditAttribution: cpliakas commentedHi careernerd.
A widget is probably the most repeatable approach, although it is not necessary. You can simply override theme_facetapi_link_inactive() and theme_facetapi_link_active() in your theme's template.php file to have control over display of these links.
At this point in the game I don't have any interest in modifying the core widgets since we are so close to a 1.0, however I would love to discuss a strategy for default widgets with better UX going forward.
Thanks,
Chris
Comment #2
fastangel CreditAttribution: fastangel commentedBut the problem is:
in facetapi.js
So I think that you could not make such changes to themes functions. It would not be correct to add a setting to make this work?
Regards.
Comment #3
fastangel CreditAttribution: fastangel commentedI attached a patch for push the active link in configuration.
Comment #4
cpliakas CreditAttribution: cpliakas commentedChanging to a feature request.
Comment #5
cpliakas CreditAttribution: cpliakas commentedMarking as needs work. The patch is made against Drupal's document root as opposed to the module's directory, it looks like there is a double ",," after an aray item, and there is also a fatal error in the patch due to a missing parenthesis after the drupal_add_js() function.
Comment #6
cpliakas CreditAttribution: cpliakas commentedFixed the issues noted above, keeping the status as "needs work" since there are a ton of "Warning: Illegal offset type in drupal_add_js() (line 4085 of /Users/cpliakas/Sites/facetapi/includes/common.inc)." errors and the setting doesn't seem to make the text a part of the link.
Comment #7
cpliakas CreditAttribution: cpliakas commentedFixed the issues noted above, keeping the status as "needs work" since there are a ton of "Warning: Illegal offset type in drupal_add_js() (line 4085 of /Users/cpliakas/Sites/facetapi/includes/common.inc)." errors and the setting doesn't seem to make the text a part of the link.
Comment #8
cpliakas CreditAttribution: cpliakas commentedSorry for the dup.
Comment #9
tecjam CreditAttribution: tecjam commentedI've been trying to get this to work also, but so far no luck.
I have rewritten the theme_facetapi_deactivate_widget($variables) {} function in my theme template.php file to return an image (a red x) rather than the standard (-), but I also wish to make the whole string the link, not just the image.
Looking forward to a fix for this.
Comment #10
ianthomas_ukHow about just using label?
<input type="checkbox" id="foo"/> <label for="foo">Facet label</label>
This is good practise for accessibility, and is less likely to affect the styling. You can use full tags for the actual link too, or just use
<a for="foo">
Comment #11
cpliakas CreditAttribution: cpliakas commentedHi guys.
Patches are very welcome for this. Given other priorities I don't see myself getting to this any time soon, so if you are just waiting and not contributing then the wait will be pretty long!
Thanks,
Chris
Comment #12
David_Rothstein CreditAttribution: David_Rothstein commentedJust a quick note that I have a patch at #1809030: Facet checkbox links don't have labels which might help a bit with #10. It doesn't implement that exactly at the moment, but it may be the direction the issue eventually heads (and in any case the patch there should be a good start).
Comment #13
Jon PughI totally agree with #10 #11 and #12!
I hadn't even thought about the label tag... those are clickable, too! Shall we abandon this option to use #1809030: Facet checkbox links don't have labels?
Comment #14
Rob230 CreditAttribution: Rob230 commentedI have clients that want this. For usability, accessibility and just following web guidelines, the checkboxes should use labels, which would mean the invisible text is not even necessary for screen readers. Additionally, having to click on a checkbox for an active facet is more awkward. Even if it's not changed to labels, I think the deactivate widget should not be used for checkboxes, and the link shouldn't be hidden.
Unfortunately I found it was very difficult to override this for my site because as has been said, the link gets removed by JavaScript. So even when I override theme_facetapi_link_active to not use theme_facetapi_deactivate_widget or theme_facetapi_accessible_markup, I end up with no text or link.
I don't have time to investigate doing this myself at the moment.
Comment #15
ace11 CreditAttribution: ace11 commentedNeed solution for this too. Pretty odd that text does not have link.
Comment #16
mpp CreditAttribution: mpp commentedWhy not simply altering the theme function for the current search keys?
You'll need to add in the path to the theme variables. Something like this:
/**
* Returns HTML for a search keys facet item.
*
* @param $variables
* An associative array containing the keys 'keys' and 'adapter'.
*
* @ingroup themeable
*/
function theme_current_search_keys($variables) {
return theme('facetapi_link_active', array(
'text' => check_plain($vars['keys']),
'path' => $vars['path'],
'options' => array(
'attributes' => array(),
'html' => TRUE,
'query' => drupal_get_query_parameters(),
),
));
}
Comment #17
derekwebb1 CreditAttribution: derekwebb1 commentedThis is still an issue. Even if you do make the text a link this js:
if (active) {
// Add the checkbox and label, hide the link.
$link.before(label).before(checkbox).hide();
}
Will hide it... and leave me with not one but two checkboxes!
So unless I write some js to unhide my added linked text I am apparently out of luck. I can't understand why the linked part of an active facet is made to be un-clickably small (-).
EDIT:
I got around this issue by adding a theme_facetapi_link_active function that adds a classed span around the text part of the active facetlink like so:
return theme_link($variables) . '<span class="linktxt">'.$link_text.'</span>';
Then in a js file that runs after facetapi I update the link text to have an actual link inside it. This is a tidbit from the search related js file that creates the link:
Comment #18
mparker17Patch in #7 didn't apply anymore, so I re-rolled it.
There was a merge conflict in facetapi.js, I resolved it to...
... as it looked like the if() statement was the part that this issue is trying to change, not the hide() statement.
Comment #19
mparker17Hmm, a misplaced closing parenthesis in the call to drupal_add_js() in
plugins/facetapi/widget_links.inc
causesWarning: Illegal offset type in drupal_add_js() (line 4200 of includes/common.inc).
errors. The attached patch should fix this.Comment #20
mparker17Actually, this addresses the problem @cpliakas pointed out in #6 so I'm marking this as "Needs review" again.
Comment #22
mparker17Ah, I was patching against 7.x-1.x-dev, not 7.x-1.0-rc4...
Comment #23
mparker1719: active-link-option-1526020-19.patch queued for re-testing.
Comment #24
mparker17Hmm... the patch doesn't work because the JS isn't getting the data from Drupal.settings.
Also, the active_link setting is not saved in the facet display settings.
I'll fix.
Comment #25
mparker17The "active_link" setting not being displayed in the facet display settings was a result of #735528: FAPI #states: Fix conditionals to allow OR and XOR constructions.
Getting data from Drupal.settings was easy.
Also I fixed a comment to conform with coding standards.
Feedback welcome!
Comment #26
cpliakas CreditAttribution: cpliakas commentedPatch looks good, but marking as needs work because it no longer applies to HEAD due to some recent commits.
Thanks for the contribution and continued efforts on this issue.
Chris
Comment #27
mparker17Straight re-roll, no changes, so no interdiff.
Comment #28
mparker17A few issues:
Links
.!empty($this->settings->settings['active_link'])
returns TRUE even ifactive_link
has not been set. If this change was to be released, existing site facets would work normally until someone saved the facet settings, at which point it would change behavior, even if they did not touch the "Active facet is a link" checkbox. This change would be unexpected.Comment #29
mparker17Okay, I'm going to work on the problems I identified one at a time. I apologize in advance for the number of new comments+patches this is going to create; but I was having trouble doing it all in one patch. On the plus side, hopefully reviewing the interdiffs will be easier.
This patch fixes the name of the setting not being very descriptive; i.e.: this:
The name of the setting has been changed for consistency. That means if you applied a previous patch from this issue, and you update to the patch attached to this comment or later, you'll need to re-save your configuration! Note that, due to the "The active_link toggle passed to the front-end is not keyed per-facet" bug I identified earlier, this means you'll have to re-save all your configuration. Sorry... it had to happen sometime, might as well be early, right?
Comment #30
mparker17This patch fixes:
Comment #31
mparker17This patch fixes:
Comment #32
mparker17Whoops... forgot to remove some Javascript from the
facetapi.admin.js
file. This was supposed to happen in the previous comment (#31).Comment #33
mparker17Okay, here's the patch to fix:
Marking as "needs issue summary update".
To get this working, I ended up having to change the JavaScript-API for
Drupal.facetapi.makeCheckbox()
slightly. Any function that calls it and expects it to treatthis
as the HTML element to manipulate will not work. These functions will have to passthis
as the first argument.However, I don't anticipate this JavaScript-API change to be a problem, because, as far as I know, the only JavaScript that calls
Drupal.facetapi.makeCheckbox()
is insidefacetapi.js
, and I've fixed that. I also searched as many other modules as I could think of that depended on FacetAPI (current_search
,search_api_facetapi
,date_facets
,facetapi_bonus
,facetapi_i18n
,wetkit_search
,wetkit_apachesolr
, andfacetapi_select
) — none of these call it.Detailed notes on my JavaScript changes if you want to follow along:
Drupal.facetapi.makeCheckbox()
function.$links.once('facetapi-makeCheckbox').each(Drupal.facetapi.makeCheckbox(facet_settings));
would NOT work because the jQueryeach()
function passes in it's own two arguments (index
andelement
).makeCheckbox()
was defined separately (i.e.: not an anonymous function in the first argument of theeach()
call), JavaScript scoping rules didn't apply, so I could not access variables defined inDrupal.facetapi.makeCheckboxes()
.each()
call, which could take advantage of the scoping rules. The only thing this function does is callmakeCheckbox()
with the additional parameters:element
tomakeCheckbox()
and I have replaced references tothis
with references toelement
inside it. This was necessary because thethis
object exists in the scope of the anonymous function in the first parameter of theeach()
call, but it's not passed to anything called by that anonymous function.element === this
(exactly equivalent i.e.: literally the same object in memory), so I know I can safely do that.Drupal.facetapi.makeCheckbox()
,this
has been assigned to something different, so I can't easily add something likeif (!this) { this = element; }
to the top ofmakeCheckbox()
.Comment #34
mparker17Updated the issue summary.
But, as I was writing the update, I realized the output is
[checkbox] [linked deactivate widget] [unlinked label]
, which is not Jon Pugh's desired output. So back to needs work.Comment #35
mparker17Turns out that the output comes from the
theme_facetapi_link_active()
function, and is the same as the active "Link" widget text.I guess we have a few different ways to proceed:
theme_facetapi_link_active()
.$variables
variable inFacetapiWidgetLinks::buildListItems()
, then modifytheme_facetapi_link_active()
so it outputs the markup differently based on that parameter (and force #2290031: Make inactive, active facet markup more consistent and easier to theme to be refactored).theme_facetapi_link_active()
andtheme_facetapi_link_inactive()
output essentially the same markup.... some feedback from someone else would be appreciated at this point.
Comment #36
mparker17Needs someone else to review...
Comment #37
legolasboI've reviewed the patch and could only find nitpicks. Besides that I can report that the patch works as advertised.
I think this comment only adds noise because the code speaks for itself.
It seems this code was moved within the same file. This adds noise to the patch and makes it harder to review.
This could be a simple but descriptive function call, like addShowActiveLabelJsSetting.
That way we could lose the comment and have cleaner more descriptive code.
Comment #38
mparker17@legolasbo, makes sense to me. I'll try to upload a new patch in the next week or so with the nitpicks, but I'll leave it as RTBC in case the maintainer gets to it before I do.
Comment #39
mparker17Patch in #33 needed a re-roll. Since this is a re-roll, an interdiff is not necessary.
Comment #41
mparker17Oops, apparently I fail at fixing merge conflicts during rebase. Let's try this new, better re-roll that doesn't accidentally duplicate
FacetapiWidgetLinks::getItemClasses()
.Comment #42
mparker17The attached patch addresses the issues pointed out in the code review in #37:
1. The comment has been removed.
2. This is no longer happening since the re-roll, so there is nothing to do.
3. I added a private function with the name you suggested (
addShowActiveLabelJsSetting()
) and called it.Further reviews welcome!
Comment #43
B-Prod CreditAttribution: B-Prod commentedThe patch seems to work fine and do not break existing behaviours.
Comment #44
Internet CreditAttribution: Internet as a volunteer and commentedPatch failed on both the 7.x-1.5 module and dev version ( git and manually attempt ).
widget.links.inc broke site.
facetapi.js failed -
The dev version uses: $elem while the 7.x-1.5 version uses $link
if (active) {
checkbox.attr('checked', true);
// Add the checkbox and label, hide the link.
$elem.before(label).before(checkbox).hide();
}
else {
$elem.before(label).before(checkbox);
}
}
if (active) {
checkbox.attr('checked', true);
// Add the checkbox and label, hide the link.
$link.before(label).before(checkbox).hide();
}
else {
$link.before(label).before(checkbox);
}
}
Any guidance? Would really like to use this patch.
Comment #45
mparker17I can confirm that the patch does not apply against the latest 7.x-1.x HEAD (and I will re-roll the patch).
However, the patch applies with no errors or warnings to 7.x-1.5 for me:
@Internet, do you have any other patches applied to Facet API 7.x-1.5?
***
Also, what do you mean by "widget.links.inc broke site."? How did it break the site / what error did you get? Did you flush caches before you got the error?
Comment #46
mparker17Here is an updated patch. This is a straight re-roll so no interdiff is needed. I have not had a chance to run it though.
Comment #47
Internet CreditAttribution: Internet as a volunteer and commentedThank you, the Instructions worked well and the patch is applied (below). However the facet links appear to behave similarly to before (caches cleared and update run) as the link label / text does not form part of the (deselect) link - screenshots below. Is this the intended behaviour, if so is there perhaps a method to include the text?
HEAD is now at 316b904... Issue #2159883 by Daemon_Byte, cpliakas: Date facets not displayed when the configured granularity is larger than the calculated granularity.
Marks-MBP:facetapi markcullen$ git apply --index ~/Downloads/when_a_facet_is_active-1526020-42.patch
Marks-MBP:facetapi markcullen$ git status
HEAD detached at 7.x-1.5
Changes to be committed:
(use "git reset HEAD ..." to unstage)
modified: facetapi.js
modified: plugins/facetapi/widget_links.inc
Comment #48
Internet CreditAttribution: Internet as a volunteer and commented#46 patch works against the current module, though as above, the text is not actually a link, which would be highly desirable as it allows for some very elegant styling solutions. If this is not currently possible I'll attempt a css fix to overlay the text and post here.
Very nice module though that we use on High Visibility house sales (real estate) website https://www.ibayhomes.com/properties
Comment #49
eugene.ilyin CreditAttribution: eugene.ilyin as a volunteer and at DrupalJedi commentedI don't know why theme_facetapi_link_active could not help before, but now you can meet your requirements easy.
1. Overwrite function theme_facetapi_link_active in your theme, for example as my_theme_facetapi_link_active
2. Change logic a bit from
to
3. Overwrite function Drupal.facetapi.makeCheckbox in JS to do not hide a link.
Comment #50
eugene.ilyin CreditAttribution: eugene.ilyin as a volunteer and at DrupalJedi commented