Back to results button in item view

[mspalti]

Description

Adds a "back to results" button to the Item view when the previous url in the router history is a search, browse or recent submissions list.

Instructions for Reviewers

A button appears in the item view when you navigate from any of the lists mentioned above. Use it to navigate back to the previous page in the pagination list.

The back button is added when the previous url in history begins with /search, /browse, or /collections (see item.component.ts).

Currently, the only way to navigate back to a page in the pagination list is via the browser back button. That's a usability problem. The solution here is fairly simple. I spent time looking for an edge case where the approach doesn't work, but I couldn't find one. Logically that makes sense since the back option uses the Router history, but let me know if an edge case exists.

List of changes in this PR:

• Button added to all item html templates
• Button is shown when routeService.getPreviousUrl() returns a matching route
• Clicking on the button uses Router.navigateByUrl() to navigate back to the previous pagination list.

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 ESLint validation using yarn lint
• My PR doesn't introduce circular dependencies (verified via yarn check-circ-deps)
• 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 libraries/dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
• If my PR includes new features or configurations, I've provided basic technical documentation in the PR itself.
• If my PR fixes an issue ticket, I've linked them together.

[mspalti]

I refactored the back button to be a shared component and made it theme-able while I was at it.

[tdonohue]

@mspalti : Great work! I didn't realize this would be so easy, and I agree that it helps with usability. Overall, I'm basically a +1 vote, but I have a few minor requests.

1. We need to avoid adding code to the SharedModule unless absolutely necessary, as we've found the SharedModule has become bloated & we need to shrink it (see #1975 and #1921). I think this new code can easily move to ItemPageModule though (haven't tried it, but it looks like it should cleanly move over)
2. I noticed that this back button does NOT yet appear in the following places. If possible, it might be nice to enable it in these two situations as well:
   • If you come from the Admin Search results (Login as Admin -> "Admin Search" -> click on a result).
   • If you come from the MyDSpace list of Submissions or Workflow Tasks (Login -> MyDSpace -> Show="Your Submissions" OR "Workflow Tasks" -> Click "View" button)

Overall, I like this PR & I don't see any immediate issues myself. I would appreciate it though if @artlowel took a quick look just to make sure he's not seeing any issues with this implementation of a back button.

[tdonohue]

Rather than adding these to the "shared" module, could we add them to the item-page.module.ts instead? It appears they are specific to the ItemPageModule (which is also used by all Entity pages as well), so they don't need to be in the "SharedModule"

[mspalti]

Sure, I can look into this more.

We may be able to use the item-shared-module.ts. That module is imported by the entity-group modules. Also, I just added another wrinkle here. I decided that because the button is now a themed component it makes sense use it for the browse-by component back buttons. If @artlowel agrees with this approach then we would need to add the ItemSharedModule to browse-by.module.ts as well.

[mspalti]

The solution I came up with was more complex than I guessed. Because the button is also used for browse-by components I decided to make separate modules for both the results back button and the shared browse-by component. That removes both from the "shared" module.

I know that the Atmire team is working on shrinking the size of shared.module and the approach taken here might not align well with what they are thinking. If so, that's fine. I can fix things here if needed.

The back buttons are working now for Admin Search and MyDSpace.

[mspalti]

@artlowel, @tdonohue, I decided to use the themed button component with shared/browse-by/browse-by.component so there's consistent theming across contexts. Basically, if pagination Input()'s are provided in the back button component the PaginationService is used for back navigation. Otherwise we just use the router.

If this turns out to be a bad idea I can revert back to a separate button for browse-by.

[github-actions]

Hi @mspalti,
Conflicts have been detected against the base branch.
Please resolve these conflicts as soon as you can. Thanks!

---

This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there.

[artlowel]

Thanks @mspalti!

It works, and I like the look and the positioning. I just have a few suggestions to reduce code duplication.

I appreciate that you've made it themeable out of the box!

[mspalti]

@tdonohue, @artlowel, sorry about the latest bug. I didn't notice it after the last code change.

When I moved the conditional logic from the item page components into the button component that caused the scenario you noticed. The browse component has it's own show/hide logic that's inconsistent with the item logic we moved into the button component. I decided the best way to handle this is to add a new alwaysShowButton input. This fixed the problem with the browse button and might be useful in other future situations. On second thought I really dislike this solution. So instead I returned the logic for showing/hiding the back button to ItemComponent and added *ngIf to the ds-themed-results-back-button. That avoids code duplication in the various item templates without introducing complexity elsewhere.

I also noticed a silly problem in the theme component that's now fixed. I think everything is working now. Check me on that. :-)

[mspalti]

I made one last change/improvement. The back button component shouldn't worry about setting up a back() function that works for different parent components. That's a little confusing and error-prone. It's bothered me for a while but I haven't had time to consider the options.

Here's the change: functionally everything's the same, but the back() method is now defined in the parents (browse-by.component and item.component respectively) and passed as an input to results-back-button.component. That simplifies the button component and helps avoid the kind of problem we ran into above, where the button was being accidentally hidden.

[mspalti]

I've seen a somewhat intermittent test error in my local environment that never seemed to manifest in PR tests. That changed today and I was able to track it down. It was caused by an unnecessary module import in one of the tests. The problem didn't affect builds and running the code.

[mspalti]

Just to wrap up. (I'm optimistically assuming this work is complete.)

The back button feature in this PR works right now with items and metadata browse lists. Although given the way it's implemented it could be used in other situations as well, perhaps navigating between entities types (if that's possible).

To use the themed button component, a parent component just needs to decide when to show the button, provide a label translation (or accept the default "back to results" label), and create a back() method that the button will use for navigation. The label and back() method are inputs to the button component.

[tdonohue]

👍 Thanks @mspalti ! I retested this today, and verified that the small bug that I saw (and @artlowel originally reported) is now fixed! I'm not finding any other immediately obvious issues, so I think this looks good (and if we find later issues we can fix them later).

I have one very minor comment inline. Nothing required, but a minor code cleanup that could make the path Regex you are using easier to update/find.

[artlowel]

Thanks @mspalti!

I've tested it again, and indeed the issue is gone. I also agree with the reasoning to move the showBackButton code back

[tdonohue]

Merging as this is now at +2. Thanks again @mspalti !