Dynamic theme improvements #1326
Conversation
…3628_Dynamic-theme-fixes
There was a problem hiding this comment.
@artlowel : Gave this a test today & it all seems to work. I tried out the PR as-is, just to make sure I saw no obvious changes in behavior (none noticed). I then tried out your sample branch to verify that the bug you noted is fixed...and it worked as well.
Overall, it looks good. That said, I have a few minor requests... a couple specs have very confusing wording, either they need rewording or more inline comments. And, I think this new extends option needs documentation (as I think this is a new feature we should advertise in 7.1). Beyond that, it looks good!
src/app/shared/metadata-representation/metadata-representation.decorator.spec.ts
Show resolved
Hide resolved
src/app/shared/object-collection/shared/listable-object/listable-object.decorator.spec.ts
Outdated
Show resolved
Hide resolved
| * Specify another theme to build upon: whenever a themed component is not found in the current theme, | ||
| * its ancestor theme(s) will be checked recursively before falling back to the default theme. | ||
| */ | ||
| extends?: string; |
There was a problem hiding this comment.
I think this extends is a great concept & one that people will be excited about. But I think we need to document it. Could we add an example or two to environment.common.ts?
I'd also really appreciate it if you could add some basic docs to https://wiki.lyrasis.org/display/DSDOC7x/User+Interface+Customization about "Extending Other Themes"(with a note saying "This feature is coming in 7.1")
There was a problem hiding this comment.
Added an example in comments.
Also have a little doc entry ready to go, don't have a wiki account yet though
There was a problem hiding this comment.
@ybnd : Comments look good, thanks!
If you'd like a wiki account you can request one at wikihelp@lyrasis.org. Once you have a wiki account created, I can give you access to edit the "DSDOC7x" area of the wiki.
|
Thanks @artlowel and @ybnd and @Atmire-Kristof ! Merging this with +2. I've also gone ahead and added some very basic docs (about
Feedback or improvements to the docs are welcome. |
Gdpr metrics DSC-1522 Approved-by: Andrea Barbasso Approved-by: Giuseppe Digilio
Description
This PR fixes an issue where if you have a different theme that's only active on certain routes, you can still see the old theme flash for a second first, before the correct theme is loaded. The cause was that the theme matching the route was determined in an ngrx effect, and effects are run in (something emulating) a different thread, so you could have some situations where the timings were off.
This PR addresses the issue by moving that logic to the theme service and triggering it from app component. That ensures the loading component is shown until the theme resolution is complete.
The PR also adds the ability for themes to extend each other, that way, if you want for example to have a shared root theme, and a slight variation for each community you don't need to copy the entire root theme to each community theme. The community themes can simply extend the root theme and only contain the components that differ.
Instructions for Reviewers
You can test that the issue is fixed by configuring a visually distinct theme for a certain route in dspace, and going to that route by clicking a link in the UI (so not directly by pasting the URL) With this PR, you should always see a loading screen until the new theme is fully loaded.
You can test the extending of themes by creating a new theme (theme A) with one or two components, setting the
extendsproperty for that theme in the environment file to the name of another theme (theme B) and verifying that if theme A is active, the components in that theme are used, components that aren't in that fall back to the theme B versions.Update: To make reviewing easier, you can find a branch that builds on this PR and adds a few themes to test with here: dynamic-theme-fixes-review. It adds 3 themes that are always active. theme-c extends theme-b, theme-b extends theme-a. theme-c is active on the homepage, but if you go to the homepage can see that for components that aren't available in theme-c, it falls back first to theme-b then to theme-a
The custom theme has a red background on that branch and is configured to show for any community. A link to a random top level community is added to the homepage. So if you click the link you should see a loading screen, and then immediately the community with a red background. It shouldn't briefly show with a white background first.
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!
yarn run lintpackage.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.