The current JS implementation blindly applies the checkbox behavior and show/hide behavior to all block facets even if the type of widget is something totally different.

This is possibly relevant for the tagcloud, slider, and other possible block implementations. If nothing else, the JS should be faster if we don't try to do that extra processing.

CommentFileSizeAuthor
#18 1175718-18.patch3.43 KBpwolanin
#17 1175718-17.patch3.32 KBpwolanin
#8 1175718-8.patch2.49 KBpwolanin
#7 1175718-7.patch1.69 KBpwolanin
#5 1175750-5.patch3.35 KBpwolanin
#4 1175750-4.patch3.17 KBpwolanin
#1 1175718-1.patch1.81 KBpwolanin

Comments

pwolanin’s picture

pwolanin commented
Status: Active » Needs review
StatusFileSize
new 1175718-1.patch1.81 KB

sacrificing the 'type' setting, since we are debating making that an array or even how it's relevant.

cpliakas’s picture

cpliakas commented
Category: bug » feature

I see what you are getting at. Could you further explain the "widgetFamily" to me? We already have the machine readable name of the widget. Couldn't we use that for the conditional?

Changing to a feature request because I think the functionality works as-is and I don't see it conflicting with future widgets. Obviously I could be missing something.

pwolanin’s picture

pwolanin commented

Well, I did not want to hard-code the allowed widget names there. Imagine I want to make another widget that derives from FacetapiWidgetLinks and wants to use the soft/hard limit feature.

By having this "family" notion, we allow this new custom widget to benefit from the JS defined by the main module.

The alternative I initially considered was having a setting that lists all the link-type widget names, but that was a bit uglier in terms of implementation and further bloats the JS settings.

pwolanin’s picture

pwolanin commented
Category: feature » bug
StatusFileSize
new 1175750-4.patch3.17 KB

per discussion with Chris, making this more targeted as part of the settings, since the "Family" concept is not necessarily clear.

pwolanin’s picture

pwolanin commented
StatusFileSize
new 1175750-5.patch3.35 KB

After more discussion - a simplified patch.

pwolanin’s picture

pwolanin commented
Title: facetapi.js applies link-type behavior to all facet blocks » Add checks to facetapi.js before applying link-type behavior to all facet blocks
Status: Needs review » Fixed
pwolanin’s picture

pwolanin commented
Status: Fixed » Needs review
StatusFileSize
new 1175718-7.patch1.69 KB

Follow-up to better target the adding of checkboxes and minimize the JS overhead.

Also replaces the class rather than adding another.

pwolanin’s picture

pwolanin commented
StatusFileSize
new 1175718-8.patch2.49 KB

better comments and method names too.

cpliakas’s picture

cpliakas commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

pwolanin’s picture

pwolanin commented

Given the other checks we have, we can probable even remove the condition

 if ('block' == settings.facetapi.facets[index].realmName)

what do you think?

cpliakas’s picture

cpliakas commented

I think that makes sense. I want to try to get away from hard-coding the realms if possible. I created a task a while ago at #1147500: Improve the JavaScript API to remember to improve the JS API. I am even tpying with the idea of specifying the method or methods that are invoked per-facet in the jsSettings. For example:

$this->jsSettings['callbacks'] = array(
  'Drupal.facetapi.method1',
  'Drupal.facetapi.method2',
);

The methods would be out checkbox functionality or show more / show less stuff. That way we can eliminate any checks all together. Would like to hear thoughts on that approach.

pwolanin’s picture

pwolanin commented

What's the use case for such dynamic specification of the methods? Two facet widgets with the same 'limit' setting but different ways of handling it? Seems clearer to just have a different setting name?

cpliakas’s picture

cpliakas commented

How I see it, this would allow for different widgets to reuse the same settings. For example:


// For the "links" widget...
$this->jsSettings['callbacks'] = array(
  'Drupal.facetapi.applyLimit',
);

// For the "checkboxes" widget...
$this->jsSettings['callbacks'] = array(
  'Drupal.facetapi.makeCheckboxes',
  'Drupal.facetapi.applyLimit',
);

This way you could reuse settings without having to do the conditionals we have been working with. Also, as it stands every JS file associated with a widget has to iterate over the settings and do their own conditional to apply their own settings. Check out the for loop in the Chart Facets module's JS file. Defining the callbacks would eliminate the need for the widgets to loop through the settings in order to figure out which widgets should be acted on.

pwolanin’s picture

pwolanin commented

Any concern about bloating the settings?

cpliakas’s picture

cpliakas commented

In what way? Are you concerned about the amount of data being transferred as JS settings, the actual $this->jsSettings variable getting too big and unwieldy, or something else I am not thinking of?

pwolanin’s picture

pwolanin commented

More the JS settings in the page getting big, though really it's not going to make much difference if there are only a half dozen fcets displayed on a page.

pwolanin’s picture

pwolanin commented
StatusFileSize
new 1175718-17.patch3.32 KB

Here's a version with somewhat more changes that reduces code duplication and should be more compatible to defining a single callback.

pwolanin’s picture

pwolanin commented
StatusFileSize
new 1175718-18.patch3.43 KB

Make code clearer by removing repeated use of $(this)

pwolanin’s picture

pwolanin commented
Status: Reviewed & tested by the community » Fixed

committed

cpliakas’s picture

cpliakas commented

Awesome. Love the elimination of all that code. Moving the "callbacks" functionality to a post at #1147500: Improve the JavaScript API.

Status: Fixed » Closed (fixed)

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