Upgrade to Bootstrap 5 & realign themes #3506
Conversation
…pgraded to Bootstrap 5.3.3
… side of its label fix
…rd-dspace-themes-after-bootstrap-5-upgrade-9.0
atarix83
requested review from
FrancescoMolinaro
and removed request for
FrancescoMolinaro
There was a problem hiding this comment.
Hi @Wout-atmire , many thanks for these changes!
I had a look to the PR and it looks good, anyway I found some issues, you can find them as comments on the code diff.
In addition I found out a couple of problems more that I couldn't spot in the code.
In specific in the submission page I can see that the arrows of ds-number-picker changed color and are now greenish:
current
before
Furthermore I believe we lost some of the classes from ngDynamicForms, for instance the is-invalid class. This is reproduceable for instance on the license checkbox, where the text is not red anymore:
current
before
src/app/shared/search/search-labels/search-label/search-label.component.html
Outdated
Show resolved
Hide resolved
| @@ -5,6 +5,8 @@ | |||
|
|
|||
| .dropdown-menu { | |||
| overflow: hidden; | |||
| min-width: 100%; | |||
| top: 100%; | |||
| @include media-breakpoint-down(sm) { | |||
There was a problem hiding this comment.
Hi @Wout-atmire I think that we should change also this media-breakpoint-down as you did for all the others.
| @@ -2,6 +2,9 @@ | |||
|
|
|||
| @import './_theme_sass_variable_overrides.scss'; | |||
| @import '../../../styles/_variables.scss'; | |||
|
|
|||
| $theme-colors: map-merge($theme-colors, $theme-custom-semantic-colors); | |||
There was a problem hiding this comment.
Hi @Wout-atmire would be possible to move this sass variable inot the sass override file?
...em-bitstreams/item-edit-bitstream-drag-handle/item-edit-bitstream-drag-handle.component.html
Outdated
Show resolved
Hide resolved
...m-page/edit-item-page/item-bitstreams/item-edit-bitstream/item-edit-bitstream.component.html
Outdated
Show resolved
Hide resolved
src/app/browse-by/browse-by-metadata-page/browse-by-metadata-page.component.html
Outdated
Show resolved
Hide resolved
| @@ -6,820 +6,58 @@ | |||
| // Variables should follow the `$component-state-property-size` formula for | |||
| // consistent naming. Ex: $nav-link-disabled-color and $modal-content-box-shadow-xs. | |||
|
|
|||
There was a problem hiding this comment.
Hi @Wout-atmire is there a reason to delete all the existing variable mappings?
There was a problem hiding this comment.
Hi @FrancescoMolinaro, thank you for the feedback. The removed mappings were dependent on variables that are no longer present in Bootstrap 5. Let me know if you need any further clarification.
There was a problem hiding this comment.
@Wout-atmire : Thanks for this PR! I gave this a code review & testing today. Overall, it's looking good, but I've run into a few things as well. (I also noticed a lot of the things that @FrancescoMolinaro mentioned above, so I've excluded those from my feedback.)
First, besides what is mentioned above, I've found a few pages which need some minor CSS cleanup as they no longer look as good.
- The collection harvesting options now have a radio button (circled below). That should not be visible. Part of this form is also slightly misaligned (see for example how this page looks on sandbox.dspace.org)
- The Administrative Reporting feature (disabled by default) is showing no space between checkboxes and the text next to them.
Other than that, I didn't notice anything that stood out. I did notice a few other minor oddities though in my code review...see inline below.
I'm basically a +1 to this work once this minor feedback is addressed along with the feedback of @FrancescoMolinaro above. Thanks again.
References
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:
@ng-bootstrap/ng-bootstrapto version^12.0.0andbootstrapto version^5.3text-righttotext-end,ml-4toms-4, ...)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 4https://demo.dspace.org.Note that we deliberately left in some style changes:
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!
mainbranch of code (unless it is a backport or is fixing an issue specific to an older branch).Reason: This is an upgrade for a major dependency and thus it affects a lot of code.
npm run lintnpm run check-circ-deps)package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.