Conversation
a0ebe23 to
d7c6b36
Compare
|
Hi @alexandrevryghem, |
# Conflicts: # src/app/core/shared/listable.module.ts
d7c6b36 to
1b2541f
Compare
FrancescoMolinaro
left a comment
There was a problem hiding this comment.
Hi @alexandrevryghem, many thanks for the change and sorry for the delay on the review.
I did some tests and checked the code, it looks great and I didn't experience any issue so I am going to approve this one.
Thanks again!
tdonohue
left a comment
There was a problem hiding this comment.
👍 Thanks @alexandrevryghem ! This looks good to me too. I tested it to verify custom themes are still working, including different themes for different collections or paths.
Just a note though, I think this will require minor Documentation updates to https://wiki.lyrasis.org/display/DSDOC10x/User+Interface+Customization as that page mentions the lazy-theme.module.ts and eager-theme.module.ts. Could you update those docs when you get the chance?
In the meantime, I'm going to merge this with +2 votes.
|
@tdonohue: I've updated the docs |
|
Thanks @alexandrevryghem ! One last question for you... is this a "breaking change" for existing themes? It seems like it may be considering a site now may need to update Do you have any recommendations for sites in migrating older 9.x themes to this new 10.0 theme structure? It seems like we may want to add those to the "Breaking Changes" section of the Release Notes in some way. (I'd be glad to help with this, but I'm not sure I know what is required in migrating a 9.x theme to 10.0 and what is optional.) |
|
@tdonohue: Yes indeed users will have to migrate their old module files to this new format, so they should follow these steps:
NOTE: when #4528 is merged the fourth & fifth step can be removed, because that PR will delete the |
|
@alexandrevryghem : Thanks for those notes. I've created a new section of the theme docs called "Upgrading an Existing Theme" and added a section for "Upgrading from 9.x to 10.x". I've copied your instructions into that location & tried to give examples (based on the changes made in this PR). If you have a chance, please review this. I'll also add a new to the Release Notes referencing this theme upgrade section. |
|
Looks great thnx @tdonohue! |
Description
Currently all the themed components in
src/themeshave been migrated to standalone declaration, but they don't work unlesss they are referenced inside either theLazyThemeModuleorEagerThemeModule. I updated the config in order to remove this constraint, this way we won't have to fix merge conflicts in those files anymore.The reason why those modules were still necesarry is because the
tconfig.*.jsonfiles only compile the*.module.tsfiles inside thesrc/themesfolder. Since those modules contained imports to all the components inside the theme, they were automatically comiled as well. I updated the config to compile all the typescript files inside thesrc/themesfolder, this way we can drop the modules.Instructions for Reviewers
List of changes in this PR:
tsconfig.*.jsonfiles to compile all the typescript files inside thesrc/themesfolder instead & removed the unusedangularCompilerOptions.entryModuleproperty that isn't being used anymore since the switch to standalone componentsLazyThemeModulesEagerThemeModules and replaced them with 2 arrays:lazy-listable-components.ts#LISTABLE_COMPONENTS: This is similar to the oldENTRY_COMPONENTSarray, but it only contains imports for components that use@listableObjectComponentdecorators. Other dynamically loaded components that don't use custom decorators likeStartsWithDateComponentshould be declared in the decorator files instead (e.g.starts-with-decorator.ts).eager-theme-components.ts#COMPONENTS: This is similar to theDECLARATIONSarray from the oldEagerThemeModule, except that it doesn't contain theENTRY_COMPONENTSentriesGuidance for how to test or review this PR:
build:statsfor this)Checklist
mainbranch of code (unless it is a backport or is fixing an issue specific to an older branch).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.