"All" link in facets?

This sounds like a great idea.

The dropdown widget seems to already have this, though:

- when first set up, the dropdown option for this has no text
- the text can set with the 'Default option label' option, which seems confusing wording to me

I think we should postpone this on #2725453: Refactor widget plugins by adding interface, base class, schema to make this easier to implement.

So, the issue we postponed this work on is now in. The slider widget that @mollux is building will also need this.

We should add a "reset_link_active" boolean and "reset_link_label" string in the schema.yml and the widget base class.

The reset link active should be a checkbox to enable the link and the label should be a textbox to add the label for the link.

I believe we also want to make sure we can add the total count of the search on the All (reset link)

To have a reset link on the Widget, I kinda did a "hack"-solution for the moment, since we needed it asap. It might come in handy for someone.

This code is placed in LinksWidget.php

    $totalCount = array_map(function (Result $result) {
      return $result->getCount();
    }, $facet->getResults());

    $request = \Drupal::request();
    $url = Url::createFromRequest($request);

    $params = $request->query->all();
    unset($params['f']); // This removes all Facets filters, beaware if you have multiple.

    $url->setOption('query', $params);

    $item = [
        '#theme' => 'facets_result_item',
        '#is_active' => FALSE,
        '#value' => 'All',
        '#show_count' => TRUE,
        '#count' => array_sum($totalCount),
    ];
    $item = (new Link($item, $url))->toRenderable();
    array_unshift($build['#items'], $item);

This patch is based on @johndevman's "hack", not sure if this is the best solution but it actually works in my limited testing, this needs validation if this is a good way to do this and if it's validated, we should add tests, both an integration test and a unit test adaption for the QueryStringTest unit test.

Actually attaching the patch now.

array_map

returns an array. So we need to use array_sum to calculate the count of the All link. Attached patch:

I think I submitted the patch wrong?

You need to create the patch from the facets directory, looks like you created it from the root-drupal directory. Can you also provide an interdiff?

Here we go again:

Ugh... @borisson_ could you help me out here? Not sure why it's not prompted for testing. Sorry for the spam.

The issue needs to have "Needs review" status for the tesbot to pick up the patch. Changed status

It seems to work as expected. Do we have any other input on this? I could provide the integration and unit tests required otherwise.

The more I look at this, the more I'm unsure about the direction we took here, by adding the config directly into the facet entity and not providing a processor that does this. I think that adding a processor would create more work for the sort processors as they shouldn't just sort this link with the other items but I'm not sure about the architecture at this point.

I wanted to discuss that at the last facets hangout, but we cancelled that because of time restrictions. I'll try to ping @StryKaizer to see if he has an opinion, once we've validated that this is the correct way to do this we can write unit + integration tests.

But we first need to validate that this is a good solution.

We had a short discussion about this in irc, StryKaizer agreed that this should not live on the facet config.

borisson_: StryKaizer: if you have some time later: https://www.drupal.org/node/2692027 needs validation if the patch is the correct way to resolve the issue.
StryKaizer: borisson_ : gave it a quick look just now, and I share your opinion at first sight, this should not live on the facet config indeed
StryKaizer: It prolly needs to be a widget setting
borisson_: StryKaizer: oh, yeah. That makes a ton more sense
StryKaizer: We could make an interface for this, of many widgets will use it
StryKaizer: Not sure though

This means that both the initial assesment in #6 and the patch I created in #10 are not a good solution.

Setting this issue back to active. The first thing we should do is write and decided a good way to do resolve this before diving into the code again.

Sorry for the work you already put into this @johndevman.

It's better to make a good solution, rather than a temporary one. So no worries. I'll see If I can contribute some once some initial work been done on the issue.

So If I got it right, the idea is to move these configuration up a level to the Widget, which decides if it should render an Reset link or not, rather then on the Facet which enforce a Widget to handle a reset button.
Is that correct?

Yep, that sounds like a better solution.

I understand this is being rewritten, but I can confirm that the patch gets the job done. For those that need the functionality right away, I've re-rolled the patch to apply cleanly on the latest dev build:

Last updated: 20 Dec 2016 at 11:38 PST
Last packaged version: 8.x-1.0-alpha6+45-dev
Git Hash: 445efcac0e76cb870e2f22a7c12bf6542a862c6a

This is the approach with the processor. Not fully tests yet but this is a good start I think, trying to confirm that later.

