Add support for "new items" facet.#4996
Conversation
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks for getting this started, @EreMaijala. I haven't had a chance to do any hands-on testing or to review everything carefully, but see below for a few minor things.
I'll comment further when I have a bit more time!
| * @param string|object|array $target String to translate or an array of text | ||
| * domain and string to translate | ||
| * @param array $tokens Tokens to inject into the translated | ||
| * string | ||
| * @param string $default Default value to use if no translation is | ||
| * found (null for no default). |
There was a problem hiding this comment.
Minor formatting suggestions:
| * @param string|object|array $target String to translate or an array of text | |
| * domain and string to translate | |
| * @param array $tokens Tokens to inject into the translated | |
| * string | |
| * @param string $default Default value to use if no translation is | |
| * found (null for no default). | |
| * @param string|object|array $target String to translate or an array of text | |
| * domain and string to translate | |
| * @param array $tokens Tokens to inject into the translated string | |
| * @param string $default Default value to use if no translation is | |
| * found (null for no default). |
| * | ||
| * @return string | ||
| */ | ||
| public function replaceFacet($field, $value, $operator = 'AND') |
There was a problem hiding this comment.
Might as well add types to new methods...
| public function replaceFacet($field, $value, $operator = 'AND') | |
| public function replaceFacet(string $field, string $value, string $operator = 'AND'): static |
| * @param string $value Facet value | ||
| * @param string $operator Facet type to add (AND, OR, NOT) | ||
| * | ||
| * @return string |
There was a problem hiding this comment.
Pretty sure this isn't returning a string...
| * @return string | |
| * @return static |
| "version": "2.11.8", | ||
| "resolved": "https://registry.npmjs.org/@popperjs/core/-/core-2.11.8.tgz", | ||
| "integrity": "sha512-P1st0aksCrn9sGZhp8GMYwBnQsbvAWsZAX44oXNNvLHGqAOcoVxmjZiohstwQ7SqKnbR47akdNi+uleWD8+g6A==", | ||
| "peer": true, |
There was a problem hiding this comment.
This random change again!
| "peer": true, |
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
@EreMaijala, I spent a little more time reviewing this today (at long last). See below for further comments.
Also, some other notes:
1.) I pushed up commit f47fa71 which is a quick-and-dirty proof of concept of redirecting from the Solr-based specialty "new items results" to a standard filtered search. If we want to do things this way, more cleanup is needed, and some other things need consideration:
a.) The [NewItem] filter setting generates hidden filters, which might make sense when "New Items" is a separate result screen, but is only going to confuse people if we integrate it with regular search. Should we just translate these hidden filters into non-hidden ones for clarity?
b.) The new item facets are currently implemented as OR facets, but this means weird things happen in the search results, because you can't de-select them normally. This appears to be an existing problem, but we may want to deal with it one way or another.
2.) I wonder if the new item control should be more tolerant of non-standard values -- that is, if a filter comes in from some other place, should we display it in the facet list as a de-selectable value, or should we keep it invisible as present. (E.g. if I apply a new item filter but then change the URL to use a non-standard number of days, the filter shows up correctly in the filter list but is ignored in the facet list sidebar).
| * @param string $date Date | ||
| * @param string $domain Translation domain | ||
| * | ||
| * @return string |
There was a problem hiding this comment.
| * @return string | |
| * @return array |
| * | ||
| * @return string | ||
| */ | ||
| protected function formatNewItemsDateForDisplay($date, $domain) |
There was a problem hiding this comment.
| protected function formatNewItemsDateForDisplay($date, $domain) | |
| protected function formatNewItemsDateForDisplay(string $date, string $domain): array |
| } | ||
|
|
||
| /** | ||
| * Format a Solr date for display |
There was a problem hiding this comment.
We should expand this comment to explain the return value... because at present, I don't entirely understand what the second Boolean value is doing.
| * | ||
| * @return array | ||
| */ | ||
| protected function formatNewItemsFilterListEntry($field, $value, $operator, $translate): array |
There was a problem hiding this comment.
| protected function formatNewItemsFilterListEntry($field, $value, $operator, $translate): array | |
| protected function formatNewItemsFilterListEntry( | |
| string $field, | |
| string $value, | |
| string $operator, | |
| bool $translate | |
| ): array |
| <li class="facet__list__item"> | ||
| <?php | ||
| $addURL = $results->getUrlQuery()->replaceFacet($facet, $newItemsFilter); | ||
| $removeURL = $results->getUrlQuery()->removeFacet($facet, $newItemsFilter); |
There was a problem hiding this comment.
Should we make these $addUrl and $removeUrl for consistency with $toggleUrl? Seems weird to have two different styles close together, and I think the Url pattern is more common in the code at this point.
|
I'm not sure why the Dependency Review action is failing; I suspect it's an intermittent issue that will go away if we try later. |
|
@EreMaijala, I think this one may have slipped through the cracks. I also see there are now some conflicts that need to be resolved. Please let me know if I can be of any help! |
2 participants
TODO: