-
Notifications
You must be signed in to change notification settings - Fork 359
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Configurable views for EDS #4199
Configurable views for EDS #4199
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question for you: if I run this with an outdated configuration file (default_view = brief
), I get:
Laminas\View\Renderer\PhpRenderer::render: Unable to render template "search/list-brief.phtml"; resolver could not resolve to a file
Do you think there might be a way to handle this scenario more gracefully, since I rather expect it is likely to happen.
Yes, I fixed the problem. Now, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @ThoWagen -- looking at the revisions inspired another thought (and also reminded me how confusing some of this logic is).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @ThoWagen -- just a few more suggestions related to comments and documentation in an effort to make this easier to understand in future. Please let me know what you think, and whether you need me to test/confirm anything on my end. Once any appropriate changes are in place, I'll do one more round of local testing, and then I suspect this will be ready to merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @ThoWagen, this is looking good and working for me. I went ahead and added a unit test for the config upgrade piece. I just noticed one small issue: all three view options are always links. In the dev branch, the active view is not a link. Maybe there's something wrong with my configuration somewhere, but I'm curious whether you're seeing the same thing on your end.
…hoWagen/vufind into pull-request/configurable-view-for-eds
@demiankatz Good catch! I fixed it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @ThoWagen, this looks good to me now, but I've asked @sturkel89 to do another round of hands-on testing just in case I missed anything. I'll wait to hear from her before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR provides a nice new functionality for EDS. Everything seems to work well! I think it's ready to merge.
More detail:
I tried reordering and renaming the options, and everything worked as expected. I like my new phrasing and order better than the default:
Brief View | Include abstracts | Titles only
Side note: I don't think the Titles only view is very useful in practice, as it does not show the publication information or date for the item. But that is not an issue for this PR!
Here are examples of the same search results in each view.
list_brief ("Brief view"):
list_detailed ("Include abstracts"):
list_title ("Titles only"):
Side note
While testing this, I noticed a problem of EDS functionality that is outside the scope of this PR: when the user selects a preferred view for EDS results (e.g., title view) that selection is lost and results go back to the default when another search is run. This is NOT how things work in the main VuFind window, where if a user prefers the grid view instead of list, then performs another search, the grid view is maintained as the new default view. I have opened JIRA ticket VUFIND-1744 to capture this issue.
This PR enables configuring the available views for EDS.