Show thumbnails in result lists

[mspalti]

Fixes #1744
Fixes #1745

Description

Introduces a new Angular configuration option to display Item thumbnail images in browse and search result lists.

Instructions for Reviewers

If the new configuration option is set to be true you should see thumbnail images in browse and search results. (The default setting is false). To test, add the following to your config.dev.yml

showItemThumbnails: true

When true, the new configuration option tells the application to embed thumbnail info in the item reponse. This step was necessary for browse requests; thumbnail info is already included in search responses so there's no change in that case.

The new configuration option also tells Angular when to show the thumbnail in the list view.

The effect is global. I looked at making a themeable version of ItemSearchResultListElementComponent since that might make it possible to add the thumbnail per individual collection. But I came to the (perhaps mistaken) conclusion that this component is not themeable.

List of changes in this PR:

• Added the new configuration option
• Initial list of "recent submissions" now requests the embedded thumbnail info regardless of configuration
• The browse components and browse service request embedded thumbnail only if so configured
• ItemSearchResultListElementComponent shows thumbnail in item list if so configured

Sample:

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

• My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
• My PR passes TSLint validation using yarn run lint
• My PR doesn't introduce circular dependencies
• My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
• My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
• If my PR includes new, third-party dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.

[lgtm-com]

This pull request introduces 1 alert when merging 78e0769 into ca4f2cc - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[tdonohue]

@mspalti : Thanks for this improvement! Overall, the basics here work, but I did run into a few bugs and I have a few suggestions to improve the code.

First, the bugs...

1. When thumbnails are enabled, they mess up the alignment of some Entities in those same results list. Here's an example from Browse By Title of the alignment when you have both Publications and Research Projects in the same list:
   
   I think there's two possible solutions here. Either display "No Thumbnail Available" for all entity types, or just make sure there's an empty column placeholder so that the titles align properly.
2. Another bug I noticed is in the display of the "No Thumbnail Available" image in search results. The text wraps very oddly. Also this screenshot shows an example of how Collections matching a search also get misaligned with Publications/Items in the same list.
   

As for the code itself, it looks good overall, but I did notice a few things that could use cleanup:

• There's an odd src/app/collection-page.tar.gz file added by this PR, likely by accident. It should be removed.
• I noticed there's no specs/tests for these changes. Is there some way to at least provide some basic specs to verify the thumbnails appear when enabled & disappear when not enabled? If it's not easy to do in basic specs, maybe we think about enhancing e2e tests for this PR?
• Finally, I have some suggestions on how to rename/refactor the configuration here (see inline below).

Overall though, I'm very glad to see this work! I don't think any of these requested changes should require massive code refactors (the largest request is likely the request for some specs).

[tdonohue]

I'd recommend we consider renaming this to align better with other configs. Arguably, this setting only impacts two sections of the UI -- browse by & search.

So, I'd propose we consider moving this under the existing browseBy configs:

browseBy:
  ...
  showItemThumbnails: true

We could either document that it's also used by Search results... or we could implement a second config which is specific to search results (as I suspect we'll end up having more search configs anyways).

search:
  showItemThumbnails: true

In any case, my point is this isn't really a global configuration, so we should likely rename it to better describe the purpose.

I'd also recommend we enable it by default (as implied above) as I suspect most people will want thumbnails shown.

[mspalti]

I'm looking into the "no thumbnails" text problem. This should ultimately be an easy fix but I'm trying to decide on the best approach.

The problem is a little tricky because we have 3 different layouts for Item, search and browse views. Ideally text should just resize with the width of the thumbnail flex container but we may instead need to tap into the Angular component lifecycle and set the font-size after the layout is rendered...but I'd really prefer a pure css solution. (An svg image won't work because we do translations.)

[mspalti]

In terms of config, to me it makes sense to have both search and browseBy configurations.

[mspalti]

@tdonohue , I think all problems you found are fixed.

For the "no thumbnail" font issue, I tried several things. I think the best approach is to add global classes for image placeholder fonts and then set the font-size based on the size of the parent container. The sizes I've chosen seem to work ok for me.

For communities and collections in the search result list I'm adding an offset so they align correctly with other items.

For entities, I'm adding a thumbnail or using the default svg file when no thumbnail is available for the item.

I also added some specs to be sure thumbnails are added/removed, etc.

[mspalti]

Oh, regarding config files I decided to add the thumbnail setting to the browseBy configuration and added a comment, per your first suggestion, @tdonohue . Two separate configuration locations for the behavior just seemed awkward to me on second look.

[mspalti]

OK. This should be ready for another review.

[lgtm-com]

This pull request introduces 3 alerts when merging b692fa5 into e7dc5f8 - view on LGTM.com

new alerts:

• 3 for Unused variable, import, function or class

[mspalti]

I noticed that subject and author browses were failing because the thumbnail embed was added to metadata browse requests. This is fixed.

(BTW I noticed a small unrelated problem. When the user chooses to move between a metadata (author, subject) browse and an item (title, date) browse option an errant request is sent to the server, resulting in a 500 error. This doesn't break the UI. I don't see this already noted in an issue.)

Looks like e2e test failed with the latest commit. I suspect that's an unrelated problem.

[ybnd]

@mspalti About ds-dynamic-form-control-container: IIRC that would be for the list of related Items in the submission form itself, i.e. after you add relationships via the modal.

They're represented in a very compact way there, so we shouldn't add thumbnails IMO (or at least not until we can configure them separately from e.g. search/browse)

[mspalti]

Thanks for the screenshot. I agree that thumbnails aren't required.

[tdonohue]

@mspalti : Apologies, but this seems to have merge conflicts again. If you get a chance, could you rebase it on main? I think the minor conflicts were caused by merging #1771 (as this PR added a config to the same browseBy section in the configuration), but they should be easy to clean up.

[mspalti]

@tdonohue the merge conflict is fixed. Looks like the e2e failed for 16.x. I assume we just need to run the test again.

[tdonohue]

@mspalti : Yes, that looks like one of those semi-random e2e test failures. I just restarted the tests on Node 16.

[ybnd]

Looked everyhing over again, I'm +1 once we discuss/address one more issue:

Font scaling on Relationships on Item pages is still bugged for some "tablet" widths.

• Scaling the font down enough to fit would make the placeholder unreadable.
• Turning off thumbnails only here would require yet another Context & set of listable objects -- not great IMO.
• We could consider using SVG placeholders for Publication/Issue/Series/Journal entities (those are the only ones that could appear in this view in default DSpace)

[mspalti]

@ybnd , I'm not seeing the font issue that you encountered.

The font sizes aren't truly responsive (because they're not based entirely on css media queries and the viewport size) so perhaps you need to reload the page? For example, if I view the page using a Chrome DevTools phone layout then switch to the iPad layout I see something similar. Reloading corrects the font size. It's true that the font is small. But I think it's readable.

BTW, we might be able to make the font size more responsive by using ResizeObserver (or a polyfill) with the ElementRef. I'm not sure our problem merits that level of responsiveness, but it looks feasible if we do. Browser compatibility: https://developer.mozilla.org/en-US/docs/Web/API/ResizeObserver#browser_compatibility

I'll play around a bit with the columns on that page. It may be possible to increase the size of the thumbnail column. I think that would be better, even in the case of actual thumbnail images.

All of that said, svg images would be a great improvement! If others agree, it would be simple to add them here if svg's are available.

[mspalti]

@ybnd we could make the images and font a bit larger and more readable if the left column on the page is col-md-3 (rather than col-md-4) and the right column col-md-8. This basically aligns things with the Person search result columns below when they are present.

I assume that there was a reason for the column widths on this and other entity type pages so I'm not inclined to make this change unless others agree. If there's too much to consider, I suppose we could keep the current column layout and discuss on a separate issue if we want.

[tdonohue]

👍 Thanks @mspalti ! The code looks good and my prior feedback has all been addressed.

I tried to reproduce the font issue that @ybnd reported above. I can briefly see it (it appears to exist in the server-side rendering of the page?) and then it disappears a moment later & I see the same thing as @mspalti (where the font does scale down properly to really tiny, but readable).

It's most easy to reproduce by clicking reload (in your browser) on an OrgUnit page (or similar). When the page initially loads, the "No Thumbnail Available" text is overly large (like @ybnd 's screenshot), but a moment later it resizes (like @mspalti 's screenshot). It almost seems like the server-side rendering starts with larger text, and the resize occurs once things switch to client-side rendering. At least this is the behavior for me on both Chrome & Firefox. Because it resizes itself appropriately, I don't think this is something that should hold up this PR.... though maybe we can improve it in the future in a later PR.

So, all in all, I'm +1 getting this into 7.4.

[mspalti]

@tdonohue nice catch. I should have thought to test in production before calling it good! The element reference on the sever side doesn't have a width so the default (large) font was being chosen.

I added platform detection to the components. In the server context the font is hidden and appears correctly formatted on browser execution.

[ybnd]

@tdonohue @mspalti Now I'm not seeing the font issue either (and it wasn't only SSR last time - maybe I wasn't checking the latest version of the branch?)
+1

[tdonohue]

👍 Gave this one last test again today & the SSR issue from yesterday is fixed! So, this is all working perfectly now. Thanks again @mspalti for your hard work on this!