Isn't this approach similar to the first one? It enforces an Add link, but not actually anything confirming that the widget will handle it, for example the Dropdown widget already have an All link, with this one enabled it would show 2 all options. Per se it's not that much of issue, but imo confusing.

In functionality this doesn't really change a lot, that's true. However this is a processor so we're not baking it into the facet object and thus making it easier to swap out with a custom implementation. While it is true that the current dropdown widget already has an all link, this can be replaced with this approach. Since we're adding #2611198: Give widgets the ability to require settings we can make the dropdown widget require this processor.

But doesn't that mean we actually enforce an All link to the widget? Maybe that's okay, but I believe it makes more sense for it to be an option, not an requirement.

This processor results in an error on my site.
Error: Call to undefined method Drupal\facets\Plugin\facets\processor\AddAllLink::sortResults() in Drupal\facets\FacetManager\DefaultFacetManager->Drupal\facets\FacetManager\{closure}() (line 334 of modules/contrib/facets/src/FacetManager/DefaultFacetManager.php).

In my facet config, this processor seems to be added automatically to the 'sort' stage so DefaultFacetManager calls this method sortResults on it.

Yeah, this is because the processor also defines that stage and doesn't add the method. I think that's also the reason why there's so many failures on that patch. I figured we'd also need to include a implementation to sort because I don't think we want to have the 'all link' sorted alphabetically. I was half-way trough trying to figure that out when I uploaded this patch. This is currently very low on the priority-list for me but if you (or anyone really) would want to work on this I've updated the IS.

I created a patch for 'List of links' widget, which adds an option to add "Show all" link in the facet widget. I also made text on link configurable on widget configuration form.

Let's see what the test thinks

Needs work for the test

For me the patch was not working as expected, seems that whatever the first element in the results array is, the reset link will activate the same facet. Clearing the query value on that url object should take user to the path where all facets are cleared. Patch provided. Tests still needed.

Darn, the $results[0] url object was being altered with that approach. Creating a new object seems to work better, patch attached.

It looks like the reset link doesn't have the same classes as a normal facet item.

Further, this will reset all facets at once, not just this facet. I think that's a problem.

In any case, this adds test-coverage on both unit as integration level.

Looks like there was no config declaration. I fixed that, however tests don't pass yet locally. I don't have more time today so uploading current work.

This should do it, at least for the kernel test.

We have sufficient test-coverage now, I'd love if anyone here can manually test and rtbc this patch.

This patch removes all selected facets. Initially, I wrote a patch (#34), which removes just facet selection from one facet widget, not all (this was the use case we needed in our project). If this is the feature you want, then this patch works. Maybe we could add another setting, where users could choose if they want to reset all facets or just this one. What do you think?

#46, this is not what's intended, let's expand the testcoverage to make sure that we include that and fix it in the patch.

It should reset only one facet.

Here is a patch, which resets only one facet. Tests need to be updated.

@icurk: do you happen to have an interdiff to #44?

Here is interdiff between #44 and #48.

Cfr An option to add a (block) link to reset all facets at once would be a valuable UX improvement.

Rerolled.

Go testbot, go!

Refactoring of fantastic patch 52 (https://www.drupal.org/files/issues/all_link_in_facets_-2692027-52.patch) from @borisson_
If the checkbox 'Show the amount of results' on the facet is checked, then show the total number of results between brackets.

Refactoring of fantastic patch 52 (https://www.drupal.org/files/issues/all_link_in_facets_-2692027-52.patch) from @borisson_
If the checkbox 'Show the amount of results' on the facet is checked, then show the total number of results between brackets.

Hello, After apply the patch I noticed 'All' remove the query string from the URL, I've updated the latest patch to work latest dev version and fixed the query string issue.

I think we should make use of the new url generator class to do this. This is also a reroll that actually applies and uses the new service.

It doesn't work yet - and doesn't passes tests either, I hope @StryKaizer can have a look at this.

No interdiff, because this didn't apply anymore. tests should be green.

I forgot to copy over the config.

This works, but the resulting link doesn't have any special class to identify the list item for the reset link, making it a pain to theme.

Also, unlike any of the other facet links, it's always set to being inactive, which works for a "reset" link, but not for designs that want to have an "All results" link and have that highlighted when no other facet is active.

Probably also needs tests?

• Adds a 'facets-reset' class
• Adds an is-active class when no other facets are active.

I did not review this, however I did a quick test. I am using Search API 8.x-1.7. I am using facets alpha11 for which #70 does not apply.

First, I updated facets to the latest 8.x-1.x branch, applied the patch (it applies successfully). I ran drush updb (there are no updates). Then, I ran cache flush. I confirmed search functionality still works as expected (i.e. confirmed that what used to work still works).

Next, I edited the facet and see the reset options show! My facet is set to "List of links". I checked "Show reset link" and noticed the "Reset text" input appears and is required; I set it to the text "All Results". I went to the search page and clicked a facet and confirmed the result set is limited to that facet item. Then, I clicked "All Results" and confirmed the result set cleared to showing all results.

This is exactly the functionality I was needing! Thank you!!! I hope this feedback is helpful / affirming of the behavior in #70.

Yes, thanks! This still needs tests as mentioned in #70, so tagging as such. Hopefully I can get to that soon.

Updated the patch and added the class facet-item as suggested.

Looks super solid.

Attached patch fixes following issues

- hide "show all" link when all items are already shown
- generate correct url in show all link, as currently the active-items array was incorrect, which caused pretty paths to show the wrong url.

I did not yet look into the tests, so they are probably broken now.

NW based on test in #78.

#78

- hide "show all" link when all items are already shown

Actually there is genuine use case for showing the "show all" link when all items are already shown. See this image:

- hide "show all" link when all items are already shown

I exposed this one as an option. Patch test will fail for the same reason as the previous one.

@ndf,

Nothing to add. Just needed one rolled against a released version. Hid it from the list.

Thx

Hello,
Patch in #2692027-82: "All" link in facets? worked for me. I wonder if we could add an option in the facet configuration adding something like "Reset this facet" (maybe only if Show only one result is configured). In the code something like this (I don't know what could happen, I've just do a PoC and worked for my use case):

Drupal\facets\Plugin\facets\url_processor\QueryString()

        // Exclude currently active results from the filter params if we are in
        // the show_only_one_result mode.
        if ($facet->getShowOnlyOneResult()) {
          foreach ($results as $result2) {
            if ($result2->isActive()) {
              $active_filter_string = $this->urlAlias . $this->getSeparator() . $result2->getRawValue();
              foreach ($filter_params as $key2 => $filter_param2) {
                if ($filter_param2 == $active_filter_string) {
                  unset($filter_params[$key2]);
                }
              }
            }
          }
        }

        // For the example, something like:
        if ($facet->getResetMe()) {
          $filter_params = [];
          foreach ($results as $result2) {
            if ($result === $result2) {
              $filter_params[] = $this->urlAlias . $this->getSeparator() . $result2->getRawValue();
            }

Could this workaround work if we configure the "parent" facet (the one which dependent facets depend) to be reseted?

This patch does not take into account when there are other - non-facets - query parameter values present in the url. This means that for instance a view with an exposed search filter will not work because it removes that from the query parameters as well. That means that the solution in #8 actually works as intended, only remove the current filter from the query string leaving all else intact.

I've created a patch which takes into account existing query parameters.

I noticed my previous patch didn't have correct indenting so I've updated the patch.

Rerolling for better testing (maybe?)

This looks good now, thanks @lapek! I think this now has sufficient test-coverage as well. Setting to rtbc to commit the next time I get to committing things :)

Committed and pushed, thanks!

There seem to be some unintended side effects from this feature addition. If I update to 8.x-1.4 suddenly this facet is popping up on pages where it was not visible when using 8.x-1.3, and it is making these pages uncacheable.

I had to lock on the previous version 8.x-1.3 for the time being.

edit: False alarm! After investigation this turned out to be caused by a theming issue in our custom facets widget, Facets is working fine!

Thanks All for great work.

Found one point - reset link in not compatible with checkbox widget.

Patch attached

@niko-, closed tickets do not get re-patched. I created a new ticket #3064344: "All" link in facets: Fix Checkbox Widget based on your report. The patch on 3064344 takes it one step further and uses the buildListItems() so that the reset link is process the same as the other links. Testers welcome.

+++ b/src/Plugin/facets/widget/LinksWidget.php
@@ -47,6 +52,64 @@ class LinksWidget extends WidgetPluginBase {
+      $max_items = array_sum(array_map(function ($item) {
+        return $item->getCount();
+      }, $results));

This amount is incorrect when:

1. a result has multiple values for a field
2. not every result has an associated facet item (eg. field is optional)

I have filed #3087076: Amount of results for 'reset link' is calculated incorrectly to fix the amount of results for the 'reset link'.