Add an additional template on top of facets_result_item for wrapping of those results. See #12

CommentFileSizeAuthor
#16 add_template-2796143-16.patch4.83 KBshkiper
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dkontorovsky created an issue. See original summary.

borisson_’s picture

That sounds like a good idea, no idea how to go about it, but it sounds good.

johnwebdev’s picture

Instead of the plugin returning a item_list, it should instead return another template that wraps the item_list. This allows us to override the wrapper template.

borisson_’s picture

@johndevman, that sounds like a solution, not sure if it's a good solution though. It'd be great to have an opinion of a front-end type person about this.
However this doesn't really fix the specific problem in the IS.

johnwebdev’s picture

I did some quick drawing:

http://i.imgur.com/EhjFNFQ.png

By having a "Wrapper" template, we should be able to place a label, and then "Item list" and finally some "Reset button". The Reset button could perhaps be included as an item itself, as in the the issue "All links". Not sure.

The Item list could be anything, perhaps a <select> or, if we just listed as links a <ul>. The item could pretty much remain the same, as the current template. But for this to work, each Widget would be responsible for having it's own set of templates to work with.

This gives us much more flexibility as a front-end developer and we can more or less remove most of the JavaScript code.

So we could probably (for item list and item) get away easy by changing:

  protected function buildResultItem(ResultInterface $result) {
    $count = $result->getCount();
    return [
      '#theme' => 'facets_result_item',
      '#is_active' => $result->isActive(),
      '#value' => $result->getDisplayValue(),
      '#show_count' => $this->getConfiguration()['show_numbers'] && ($count !== NULL),
      '#count' => $count,
    ];
  }

and

build()-method in the WidgetPluginBase:

    return [
      '#theme' => 'item_list',
      '#items' => $items,
      '#attributes' => ['data-drupal-facet-id' => $facet->id()],
      '#cache' => [
        'contexts' => [
          'url.path',
          'url.query_args',
        ],
      ],
    ];

which theme it uses. Perhaps by passing some data when calling the parent::build on each Widget.

$build = parent::build($facet);

ollu’s picture

@johndevman, looks like a nice solution and I like the idea of having template files that I can override instead of javascript.

johnzzon’s picture

I too like the possibility to have template files for this.

johnwebdev’s picture

Have there been any discussion around this one @borisson_?

borisson_’s picture

This was planned to be discussed at last week's hangout, but we had to cancel that due to other engagements that the other maintainers had. I asked the frontend developer here at work to have a look at this issue to help decide direction - he hasn't had the time to look at this so I'll ask again. I'll send out a tweet to ask for more input on this issue.

StryKaizer’s picture

Sorry I missed the hangout, here's my POV

We can/should not allow users to theme the checkboxes using TWIG.
The reason is, checkboxes in facets are actually links (getting tranformed into checkboxes by javascript).
This is because facets does not support changing more then 1 facet at once. Therefor, every check/uncheck should always trigger a request.

Once we allow twig templates on checkboxes, we can not ensure every check/uncheck triggers a request, which breaks facets.
Also we can not ensure gracefull degration into links once javascript is disabled as we do now.

So for me, this can only implemented if we rewrite our url_processors to support multiple facet-enabling-values at once.

borisson_’s picture

This is because facets does not support changing more then 1 facet at once. Therefor, every check/uncheck should always trigger a request.

So for me, this can only implemented if we rewrite our url_processors to support multiple facet-enabling-values at once.

I totally forgot about this. I guess this means this issue can be closed as won't fix and we should document the reason somewhere?

johnwebdev’s picture

What if each widget are responsible if it's overridable or not? Personally I'd rather deal with those 2 issues but still having the markup control rather than not being able to do anything, or resolve things by doing ugly JavaScript/CSS-hacks.
Or we could make sure a checkbox/dropdown widget are not overridable. Either way having a wrapper template around the item list does not make any difference to these issues above. Even though we can override things with templates, we don't have to remove the JavaScript. So checkboxes template could still be links with an JavaScript "transformer".

Being able to override templates, and wrapping everything using a wrapper instead of returning an item list gives so much more flexibility. However there are room for limitation if required be.

borisson_’s picture

We figure that a wrapping template is a good idea as a nice to have.

borisson_’s picture

Title: Allow full markup control » Add a wrapping template
Issue summary: View changes

Updated IS.

borisson_’s picture

To finish this issue we need to do the following:

  1. Add a new entry to the hook_theme implementation. (facets_item_list)
  2. Add a template that just copies the contents of item_list.html.twig
  3. Test that changing that template changes the output.
shkiper’s picture

Status: Active » Needs review
FileSize
4.83 KB
borisson_’s picture

Status: Needs review » Reviewed & tested by the community
borisson_’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed, thanks!

  • borisson_ committed 5bac28d on 8.x-1.x authored by shkiper
    Issue #2796143 by shkiper, borisson_, johndevman, StryKaizer: Improve...

Status: Fixed » Closed (fixed)

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