Minimal 'dspace' theme

[artlowel]

References

• Fixes Simple default theme for UI #727
• Depends on Make all non-admin page-level components themeable #1042

Description

This PR adds a simple default theme to dspace, based on the mantis theme we had for the preview release.

Instructions for Reviewers

This PR removes the mantis theme folder, and adds a new "dspace" theme.

All changes are done using css apart from two exceptions:

• I did override the HTML for home news component. No institution using dspace is going to leave that at its default contents. So it's good to have an example of how to customize it.
• I fixed a small issue we've had for a long time in root.component.ts (so outside of the themes folder): while the app is loading the next route, the loading component appeared above the contents of the old route. My change hides the contents of the old route while loading. → was already fixed by #728 accessibility theming #1057

To test this, simply build it and make sure you're not overriding the themes array in your environment files. It should show up by default as I've made it the default theme.

This PR seems bigger than it is, because it depends on #1042 You can see only the changes in this PR here

Checklist

• 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 TSLint validation using yarn run lint
• 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.
• 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]

This pull request introduces 5 alerts when merging f7e45e4 into 1115c58 - view on LGTM.com

new alerts:

• 5 for Unused variable, import, function or class

[tdonohue]

@artlowel : Just a quick note here...this work will need to be integrated with #1057 as well (as this PR should depend also on that PR). But, that might be more easily done once #1042 is merged. In any case, it's great to see an early version of this PR. Just wanted to be clear that in terms of priority, I feel both #1042 and #1057 should be merged first, followed by this one.

[lgtm-com]

This pull request introduces 1 alert when merging 4035739 into 1115c58 - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[artlowel]

Note that the one unused import LGTM is warning about was left in on purpose. Because the base theme is commented out, the import isn't used. But it would be needed if you were to uncomment it again, that's why I chose to leave it in.

[lgtm-com]

This pull request introduces 1 alert when merging d924c0f into 1115c58 - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[lgtm-com]

This pull request introduces 1 alert when merging be1d78a into 66463cf - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[tdonohue]

@artlowel ; I tried this out today just to see if custom themes are working for me...this one also is unfortunately not working entirely as expected (see screenshot below). A few immediate issues I hit include:

1. Strangely, the banner.jpg doesn't load for me at all. In Chrome DevTools, I see it returns a 404 Not found for GET http://localhost:4000/assets/dspace/images/banner.jpg
2. The new color scheme in this PR is also seemingly not applying (again this sounds like the issue with _theme_sass_variable_overrides.scss not working), so the result for me is that I see the same color scheme as the demo site (https://demo7.dspace.org/), while it looks like your screenshot has a different color footer, and the blue of the links looks changed as well.

Here's a screenshot of the homepage I see:

Again, as usual, I'm doing a yarn clean, yarn install, yarn start. I'm running Node v14.6.0 and Yarn v1.22.4 on Windows 10.

I also just tried one of the more "obvious" Windows-specific issues... I changed the line endings of all the files in the dspace theme from CRLF (Windows line endings) to LF (Unix line endings). Unfortunately, that didn't have an impact. I have not tried to debug this further, but I hope to spend more time with it after getting other reviews done. In any case, I wanted to simply report that this PR currently has the same issues in my environment as previously reported with other simple custom themes.

[abollini]

hi, @artlowel @tdonohue I take the freedom to update the PR title to reflect the fact that this is about a sample, additional minimal theme added just to demonstrate the ability to use multiple theme and provide some examples to mention in the documentation. Nothing in such theme should constitute improvement in the UX, look & feel, accessibility or other aspects that should be instead applied to our "default" (i.e. in #1057 )

[artlowel]

@tdonohue I finally got a chance to set up a dev environment on my home windows PC, and I could reproduce your issues. I think I fixed them. The cause turned out to be path separators, as usual. Could you test it again?

[lgtm-com]

This pull request introduces 1 alert when merging 4579245 into a5bc6c1 - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[tdonohue]

@artlowel : I can verify that your path separators fix has fixed all my issues with getting the _theme_sass_variable_overrides.scss file to work properly! I can also see the banner.jpg in this PR now. 🎉 Glad you were able to figure out the problem.

(Just as a sidenote, while my tests are all successful with this PR, I'm refraining from a full review until #1057 is finalized & this PR can be rebased on top of the code changes in #1057. However, generally speaking, it looks good at a glance. But, we will need to look closer at any HTML changes in this dspace theme, as ideally those would be moved into the source component, allowing the dspace theme to be only CSS & new images. It looks like, at a glance, the home-news component may be changing HTML...but again, we can discuss more as needed once #1057 is finalized/cleaned up.)

[lgtm-com]

This pull request introduces 1 alert when merging f9143ac into d7daaf4 - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[lgtm-com]

This pull request introduces 1 alert when merging fb634cd into 0f7a32d - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[lgtm-com]

This pull request introduces 1 alert when merging cbee776 into 0f7a32d - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[tdonohue]

@artlowel : Gave this a quick try, overall, this looks great. I have a few very minor suggestions, but I'll go ahead and approve it 👍 .

Most of my suggestions are trying to improve color contrast for accessibility. Though, I'm not sure all the issues are in this 'dspace' theme itself, so we could choose to clean these up in a follow-up PR quickly. I also linked to the Chrome plugin/tool I was using to analyze the homepage.

[tdonohue]

We're going to have to make $cyan slightly darker...I'm getting an insufficient color contrast issues with this color on a white background when using the axe DevTools tool in Chrome. Maybe change this to something like #207698 (which, to my eyes looks similar but darker).

Similarly, I think we'll need to update the .text-muted style (used in descriptions of communities on homepage, etc) to be something like #767676 in order to solve color contrast issues there too (although this style might be in the base theme instead of the 'dspace' one).

[artlowel]

We're going to have to make $cyan slightly darker

Done. I used your suggested color

Similarly, I think we'll need to update the .text-muted style (used in descriptions of communities on homepage, etc) to be something like #767676

That would make text-muted indistinguishable from text-secondary, so then logical thing to do might be to use text-secondary everywhere we use text-muted now? But that would have to be a separate PR, updating the base theme.

For now, I've changed $gray-600 (which is what text-muted uses) to be halfway in between its old value and $gray-700 (which is text-secondary). Perhaps that's enough?

[lgtm-com]

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

new alerts:

• 1 for Unused variable, import, function or class

[atarix83]

thanks @artlowel for this PR

it looks quite good, I've a couple of feedback:

• the chosen font seems to render the characters too thiny, and in my opinion it creates contrast issue on a white background. this is most notable with the links
  
  The default one seems to be clearer and more functional in my opinion
• the background image used in the home-page creates contrast issues too, because the text results difficult to read in some points of the image. If it's not strictly necessary I would remove it

[atarix83]

why this has been replaced with the themed css? Should the theme applied override the base-theme.css automatically?

[artlowel]

It should, feel free to put it back and verify that it will update automatically. But it's an efficiency thing.

If you put the css needed for the initial page load there in the first place, it prevents people from having to download the wrong css first, before app.component can replace it.

It is something I would recommend everybody does after they've decided on the theme they want.

[atarix83]

it makes sense, maybe for the 7.1 we can think to improve it by selecting the css according to the chosen theme. It could be made with webpack, here an article that could help https://medium.com/dailyjs/inserting-variables-into-html-and-javascript-with-webpack-80f33625edc6

[artlowel]

Thanks for the review @atarix83

Both the font and the background image were put there to give an example of how dspace could be themed in ways that a lot of institutions might want to, as well as to give dspace a somewhat nicer look out of the box.

If we simply keep the base font, we don't give an example of how to change the font. But you're right, it was very thin. I've replaced it now with a similar but slightly thicker font.

For the same reasons I want to retain the background image. So I made the dark overlay on the background image slightly darker to make the contrast with the text bigger

[lgtm-com]

This pull request introduces 1 alert when merging 9bf2465 into 3d51110 - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[atarix83]

thanks @artlowel it looks better now

[atarix83]

it makes sense, maybe for the 7.1 we can think to improve it by selecting the css according to the chosen theme. It could be made with webpack, here an article that could help https://medium.com/dailyjs/inserting-variables-into-html-and-javascript-with-webpack-80f33625edc6

[artlowel]

it makes sense, maybe for the 7.1 we can think to improve it by selecting the css according to the chosen theme. It could be made with webpack, here an article that could help https://medium.com/dailyjs/inserting-variables-into-html-and-javascript-with-webpack-80f33625edc6

You're right. I considered that when working on #1035, but it wasn't trivial as it would require us to determine which theme is active for the home page, outside of angular (in the webpack config). As I was short on time I left it out back then. But it would definitely be an improvement.

[artlowel]

The main reason for the that css being hardcoded in the HTML in the first place, is so you don't get a flash of unstyled content while the app is loading for the first time. Perhaps a better way of solving that problem would be to create a small dedicated CSS file that only styles the initial loading component (and perhaps a few other things that might be visible in the first fraction of a second), that can't be themed based on the route, but is the same all the time

As soon as the app bootstraps, the theme css can take over.

[tdonohue]

Merging as this is now at +2. I'll create a follow-up ticket regarding selecting the CSS automatically based on the chosen theme.
UPDATE: see #1093