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!

• [x] 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.
• [x] My PR passes TSLint validation using yarn run lint
• [x] 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.
• [x] 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[bot]]

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

new alerts:

• 1 for Unused variable, import, function or class

[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[bot]]

This pull request introduces 3 alerts when merging b692fa5924e01fa9b52f38f8f96e62afddd45117 into e7dc5f8d149e5dbbc0dee4f121596f9fbc104cb2 - view on LGTM.com

new alerts:

• 3 for Unused variable, import, function or class

[mspalti]

I added thumbnail previews to mydspace items. Everything looks correct to me in what I've been able to preview thus far. A few features are things I haven't used yet and I wasn't able to preview them today (e.g. pooled tasks) so be on the lookout for issues there.

Also, components and tests were modified to inject the environment. So now this is indeed ready for review (assuming tests pass).

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 989d243c8ee00035ae19e558b4b7f2b12ebe9105 into e7dc5f8d149e5dbbc0dee4f121596f9fbc104cb2 - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 7f9c34f08e3cf1188599de036d0f76d3899ffb7f into e7dc5f8d149e5dbbc0dee4f121596f9fbc104cb2 - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[mspalti]

@ybnd , about the detailed view, the code suggests that detailed views were designed only for MyDSpace search results. The object-detail components are all MyDspace previews.

There are a few layout issues with the MyDspace detailed view but that's out of scope for this PR since detailed previews always add ds-thumbnail when the object has an embedded thumbnail. This PR has no effect on that behavior. Here's a screenshot of the MyDspace view.

When you apply view=detailed to regular search lists the result is strange. The parent container ds-object-detail is used and the child components in the list are rendered in small columns. The layout is broken even without thumbnails. Here's a screenshot of search results without thumbnail images.

It looks like the detailed layout was not intended to be used here...or maybe it's just unfinished. Either way I don't think this is something we need to worry about now. Feel free to correct me.

[mspalti]

OK. The mobile layout is now working as you suggested @ybnd . Thanks for that suggestion. I agree it's an improvement.

I need to call out one additional change that I just made. It's bothered me that the "no thumbnail" placeholder max-width is 175px. That seems ok in the Item view, even though it is larger than the thumbnails that dspace generates. But it's vexing in the list view because the thumbnails and svgs don't align well with the larger placeholder. I've set the --ds-thumbnail-max-width to what I think is a better size: 125px.

The thumbnail max-width has bothered me for some time ... but this change is only a suggestion. We could also set the max-width for result lists separately from --ds-thumbnail-max-width.

Here's how things align now in the browse view, using the 125px max-width:

[lgtm-com[bot]]

This pull request introduces 3 alerts when merging 78358a4067fd63e6dd835febc729c55939b41f56 into 6d361beb8812fa6c26e7c957ee2f4990e6612d6b - view on LGTM.com

new alerts:

• 3 for Unused variable, import, function or class

[mspalti]

@ybnd , when I throttle to mid-tier mobile in Chome I can see the choppy rendering that you observed. Now that I see it I could work on it or apply a suggested fix. But it would also be great if you would want to tackle this with #1694 !

[ybnd]

Thanks for addressing everything!

If @tdonohue agrees I'll tackle the animation issue in #1694 (direct involvement because it makes changes in that component already & will make loading take longer in some cases)

I did come accross one new problem though:

• Thumbnails are also shown in relationship metadata representations, but are a bit broken
• If we keep them there we should fix the font scaling and make sure they don't touch when the entries are short
• For some reason the Entity placeholder SVGs aren't loaded and it falls back to the alt text

[mspalti]

@ybnd regarding the relationships metadata, is there an example of this on the demo site? If so, I'll point my local angular to that for testing (I haven't actually worked with relationships yet and don't have an example locally).

[ybnd]

The Item from that screenshot was from the demo site: https://demo7.dspace.org/entities/publication/5bb46321-21c6-44ce-b388-945500c87aba

[tdonohue]

@mspalti : Just a quick note...in case you hadn't seen it yet, this PR looks to be failing tests cause of lint issues in some of your spec.ts files. It looks like you forgot a bunch of semi-colons at the end of lines. You should be able to see these same lint errors locally by running yarn lint

[mspalti]

The layout for related entity thumbnails is working but the thumbnail is still missing from items so default svg and existing thumbnails aren't appearing.

That seems to be caused by how the HALLinks for left and right Items are resolved. We don't get a thumbnail in the Item response so we only see the default thumbnail text (see the view below). If we need a partial solution while we're looking into this we could just add the default svg in ds-thumbnail whenever the item thumbnail is missing and the defaultImage is provided.

Here's the layout as it is now (with no default svg images):

[ybnd]

Already a big improvement, but with the extra small thumbnails the placeholder text breaks down again on viewports under 1200px wide

~~I tried messing around a bit in RelationshipService & couldn't get thumbnail links to resolve from there (nesting followLinks doesn't seem to work in this case). We could add the thumbnails within ds-related-items here as a workaround: Checked locally on this branch & this resolves the missing SVG fallback images~~

I wasn't following followLink's signature properly: we can fix this with followLink(..., {}, followLink('thumbnail')) here

This should only be done if thumbnails are enabled though, like you had in BrowseService. Because relationships are used in more contexts I would add an optional embedThumbnails to the method itself & call it with true here

[mspalti]

Thanks, @ybnd . I found the followLink signature this morning and then read your comment more carefully. Good solution. There are a few other methods in relationship.service that follow item links but they don't appear to require thumbnails.

Also, I'm glad to hear that it's a good idea to apply the showThumbnail configuration to services. It seemed like a good idea but I wasn't sure what others thought. I'll add the configuration here.

[tdonohue]

@mspalti : After merging #1791 (which is a larger, but important refactor), this now has some small merge conflicts. I'd appreciate it if you could rebase this again when you get the chance.

[mspalti]

There was quite a lot of overlap with #1791 so I needed to merge instead of rebase and fix a few tests after the merge. Not sure why that was needed but all looks good now.

So this is ready for review again.

I tested layouts using the device dimensions provided by Chrome tools and, so far as I can tell, everything looks good or at least acceptable. The text inside the thumbnail placeholder is always going to be tricky given the many permutations (viewport size, parent container size, translated text, etc.) but I mostly like what I'm seeing. I think I managed to look at all possible DSpace layouts, but do check me on that and let me know if we need more layout tweaks.

[mspalti]

I added configurable thumbnails for ds-item-relationships (that's one mentioned by @ybnd that I hadn't seen before!) and verified that 'add relationship' modals are working.

The only other code location I see that might be relevant here is ds-dynamic-form-control-container. I don't know where to see this component in action but it looks like it's used with submissions. If it displays thumbnails then it needs the same approach used in ds-item-relationships. Just point me to where I can see the component in action if it needs work.

[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.