Upgrade to Bootstrap 5 & realign themes

[Wout-atmire]

References

• Fixes Upgrade Bootstrap to v5 #2855
• Replaces Bump bootstrap from 4.6.2 to 5.0.0 #3316

Description

This PR aims to change the Bootstrap version of DSpace from Bootstrap version 4 to Bootstrap version 5 because version 4 is out of date. This version upgrade includes the required visual changes to maintain the same look as the DSpace version with Bootstrap 4.

Instructions for Reviewers

List of changes in this PR:

• Updated packages @ng-bootstrap/ng-bootstrap to version ^12.0.0 and bootstrap to version ^5.3
• We updated the SCSS to align with the migration-conditions of changing to Bootstrap 5. So that it remains compatible
  • Mainly updating old Bootstrap 4 classes to the renamed Bootstrap 5 classes (eg text-right to text-end, ml-4 to ms-4, ...)
• Updated the styling ot perserve the look of DSpace
  • Added / Removed / Changed HTML element's Bootstrap classes
  • Changed which HTML elements uses which Angular directive
  • Removing certain HTML elements or changing them from type
  • Added new SASS variables
  • Added new CSS mappings

To test this you can start a local Angular instance in combination with the REST of host: demo.dspace.org. You can then compare it in another browser tab with DSpace that still uses Bootstrap 4 https://demo.dspace.org.
Note that we deliberately left in some style changes:

• The width of the containers

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.
  Reason: This is an upgrade for a major dependency and thus it affects a lot of code.
• 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]

@Wout-atmire : Thanks for the quick changes! Overall, this is looking better. But, I noticed two issues still remain:

1. You still appear to be accidentally undoing the changes of #3227 (see inline comment below). That prior feedback was not addressed.
2. I agree with @FrancescoMolinaro that I don't believe you can remove all the --bs-* variables defined in the _bootstrap_variables_mapping.scss file. If you search our codebase many of these styles are still referenced in other files. Either we need to remove all those references, or we need to restore the --bs-* mappings in some way.
   • I gave one example in a comment below, but if I search for --bs- in our codebase after applying this PR, I still see 78 references that exist (and almost all of these --bs- variables have been removed from the _bootstrap_variables_mapping.scss, which means those styles will have broken or be slightly different).

Overall though, everything else that I previously reported has been fixed.

[Wout-atmire]

Hi @FrancescoMolinaro @tdonohue , thanks for the feedback.

I added the mappings from https://github.com/DSpace/dspace-angular/blob/main/src/styles/_bootstrap_variables_mapping.scss and only removed the ones that didn't have any reference in the code & didn't have a value in bootstrap 5.
In the case where the variable had a reference but didn't have a value in bootstrap 5, I removed the mapping & updated the refernced code to not use the variable anymore.
Indeed, as you said, I found many mappings that were removed despite having a value.

@FrancescoMolinaro Regarding the (collection selector in submission li:hover)
I mapped $dropdown-link-hover-bg to a CSS variable but the value has been
changed from #e9ecef -> #f8f9fa in bootstrap 5.
Does this need to be changed back to the old value (on collection selector in submission li:hover), or was it just an example of a missing mapping and its possible results?

Please let me know if any further changes are needed. I appreciate your review.

[tdonohue]

@Wout-atmire : Thanks for the updates! This is looking much better now. However, I've stumbled on two more small styling bugs.

1. The tooltip (hover over) for Entities (or ORCIDs) is no longer readable. The text should be either light gray or white. See https://demo.dspace.org/entities/publication/c491122e-9192-4ecd-a3d7-04062701432f as an example. Here's what it looks like in this PR:
   

2. On the Edit Item -> Bitstreams tab, the bitstream listing shows the filename under the drag handle. On the demo site, the filename appears next to the drag handle. This is minor, but I think we should fix it. Here's what it looks like in this PR with the filename wrapped under the drag handle.
   

Beyond that, it's looking great for me now. I think it's nearly ready to merge.

[FrancescoMolinaro]

Hi @Wout-atmire, thanks again for the swift response, I agree with @tdonohue on the couple of issues he found and I believe that other than that the PR looks good.
About the bootstrap color change, I think that if the change is internal to bootstrap and we didn't have any override in place we can keep it as it is, there is almost no visual difference and most likely the change has some purpose, maybe a little contrast enhancement, I just thought was a change from our side.

Thanks again for looking into it.

[Wout-atmire]

Hi @FrancescoMolinaro @tdonohue,
Updated the tooltip color so that it uses the old color;
Also made it so that on the edit-bitstream page the icon is displayed inline with the text.
Please let me know if any further changes are needed. I appreciate your review.

[ybnd]

@tdonohue @FrancescoMolinaro
Quick note regarding the --bs-* CSS variables: Bootstrap 5 exposes many of these by itself, so the old file is doing "double work".

Our original intent was to trim down the SCSS → CSS variable mappings to what's not provided by Bootstrap 5 out of the box.

E.g. if you revert 4e61b5f you'll still see --bs-white pop up in the styles as follows:

[tdonohue]

@ybnd and @Wout-atmire : I see. That was not clear to me at all. If these --bs-* settings are being generated by Bootstrap itself, then I agree that there's no reason for DSpace to manage them itself. So, I'd be OK with reverting the change you mentioned.

However, if we wish to go that route, then I'd highly recommend we add some code comments to our _bootstrap_variables_mapping.scss file to note that Bootstrap 5 now exposes the --bs-* variables and link to the Bootstrap docs on those variables (which appears to be here? https://getbootstrap.com/docs/5.0/customize/css-variables/). That will document this change better for people who eventually upgrade to DSpace 9.0 (otherwise, I bet we'll get questions about the removal of all these --bs-* variables on mailing lists).

The reason I found the original changes so confusing was that I was not aware these are now being set by Bootstrap 5 itself.

[tdonohue]

@ybnd and @Wout-atmire : Just a quick note though, I'm still not sure if we can revert all of 4e61b5f because Bootstrap says it only defines around 24 variables....namely the ones which reference colors/fonts listed here: https://getbootstrap.com/docs/5.0/customize/css-variables/#root-variables

It looks to me like 4e61b5f modified many more variables than just those colors/fonts. So, we may only be able to remove the colors/fonts in favor of those defined by Bootstrap already.

[ybnd]

@tdonohue @Wout-atmire we're upgrading to Bootstrap 5.3, so the full variable list includes a few more: https://getbootstrap.com/docs/5.3/customize/css-variables/#root-variables

We'll go over this once more to make sure everything we removed is covered.

100% agree about documenting this, the confusion is on me, sorry about that.

[Wout-atmire]

Hi @FrancescoMolinaro @tdonohue,
Reverted the commit 4e61b5f.
And added the mappings of the variables that are not mapped by default by bootstrap 5 that also have a reference within DSpace.
Also added the documentation to avoid confusion.
Please let me know if any further changes are needed. I appreciate your review.

[tdonohue]

👍 Thanks @Wout-atmire and @ybnd ! This now looks good to me. I gave it another test today and verified that everything previously reported is still fixed. I also verified @FrancescoMolinaro 's prior feedback still seems to be resolved.

Because @FrancescoMolinaro submitted a prior approval, I'm going to consider this +2 approval, and merge it immediately.

However, as a sidenote, I will have to "squash merge" this PR (as a single commit) because the commit history has gotten very messy & lengthy (over 120 commits, some of which look unrelated to this PR). In the future, you can avoid this by rebasing your own commits to squash them down yourself (using git rebase -i or similar).