Skip to content
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

Merged

Conversation

ThoWagen
Copy link
Contributor

This PR enables configuring the available views for EDS.

Copy link
Member

@demiankatz demiankatz left a 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.

@ThoWagen
Copy link
Contributor Author

ThoWagen commented Jan 23, 2025

Yes, I fixed the problem. Now, getDefaultView returns list if defaultView consists only of one part.

Copy link
Member

@demiankatz demiankatz left a 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).

module/VuFind/src/VuFind/Search/EDS/Options.php Outdated Show resolved Hide resolved
Copy link
Member

@demiankatz demiankatz left a 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.

config/vufind/EDS.ini Outdated Show resolved Hide resolved
Copy link
Member

@demiankatz demiankatz left a 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.

@demiankatz
Copy link
Member

To clarify my problem, here's the dev branch:

image

...and here's this branch:

image

@ThoWagen
Copy link
Contributor Author

@demiankatz Good catch! I fixed it.

@demiankatz demiankatz added this to the 11.0 milestone Jan 27, 2025
@demiankatz demiankatz requested a review from sturkel89 January 27, 2025 15:28
Copy link
Member

@demiankatz demiankatz left a 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.

Copy link
Contributor

@sturkel89 sturkel89 left a 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"):

image

list_detailed ("Include abstracts"):

image

list_title ("Titles only"):

image


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.

@demiankatz demiankatz merged commit 6311aa5 into vufind-org:dev Jan 27, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants