Skip to content

Fix: Improve footer responsiveness and download button on the full item page#4218

Merged
tdonohue merged 10 commits intoDSpace:mainfrom
jesielviana:fix-footer-responsiveness
May 6, 2025
Merged

Fix: Improve footer responsiveness and download button on the full item page#4218
tdonohue merged 10 commits intoDSpace:mainfrom
jesielviana:fix-footer-responsiveness

Conversation

@jesielviana
Copy link
Copy Markdown
Contributor

References

Description

Improved footer responsiveness across all pages by adjusting the COAR Notify position on mobile, and enhanced the download link responsiveness on the full item page.

Instructions for Reviewers

  • Check the footer display on both desktop and mobile views (starting from 390px width and up)
  • Review the file download section on the full item page in both desktop and mobile views (starting from 390px width and up)

List of changes in this PR:

  • Removed the lead class from the "No Thumbnail Available" text, as it increased the font size to 1.25rem
  • Moved the COAR Notify link below the DSpace footer links on mobile devices
  • Styled the files download link on the full item page to display both icon and label ("download") on desktop, and icon only on mobile

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You do not need to complete this checklist prior creating your PR (draft PRs are always welcome).
However, reviewers may request that you complete any actions in this list if you have not done so. If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

  • My PR is created against the main branch of code (unless it is a backport or is fixing an issue specific to an older branch).
  • 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 npm run lint
  • My PR doesn't introduce circular dependencies (verified via npm run 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.
  • My PR aligns with Accessibility guidelines if it makes changes to the user interface.
  • My PR uses i18n (internationalization) keys instead of hardcoded English text, to allow for translations.
  • My PR includes details on how to test it. I've provided clear instructions to reviewers on how to successfully test this fix or feature.
  • 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.

@tdonohue tdonohue added bug usability 1 APPROVAL pull request only requires a single approval to merge labels Apr 18, 2025
@tdonohue tdonohue moved this to 🙋 Needs Reviewers Assigned in DSpace 9.0 Release Apr 18, 2025
@tdonohue tdonohue added ux User Experience related works testathon Reported by a tester during Community Testathon labels Apr 18, 2025
@tdonohue tdonohue added the port to dspace-8_x This PR needs to be ported to `dspace-8_x` branch for next bug-fix release label Apr 30, 2025
Copy link
Copy Markdown
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

@jesielviana : Thanks for this PR! I reviewed and tested it today. Overall, the entire PR looks good, and I've verified that it fixes the two main layout issues...the one with the footer at 390px, and the one on the "Full item page" with the download buttons at 390px.

I noticed though that it also changes the download file links to be buttons like this:
buttonlinks

I wanted to call that out simply because I didn't see it mentioned in the title or description of this PR. But, it looks like that change is intentional, and I don't mind the new look. I just had a minor note on it below.

Overall though, I'm a +1 on this PR. I may want to run the new download button look past some other developers to ensure no one else sees an issue with that, but it looks good to me.

Comment thread src/app/shared/file-download-link/file-download-link.component.html Outdated
@kshepherd
Copy link
Copy Markdown
Member

Thanks @jesielviana , improvements to responsiveness are always welcome!

Regarding the buttons and layout, I personally prefer the plain links if we're just displaying an unstructured list with badges and href... but I do like the idea to make the download / file elements more prominent.

I think the buttons could be improved by:

  1. Making the width consistent - this is hard to do with variable filenames, so perhaps an alternative to wrapping the file and some badges in the button, is to display the filename and badges / access status as text and badges (or a link and badges), and then a consistently sized square-ish "Download" button with an icon?
  2. Keeping good structure and context with the other elements in each list item. I can't really tell which download button the embargo badge belongs to, and while that badge is outside the button, the "primary bitstream" badge and "restricted" badge is within it

Maybe making use of a flex div, or something more table-like, could help. As it is, to me it looks a little bit cluttered.. less like functional "buttons" and more like variable-width divs with borders.

(Does it perhaps look tidier in different screen sizes? I'll have to check some cases)

@tdonohue
Copy link
Copy Markdown
Member

@jesielviana : For what it's worth, I like @kshepherd 's idea (his first bullet) to potentially just add a square download button next to each filename and display the filenames as links or text again. But, I almost feel like we'd need a couple mockups of how this might look in order to choose the "best" one.

To that point, I also want to note that we could also consider undoing the changes you've made to the file listing on the simple Item page, and treat that as a separate PR (to give us more time to come up with a better look). The changes you've made in this PR to the footer and the full item page are non-controversial, in my opinion, and could be merged more immediately.

@jesielviana
Copy link
Copy Markdown
Contributor Author

jesielviana commented May 1, 2025

Thanks @tdonohue and @kshepherd for your contributions. I agree with your points and will follow @tdonohue's suggestion to revert the changes to the Download button on the simple Item page.

@mwoodiupui
Copy link
Copy Markdown
Member

If we want to draw attention to the list-ness of the file controls, we could simply give the list a 1px or 2px border.

The badges are a complication for the list members. Short rules between them should delimit them effectively.

Or the list could be done with alternating soft background colors, which would handle both.

See also https://dspace-org.slack.com/archives/C3SG47SGY/p1746042279227259 ff.

@tdonohue tdonohue self-requested a review May 1, 2025 13:56
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 1, 2025

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

@tdonohue
Copy link
Copy Markdown
Member

tdonohue commented May 1, 2025

@jesielviana : I believe the merge conflicts here are accidentally caused by the merger of #4244. If you can get those conflicts resolved though, I'll gladly give this another review/test to ensure your recent updates look good.

@jesielviana
Copy link
Copy Markdown
Contributor Author

@tdonohue : Thanks for the heads-up! I’ve resolved the merge conflicts — this should be ready for another review now.

Copy link
Copy Markdown
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

@jesielviana : Tested & reviewed this again today. I've verified that the default Item page changes are now reverted & the download links are restored. The other functionality still works as well, but I ran into a minor bug on the "Full Item Page"

If you have an embargoed bitstream, on the "Full Item Page" it will show both a lock icon and a download icon. Here's what it looks like:

fullitempage

In that scenario, I'd recommend replacing the download icon with the lock icon, rather than showing the two icons next to each other.

Beyond that, everything else in this PR looks good.

@github-project-automation github-project-automation Bot moved this from 🙋 Needs Reviewers Assigned to 👀 Under Review in DSpace 9.0 Release May 2, 2025
@jesielviana
Copy link
Copy Markdown
Contributor Author

Thanks @tdonohue for the thorough re-test and detailed feedback!

You're absolutely right — showing both the lock and download icons for embargoed bitstreams on the Full Item Page is confusing. I've updated the logic so that only the lock icon is shown in that scenario.

The fix has been pushed — feel free to take another look and let me know if you notice anything else!

Copy link
Copy Markdown
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

@jesielviana : Thanks for the updates. I tested it again, and the "Full Item Page" issue is fixed. But, now I'm seeing download icons next to all files on the main Item Page like this:

icons

Was this change intentional? To be honest, I don't mind that look, but the "Primary" label now looks oddly squished up against the download icon. If we wanted to stay with this look, then the "Primary" label may need to be moved after the filename or similar.

If this change was not intentional, then it's also fine to remove it. I was just confused by it when I tested this PR, as that download icon wasn't there before your last modification.

@jesielviana
Copy link
Copy Markdown
Contributor Author

That wasn’t intentional — sorry about that! I’ll make the change right away.

@jesielviana
Copy link
Copy Markdown
Contributor Author

@tdonohue I’ve just pushed a fix for the issue.

This change introduces a boolean field in full-file-section.component to enable the use of the ds-file-download-link component with the option to dynamically display the download icon.

  • The new input is called showIcon.
  • By default, it is set to false to preserve the existing behavior across all views.
  • When set to true, the download icon will be shown next to the file link.

Apologies for the inconvenience, and thanks again for your feedback!

@tdonohue tdonohue self-requested a review May 5, 2025 19:48
Copy link
Copy Markdown
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

@jesielviana : Thanks for your hard work on this. It's looking great overall now, but I've still noticed a few minor issues we may want to clean up:

  1. On the Full Item Page, if you shrink the screen size (390px or similar), then any "Embargo until [date]" labels will be cut off. This was shown also in the screenshot of original ticket #4170, so we should clean it up. Here's how it looks in this PR:

embargo-label

  1. This is really minor, but I notice if you hover over one of those lock icons for an embargoed bitstream, you'll still see the title "Download [filename]". We might want to consider changing it to be "Request copy of [filename]", as clicking the icon will send you to the "Request Copy" form. This could be optional though, as it's very minor.

@jesielviana
Copy link
Copy Markdown
Contributor Author

@tdonohue, thanks for the feedback!

I fixed these issues by doing the following:

  1. I enabled white-space: normal for the embargo badge, so on small screens the text will now wrap instead of being cut off. This causes the label to break into multiple lines on very narrow screens, but I believe it's the best option for preserving readability.

  2. I created a method in the component to return the title dynamically. This way, when the file is under embargo, the title now displays "Request copy of [filename]" instead of "Download [filename]".

Copy link
Copy Markdown
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

👍 Thanks @jesielviana ! This all looks good to me now. I've tested and all the issues look to be resolved. The code looks good as well. I'm waiting on one last automated test to finish completion & then I'll merge this.

As noted in the original ticket, I think we should port this also to dspace-8_x. I'll see if the automatic backport works (triggered by the port to dspace-8_x label) once we merge this, or if we'll need a manual backport.

@github-project-automation github-project-automation Bot moved this from 👀 Under Review to 👍 Reviewer Approved in DSpace 9.0 Release May 6, 2025
@tdonohue tdonohue added this to the 9.0 milestone May 6, 2025
@tdonohue tdonohue merged commit 8757712 into DSpace:main May 6, 2025
17 of 18 checks passed
@github-project-automation github-project-automation Bot moved this from 👍 Reviewer Approved to ✅ Done in DSpace 9.0 Release May 6, 2025
@dspace-bot
Copy link
Copy Markdown
Contributor

Backport failed for dspace-8_x, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin dspace-8_x
git worktree add -d .worktree/backport-4218-to-dspace-8_x origin/dspace-8_x
cd .worktree/backport-4218-to-dspace-8_x
git switch --create backport-4218-to-dspace-8_x
git cherry-pick -x 84437149132498f59b75cf9b4a3b345a692b395c 4dd756a9c5a019d5405f1b123b316a3a030b4ab1 88e8251d741a9b3cf137dd7ebb8c1b4328fccdfe 21a54e97168099a2902d7b85fc501178478490a6 1496b3de2d33ff37f2882e2400200fb005a6ebfe 37ff8b1fca8d7d94e703c46f49a5eba1522dc8b5 a617b752bbd7bd40880e5310d51a26e86f06805e 72203966fc4264bc5e9f9ed2ae1128fa7df47323

@tdonohue
Copy link
Copy Markdown
Member

tdonohue commented May 6, 2025

@jesielviana : The automated backport to 8.x failed (see above). Would you be able to manually backport this to the dspace-8_x branch (in a separate PR) to try to fix these issues there? If it becomes too difficult, I think the more important issue to fix on that branch is the footer issue, but it would be nice to backport the entire PR if possible to do so.

4science-it pushed a commit to 4Science/dspace-angular that referenced this pull request Mar 20, 2026
Task/dspace cris 2025 02 x/DSC-2624

Approved-by: Francesco Molinaro
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1 APPROVAL pull request only requires a single approval to merge bug port to dspace-8_x This PR needs to be ported to `dspace-8_x` branch for next bug-fix release testathon Reported by a tester during Community Testathon usability ux User Experience related works

Projects

No open projects
Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

Responsiveness issues

5 participants