I have a lot of product facets based on dimensions. Now only the indexed value is displayed.
Width:
* 7
* 10
* 15
It would be quite helpful to show the field suffix so it would list:
Width:
* 7 mm
* 10 mm
* 15 mm
Or in case if a price is indexed it could prefix the currency.
I can't find an option do this right now.
I would like to help but don't know where to start, would this be a separate plugin, widget or processor? Or an addition to the current default settings.
My first guess would be to extend the current list of options with a [] Display field prefix, [] Display field suffix. And fetch the field settings of the indexed field and show the prefix / suffix and extend the item template (facets-result-item.html.twig). I'm guessing this would be helpful for the checkbox, dropdown and links widget.
The most themable thing to do is to print prefix and suffix in separate values so they can be wrapped within html inside the facets-result-item.html.twig file. Making a processor limits this because the prefix and suffix would be part of the value.
Comment | File | Size | Author |
---|---|---|---|
#34 | 2909115.patch | 912 bytes | borisson_ |
Comments
Comment #2
borisson_Adding it trough a processor sounds like the most simple idea. It's possible to add html trough that as well if we use
| raw
.Comment #4
ericclaeren CreditAttribution: ericclaeren as a volunteer and at ezCompany commentedWow that's quick :) Thanks for the work. I would not have mind to put in some of the work for this. I was looking for some advice how to take on this request.
The patch applied but didn't quite worked out as my feature request intended.
A few concerns:
Undefined index: prefix in Drupal\facets\Plugin\facets\processor\PrefixSuffixProcessor->buildConfigurationForm() (line 52 of modules/contrib/facets/src/Plugin/facets/processor/PrefixSuffixProcessor.php
).Although it would be more work, a solution like count, which lives beside the value in the template would be a more stable and easier to change. Maybe like this.
<span class="facet-item__value">{{ prefix }}{{ value }}{{ suffix }}</span>
In that way currently overridden templates would not break and both prefix and suffix would be escaped.
When not using a processor, what would be the best way to handle this? I won't mind to provide a patch.
Comment #5
borisson_I was wondering if we could do this trough a processor and testing that was the same work as actually writing this implementation.
It would break overridden templates anyway, because that output is not added. So we're dealing with a bc break no matter which way you slice it.
In that case, we should change the Facet entity and adding those fields to it. Doing that makes this slightly harder to test, because it's not as isolated.
In any case, we need to write actual tests for this as well, but holding off on that until we agree on a solution
Comment #6
ericclaeren CreditAttribution: ericclaeren as a volunteer and at ezCompany commentedSorry for the delay, I might have an idea. Will try to work out an additional patch and test the solution in #2851851.
Comment #7
StryKaizerNot sure if we should include this in core facets or not.
Implementation is trivial though (although this does not support plural/singular fallback).
Should we keep on writing processors for all use cases and add them to core facets, or should we document creating processors and point users for cases like this to the documentation?
Comment #8
borisson_Because this also created a need to change the template, we might just commit the template change and the formatter should go in a seperate module. We shouldn't just keep on creating new processors.
Comment #9
ericclaeren CreditAttribution: ericclaeren as a volunteer and at ezCompany commentedWhat's the direction? So a separate processor should be created for this?
My idea for value|raw is to put to replace the value with a renderable array, in that way a prefix / suffix could be added to this array as an additional child {{ content }} -> content.value, content.prefix etc., while the current template can still be the same. That would probably collide with this idea, do you think this would be an acceptable solution?
Comment #10
borisson_Replacing the value with a renderable array might work, but I don't see why we should do that. That sounds like it would mean a big increase in our tests to cover all the cases we currently cover - but with the renderable array as well as the current string.
If it doesn't lead to the big test-change that I'm fearing it sounds like a good solution.
Comment #11
borisson_So I'd love to see the implementation in #9 to see how much of a refactor this leads us on. Not sure if I'm happy with it but I'd love to see how it looks like.
Comment #12
ericclaeren CreditAttribution: ericclaeren as a volunteer and at ezCompany commentedWill try to work out a patch, but I haven't got experience with writing tests. Do you know a good starting point for writing them?
Comment #13
borisson_A good first step would be to write the patch and put it trough the testbot to see what breaks.
Documentation for the tests can be found at https://www.drupal.org/docs/8/phpunit
Comment #14
ericclaeren CreditAttribution: ericclaeren as a volunteer and at ezCompany commentedThanks!
Comment #15
ericclaeren CreditAttribution: ericclaeren as a volunteer and at ezCompany commentedHi I have made 2 patches, one for the changes on facets, I wanted to rename the value variable to content, which is more inline with the use of renderable arrays, but did not rename it, because it would break current templates. The only use case now, I could imagine which would break this if someone has overridden the template and applied some custom filtering which expects a string instead of an array.
Also tweaked the processor a bit into a separate module. Will publish this module if that's okay with you. Also depending on if there changes would get into facets, if not, I need to adjust the build implementation back to the alteration of the result display value. And i'm still doubting if #prefix of a separate render child would be better, because the latter could be easier extracted in the template.
Ran the patch through testbot and didn't see any errors.Please let me know what you think of this change and if it has a change for survival :)
Got the prefix/suffix from the source, so the changes in https://www.drupal.org/node/2851851 worked perfectly!
--- Edit
Back to the drawing board, maybe I could change the template to {{ content }} the content variable in a preprocess, so the rest of the widgets stay the same.
Comment #16
borisson_The testfailures on php7 are expected:
Looks like php5.x is not as happy though, so that is something we really have to look into - as long as php5.5+ is still supported by drupal core we have to do the same :(
Comment #17
ericclaeren CreditAttribution: ericclaeren as a volunteer and at ezCompany commentedI'm getting the basic errors and fixed them, but don't understand how to tackle the
#value' => Array (...)
piece from the tests. I have changed it to an array so that's clear to me. But as I understand it the tests are returning strings, Should I alter the tests? But that feels like cheating.Any ideas? I'm a bit stuck on what to do.
Comment #18
borisson_We change the data-structure of those values, so we should also update the tests. That looked to be pretty easy - so I did that. Locally this makes all unit tests green.
Comment #19
borisson_Ah, I see why php5 isn't happy - there are
string
-typehints. We're still supporting php5.x, so we can't use those yet (#2842431: [policy] Remove PHP 5.5, 5.6 support in Drupal 8.7) so we have to remove those type hints.Comment #20
borisson_Comment #21
borisson_Retitling this issue to make it easier to see what exactly is going on in here.
Comment #22
ericclaeren CreditAttribution: ericclaeren as a volunteer and at ezCompany commentedShouldn't result implement RenderableInterface due to toRenderable()?
It was in #18 but removed in #20. The rest is looking very good. Adjusted the facet prefix/suffix processor to setRenderArrayProperty and everything seems to work fine. Thanks for helping out, sorry I'm taking so long to reply.
Comment #23
borisson_I added RenderableInterface to ResultInterface, making it no longer needed for the implementation to also implement the same interface. This makes sure that others can implement their own resultInterface while toRenderable will still work.
Comment #24
ericclaeren CreditAttribution: ericclaeren as a volunteer and at ezCompany commentedAh missed that, that's indeed better!
Comment #25
StryKaizerHere we introduce a facet-specific way to alter the render array.
I'd prefer to keep this similar to what core does. People should implement hooks (in this case e.g. hook_preprocess_facets_result_item) to alter render arrays.
For the original example (pre/suffix), that module should implement this hook, which has the given facet in $variables available. Check if the processor is activated and do changes if necessary.
I understand that this is more complex for developers, but I prefer this instead of inventing a custom setRenderArrayProperty function.
No need for the 'value' key, just return an array with #plain_text and #weight as keys.
Edit: not sure if weight is required either, probably can be removed too if no keys are set anymore. We can't have multiple values anyway in 1 result.
Comment #26
borisson_This fixes both items in #25, should we also introduce a test that implements that hook and checks that the results are as expected?
Comment #27
ericclaeren CreditAttribution: ericclaeren as a volunteer and at ezCompany commentedI understand, but how do you know a processor is active on a result item? Is that information available inside the #facet object?
The downside, this preprocessor should check this on every single item. Doesn't this have a performance impact? To check on every item if a processor is active and fetch the processor information?
Comment #28
StryKaizer@ericclaeren I agree that this is not an ideal solution either. Actually I'm not a big fan of using the value to render pre/suffix.
For simplicity, maybe we should just add the {{ prefix }}{{suffix}} tags in twig and define getters/setters for them.
@borisson_ @ericclaeren opinions?
Not sure if we need the renderableinterface in this case. Its not really a bonus for us (as our renderable interface is really hard to alter anyway, and as eric states, prolly adds to much performance issues when doing so in a hook).
I checked core, and they do not use a render array for the #title value in Link.php either, which is a bit similar to our result value.
@borisson_ sorry for the confusion, and thx for your work ;)
Comment #29
borisson_Not sure if we should change the entity schema, I guess it is easier. Let's do that. This is an API break though, but we can still do that now - especially if we default it to ""
Comment #30
StryKaizerSlept another night over this.
should we really support html wrappers for the pre/suffix? Is this a use-case many people need?
If we just say people should use the displayvalue to pre/append stuff, and do not allow html in our displayvalue, we keep things much simpler.
We support REST facets too, which probably want the displayvalue not rendered in html. I dont want to add the pre/suffix tags in the rest facets as a separate tag, but I dont mind being them part of the display value.
For the few users who really need separate htmlwrappers, they can use hook_preprocess_facets_result_item and do the pre/suffix logic in there, although I really doubt many will actually need separate wrappers.
opinions?
Comment #31
borisson_Sure, that sounds like it's the best idea. I forgot about rest. So that means we can use this issue to add docs about that in readme.txt?
Comment #32
ericclaeren CreditAttribution: ericclaeren as a volunteer and at ezCompany commentedHi, sorry for the delay. My girlfriend is pregnant and a baby on the way takes up a lot of my spare time. ;)
I'm okay with not adding the renderable part, with #facet as a variable, it's possible to get the data in the same way that it's fetched from the processor. It's quite some additional work for somebody who needs it to add that inside a template preprocess, but just altering the plain text is a more performant and understandable solution.
Too bad for the work that is done by @borisson_. But it was a valuable lesson for me. Thanks for your help.
I will change the prefix/suffix processor to adjust the display data an put that on drupal.org. Will add project link to this issue when it's up.
Comment #33
ericclaeren CreditAttribution: ericclaeren as a volunteer and at ezCompany commentedPublished it: https://www.drupal.org/project/facets_prefix_suffix
Comment #34
borisson_Implemented the readme changes, like I said in #31.
Comment #35
borisson_Comment #37
borisson_