Add "between" location for recommendation modules #4806
Conversation
|
As a first-cut prototype, this is an example of an EDS results screen that includes a Between recommendation module for ProQuest results. Because ProQuestFSG provides almost no facet options, using a Blender instead would force hard choices on how to deal with EDS facets on the joint results screen -- choices that Blender supports well(!) but that may create a confusing UX in extreme cases like this. 
|
| $placements[0] = max( | ||
| $this->minPlacement, | ||
| $this->getMaxScoreDiffIndex($results) | ||
| ); |
There was a problem hiding this comment.
So as a start, I'm saying the recommendation module should appear wherever there is the largest relevancy score gap, except that it should never appear before the first minPlacement results. I'm also ignoring any recommendation modules after the first, because I haven't thought of a use case for that yet, or where to place them, and there might be UX issues displaying them consecutively.
There was a problem hiding this comment.
I can definitely imagine there being multiple between recommendations -- we can probably think of more things that might be useful/interesting in the result set than there is room to display them. I wonder if a safer default, until we can figure out a better approach, would be to push them to the end of the result set, rather than putting the first one in a good spot, and any subsequent ones at the top.
There was a problem hiding this comment.
and any subsequent ones at the top.
If I can still understand my own code 😄 , it's defaulting any subsequent recommendations to 'false' placement which effectively just hides them.
I could though take your suggestion to place them at the end of the list by default. From UX, I suppose that is better than hiding them entirely. But from a site admin perspective, you probably did something wrong if you enabled multiple 'betweens' without a real algorithm for where to display them, and by placing them on the bottom we are making it hard for the admin to notice that and fix it. And in that case, the default styling also may still be poor. So I think I lean to hiding it, but I'm open to doing otherwise.
I think once we come up with more examples of a) modules you'd want to place 'between', and b) logic for where to place them, the reasonable default will become clearer.
There was a problem hiding this comment.
Ahh, I didn't realize that hiding with false was an option, and I misunderstood your array_fill. I don't mind hiding them for now until we have a better idea of the algorithm. And we both agree that putting them at the top would have been wrong, if that had been what was actually happening. :-)
There was a problem hiding this comment.
I've improved the documentation to be clear what false does there!
Still going to leave this thread open for now, as there is probably a better default...
|
|
||
| /* ------ BETWEEN RECOMMENDATIONS ------ */ | ||
| .betweenRecommendationItem { | ||
| margin: 0 2rem; |
There was a problem hiding this comment.
I have no idea what the default styling should be here, and I expect any institution that will actually use a between recommendation will style it in some context-specific way. This is just an initial hack to keep it from looking wider than the normal results, and so we can decide on the class name.
|
This is ready for a conceptual review, i.e. does the idea and general approach make sense, before I tackle the TODOs. |
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @maccabeelevine -- I haven't tried this hands-on yet (though I'm sure @sturkel89 is open to that kind of testing if and when you're ready). See below for some thoughts/suggestions based on a read of the code.
| $listStart = $this->results->getStartRecord() + $i - $this->indexStart; | ||
| $showCheckboxes = $this->searchSettings($this->results->getParams())->checkboxesEnabled(); | ||
| $between = $this->results->getRecommendations('between'); | ||
| $betweenPlacements = $this->between()->getPlacements($between, $this->results); |
There was a problem hiding this comment.
As an alternative to creating a full between helper, I wonder if it's worth considering adding a public getBetweenPlacement method to the existing recommend helper. This could be achieved by refactoring the current __invoke method to return the whole object if $recommend is null and adding additional public methods.
(I'm also open to keeping the new helper if that's preferable -- especially if you expect it to get more complicated... but it seems like adding to the existing helper might have better semantics).
There was a problem hiding this comment.
Yeah, I'm torn on this. I do think the between helper will get more complicated, as we come up with additional placement algorithms and then have to introduce some config to manage them. I think we'll likely have algorithms that are simpler -- i.e. always place the betweens in the same spots(s) -- so as to be more predictable UX and potentially more visible. And I can also imagine more complex algorithms that actually look at the recommendation results themselves (i.e. their scores, or maybe lexical matches of the top result(s) with the query params) to help determine placement. So I can see all that overwhelming the current Recommend helper, even though in practice 'between' use will probably be rare compared with the legacy methods. That said, I don't mind combining them if you prefer.
There was a problem hiding this comment.
I can think of a couple of possible approaches to this:
1.) We could move the logic into a generic helper service that is not a view helper -- just a "BetweenHelper" -- and make this service accessible through the 'recommend' view helper. If the logic is never going to be entangled with view-specific details, this might be a little more elegant (and easier to write tests for), and it still keeps things granular and overrideable.
2.) We could leave things as they are with the two view helpers -- but in that case, maybe we should consider renaming between to betweenRecommend. The between name feels too ambiguous for my liking, which is the main reason I suggested moving it inside recommend to give it context. But just changing the name might be the simplest solution.
| $placements[0] = max( | ||
| $this->minPlacement, | ||
| $this->getMaxScoreDiffIndex($results) | ||
| ); |
There was a problem hiding this comment.
I can definitely imagine there being multiple between recommendations -- we can probably think of more things that might be useful/interesting in the result set than there is room to display them. I wonder if a safer default, until we can figure out a better approach, would be to push them to the end of the result set, rather than putting the first one in a good spot, and any subsequent ones at the top.
Co-authored-by: Demian Katz <[email protected]>
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @maccabeelevine, see below for a couple more comments:
| $listStart = $this->results->getStartRecord() + $i - $this->indexStart; | ||
| $showCheckboxes = $this->searchSettings($this->results->getParams())->checkboxesEnabled(); | ||
| $between = $this->results->getRecommendations('between'); | ||
| $betweenPlacements = $this->between()->getPlacements($between, $this->results); |
There was a problem hiding this comment.
I can think of a couple of possible approaches to this:
1.) We could move the logic into a generic helper service that is not a view helper -- just a "BetweenHelper" -- and make this service accessible through the 'recommend' view helper. If the logic is never going to be entangled with view-specific details, this might be a little more elegant (and easier to write tests for), and it still keeps things granular and overrideable.
2.) We could leave things as they are with the two view helpers -- but in that case, maybe we should consider renaming between to betweenRecommend. The between name feels too ambiguous for my liking, which is the main reason I suggested moving it inside recommend to give it context. But just changing the name might be the simplest solution.
| * @license http://opensource.org/licenses/gpl-2.0.php GNU General Public License | ||
| * @link https://vufind.org/wiki/development Wiki | ||
| */ | ||
| class RecommendBetween extends \Laminas\View\Helper\AbstractHelper |
There was a problem hiding this comment.
@demiankatz You had commented
I can think of a couple of possible approaches to this:
1.) We could move the logic into a generic helper service that is not a view helper -- just a "BetweenHelper" -- and make this service accessible through the 'recommend' view helper. If the logic is never going to be entangled with view-specific details, this might be a little more elegant (and easier to write tests for), and it still keeps things granular and overrideable.
2.) We could leave things as they are with the two view helpers -- but in that case, maybe we should consider renaming between to betweenRecommend. The between name feels too ambiguous for my liking, which is the main reason I suggested moving it inside recommend to give it context. But just changing the name might be the simplest solution.
As a start I've renamed it as you suggest. I don't mind doing the larger change though if you think it's a good idea. Where would you put the non-view helper?
There was a problem hiding this comment.
I really don't have strong feelings about this, but I think the best approach (and the answer to your question about where to put the helper) depends on the scope of use for the class. If we're only ever going to use this logic in the context of a view helper, then maybe we don't need it -- and if we do need it, maybe it could live in a sub-namespace of the view helpers, following the precedent of the RecordDataFormatter SpecBuilder helper.
If the logic might need to be reused in multiple contexts -- e.g. if we anticipate moving logic to the controller side of things -- then maybe creating a \VuFind\Recommend\Helper namespace would make some sense.
There was a problem hiding this comment.
If the logic might need to be reused in multiple contexts -- e.g. if we anticipate moving logic to the controller side of things -- then maybe creating a \VuFind\Recommend\Helper namespace would make some sense.
Done. I think it's at least possible that we'll want to do some of this calculation into the controller, to make use of some other data.
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks for the adjustments -- see below for a couple of thoughts that may not be immediately necessary but might be worth considering. :-)
| */ | ||
| public function getBetweenHelper() | ||
| { | ||
| return new Between(); |
There was a problem hiding this comment.
We should probably inject this as a dependency and define it as a top-level service, so it's easier to override. Though if you don't want to do that right away, this is certainly fine for development/testing purposes -- we should just add a TODO so we don't lose track of it. Depending on what happens with #4801, it may be less labor-intensive to do this at some point in the not-too-distant future.
There was a problem hiding this comment.
Ok, I wondered about that, but was following the pattern of SpecBuilder with direct instantiation. I can look at making this change though.
There was a problem hiding this comment.
To construct the Between helper within the Recommend view helper, I think I would have to create a RecommendFactory so that it can use the ContainerInterface to construct a Between and pass that into the Recommend constructor. Which would further complicate that constructor, adding a fourth param. Or I guess I could just use the existing constructor and add a setter function on Recommend to pass in the Between helper. Is that what you would suggest?
There was a problem hiding this comment.
That's true, I forgot that Recommend was currently simple enough to use InvokableFactory. The current Recommend class doesn't have a constructor, though, so I don't think you need to worry about complications there (I think you're looking at __invoke, which is a whole different thing). Should be simple enough to create a factory and add a constructor here.
There was a problem hiding this comment.
I think you're looking at __invoke, which is a whole different thing
Thanks, yes exactly. I'll work on this tomorrow.
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Catching up on older reviews, and I saw this hadn't moved in a while. Looks pretty reasonable from my perspective, though there are some style conflicts that need to be resolved, and I made some suggestions below about adding return types, because why not? :-)
Do you mind resolving conflicts when time permits? After that, is there anything you'd like to specifically discuss to move things forward further?
module/VuFind/tests/unit-tests/src/VuFindTest/Recommend/Helper/BetweenTest.php
Outdated
Show resolved
Hide resolved
module/VuFind/tests/unit-tests/src/VuFindTest/Recommend/Helper/BetweenTest.php
Outdated
Show resolved
Hide resolved
Co-authored-by: Demian Katz <[email protected]>
|
I fixed the merge and accepted the typing suggestions. This will take more discussion locally though (and with others as well if anyone is interested) before I'll be ready to move it forward. Still working on exactly what the placement should be -- fixed or floating by primary results scores, and possibly even taking into account the score(s) of the between recommendation results if that is relevant. Input by the new a11y committee would also be valuable, how to do this in a way that's not confusing to screen readers etc. |
2 participants
Allow a recommendation module, such an AbstractSearchObject subclass, to be displayed among the search results -- in between two standard results. This is a capability of some modern search engines, Google Web Search being the most obvious example, where the list of web results may be "interrupted" by a block of videos, or Q&As, etc.
The specific placement (which two results to appear between) can be dynamic based on any relevant factors we have available, such as the relevancy score of each search result, or of the recommendation module's results. As a simple initial implementation, it can place the recommendation module between the two results with the largest relevancy score gap.
This offers a UX middle-ground between existing recommendation locations and Blender.
#1result, which is likely the most useful item on the results screen. That's bad UX unless the top recommendation really should be viewed first, like "did you mean" logic. It probably doesn't apply to other AbstractSearchObject results.To do: