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

Books without cover art are not shown + books outside "library" are not shown by default #1851

Merged
merged 1 commit into from
Sep 27, 2024

Conversation

WakMun
Copy link
Contributor

@WakMun WakMun commented Sep 23, 2024

Description

  1. By default, library_browse.php statically filters out all books which are not located in the location called "Library". Its not based on country or language. If the locations name is not exactly "Library" (eg Bibliothek in German), the books will get filtered out by default unless the user is attentive enough to not miss the check-box "include books outside of Library". But in that case all the books are included and not only "Library's". My proposal for fix here is to have the check box checked by default, and the user can actively deselect it if too many unwanted results are shown. The confusion of having too many results seems better than the confusion of having no results at all.

  2. The template librarySearch.twig.html silently hides books from the search results if they don't have cover art associated in the database. I think the easiest fix here would be to have a default image shown, eg, that states "image not available".

  3. Abother minor and unrelated issue to the original post here but in the same file (library_browse.php) so I mention it here: Normally when a user performs a search in (or outside) library, the search criteria are displayed at the top of the results—except the criteria of readers age. I guess the feature to search on basis of readers age was added not too long ago, so this part got missed. This also seems like a straightforward fix.

Motivation and Context
https://ask.gibbonedu.org/t/library-browser-and-search-stop-working-after-update-to-v27/9083/5

How Has This Been Tested?
on a docker based testbench

Screenshots

image

Copy link
Member

@SKuipers SKuipers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR and bug fix, much appreciated! I wonder about using a local copy of an image not found file rather than a remote one, that way a page with a 100 books without covers isn't making 100 extra api calls to get blank images. Otherwise, looks great and I'd be happy to merge this.

@WakMun
Copy link
Contributor Author

WakMun commented Sep 26, 2024

You’re right. In fact an even better approach, rather than substituting the images, would be to update the Twig template to display the title and producer on the tile instead of showing blank "not found" images. This would not only reduce unnecessary API calls but also provide more meaningful results for the user. Here’s a screenshot:

image

If you like this better, then please close this PR, and I will create a new one with updated twig template.

@SKuipers
Copy link
Member

Yes, great thinking! I love this approach and it will work well for non-book things that some libraries put in their system.

@SKuipers
Copy link
Member

You are welcome to add to this branch to update the PR, rather than having to create a whole new PR.

@WakMun
Copy link
Contributor Author

WakMun commented Sep 26, 2024

Done.

Copy link
Member

@SKuipers SKuipers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @WakMun thank you for making these changes, I'm happy to merge them into the core. I think I'll add a setting for the location toggle, as I know some schools only want to show books on their browse page and not other items like IT equipment, etc.

Much appreciated!

@SKuipers SKuipers merged commit 89f4a61 into GibbonEdu:v28.0.00 Sep 27, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants