Problem/Motivation

In D7 Facet API, there is an option on widgets that allows site builders to configure a soft limit. All facets above this limit are hidden and a clickable "Show more" link is shown.

Proposed resolution

Port the functionality to Facets (D8).

Remaining tasks

None.

User interface changes

"Show more"/"Show fewer" JS enabled links.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#22 interdiff.txt1.39 KBclaudiu.cristea
#22 2724381-22.patch7.76 KBclaudiu.cristea
#19 interdiff.txt572 bytesclaudiu.cristea
#19 2724381-19.patch7.48 KBclaudiu.cristea
#8 2724381-7.patch6.93 KBclaudiu.cristea
#5 2724381-5.patch7.67 KBclaudiu.cristea
#5 interdiff.txt1.47 KBclaudiu.cristea
#2 2724381-2.patch7.12 KBclaudiu.cristea
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

claudiu.cristea created an issue. See original summary.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented
Title: Port "Show more"/"Show fewer" functionality from D7 » Port "Show more"/"Show less" functionality from D7
Status: Active » Needs review
FileSize
2724381-2.patch7.12 KB

Patch.

Status: Needs review » Needs work

The last submitted patch, 2: 2724381-2.patch, failed testing.

The last submitted patch, 2: 2724381-2.patch, failed testing.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.47 KB
2724381-5.patch7.67 KB

Fix.

Status: Needs review » Needs work

The last submitted patch, 5: 2724381-5.patch, failed testing.

The last submitted patch, 5: 2724381-5.patch, failed testing.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented
Status: Needs work » Needs review
FileSize
2724381-7.patch6.93 KB

Sorry, the patch from #5 was malformed. interdiff from #5 is OK.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented
Issue tags: +Needs manual testing
StryKaizer’s picture

StryKaizer
Primary language Dutch
Location Belgium
CreditAttribution: StryKaizer as a volunteer and commented 
+++ b/src/Plugin/facets/widget/LinksWidget.php
@@ -61,6 +62,12 @@ class LinksWidget implements WidgetInterface {
+    if (!empty($configuration['soft_limit'])) {
+      $build['#attached']['library'][] = 'facets/soft-limit';
+      $build['#attached']['drupalSettings']['facets']['softLimit'][$facet->id()] = (int) $configuration['soft_limit'];
+    }

Is this only empty when soft_limit is enabled?
No need to add the js library if its disabled I suppose

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented

@StryKaizer, yes this is what I'm doing here. I'm adding the library only when soft_limit is not 0 (aka "No limit").

StryKaizer’s picture

StryKaizer
Primary language Dutch
Location Belgium
CreditAttribution: StryKaizer as a volunteer and commented

Okay, ignore my comment
I never use the empty command obviously ;)

Patch looks good.
I used data-attribute instead in the checkbox/dropdown widgets for passing on config to js, but we prolly should use drupal.settings as you did in this patch for more consistency. I'll create an issue for that.

Code is rtbc for me, but still needs manual testing.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented

I changed the your checkbox data-facet-id > data-drupal-facet-item-id because it's about items. The I added a new data-drupal-facet-id to the list of facets (the UL list). Probably we should keep them.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented

Probably I would move the data-drupal-facet-item-id from checkbox to its parent (Links) to have it in all situations.

StryKaizer’s picture

StryKaizer
Primary language Dutch
Location Belgium
CreditAttribution: StryKaizer as a volunteer and commented
Status: Needs review » Needs work

patch from #8 was not working with me (both for lists and checkbox widget)
It looks like this is a js selector issue, as you already mentioned in #14

data-drupal-facet-id is on the parent div in my case, instead of the ul

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented

Works for me. I tried Links, Checkbox and a custom widget that is extending Checkbox. But my site the heavily themed so, probably, this affects where the selector is placed?

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented

data-drupal-facet-id is on the parent div in my case, instead of the ul

Can you investigate why this happens? I cannot reproduce that situation

StryKaizer’s picture

StryKaizer
Primary language Dutch
Location Belgium
CreditAttribution: StryKaizer as a volunteer and commented

Sure, doing right now.

Vanilla drupal with standard profile (so bartik) here.
Drupal 8.2.x

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented
Status: Needs work » Needs review
FileSize
2724381-19.patch7.48 KB
interdiff.txt572 bytes

OK, we are fixing also a bug here (see the interdiff.txt). The block build should be an array of items. Otherwise \Drupal\block\BlockViewBuilder::preRender() is moving all #attributes on the block level.

Status: Needs review » Needs work

The last submitted patch, 19: 2724381-19.patch, failed testing.

The last submitted patch, 19: 2724381-19.patch, failed testing.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented
Status: Needs work » Needs review
FileSize
2724381-22.patch7.76 KB
interdiff.txt1.39 KB

Yeah, forgot to treat also the empty scenario.

StryKaizer’s picture

StryKaizer
Primary language Dutch
Location Belgium
CreditAttribution: StryKaizer as a volunteer and commented

Great work, nice clean code!
Tested, works as expected.

Extremely minor nitpick, but this can also be fixxed while commiting I suppose.

RTBC for me

  1. +++ b/src/Plugin/facets/widget/LinksWidget.php
@@ -155,20 +162,30 @@ class LinksWidget implements WidgetInterface {
+   * @todo This is inheriting noting. We need a method on the interface and,

    Extremely minor nitpick: Typo in todo ;) (not(h)ing) 

  2. +++ b/src/Plugin/facets/widget/LinksWidget.php
@@ -155,20 +162,30 @@ class LinksWidget implements WidgetInterface {
+    // @todo This should be handled upstream, in facet entity. Facet schema
+    //   should be fixed and all configs should get sane defaults.

    Created an issue for this #2725453: Refactor widget plugins by adding interface, base class, schema

StryKaizer’s picture

StryKaizer
Primary language Dutch
Location Belgium
CreditAttribution: StryKaizer as a volunteer and commented
Status: Needs review » Fixed
StryKaizer’s picture

StryKaizer
Primary language Dutch
Location Belgium
CreditAttribution: StryKaizer as a volunteer and commented

Dropdown got broken by this, fix coming up

  • StryKaizer committed dd6660d on 8.x-1.x 
    Issue #2724381 by StryKaizer: Port "Show more"/"Show less" functionality...

Status: Fixed » Closed (fixed)

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