Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move OrchardCore.Translations.All #17036

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Move OrchardCore.Translations.All #17036

wants to merge 2 commits into from

Conversation

Skrypt
Copy link
Contributor

@Skrypt Skrypt commented Nov 18, 2024

To OrchardCore.Application.Cms.Core.Targets

Fixes #17035

To OrchardCore.Application.Cms.Core.Targets
Copy link
Member

@Piedone Piedone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a good change to me but @hishamco please comment.

@sebastienros
Copy link
Member

Should we split Translation.All in two such that Core can reference core module translations, and .All include Cms modules too?

@Skrypt
Copy link
Contributor Author

Skrypt commented Nov 20, 2024

@sebastienros That would be even better.

Translation.Core.All
Translation.CMS.All

Though, the thing is that currently the OrchardCore.Application.Cms.Targets is only there for including OC themes to the OrchardCore.Cms.Web project.

We would need to split translations for CMS modules vs Core Modules. Then have 3 or 4 .targets projects to included them all separately. I'm not sure if we have everything splitted up with Core and Modules. That may cause some issues of missing translations in the beginning.

Here, I remember that we set the Translations.All in the Cms.Target because we wanted to make the main one to be lightweight but that doesn't make sense to have modules that can be translated without having translations. We end up adding the reference to our web project. And it doesn't follow the same version as the current one which can lead to forgetting upgrading it along with the OC version.

@hishamco
Copy link
Member

I think the main issue here is that we include all the translations even if we didn't use them, what if we include them on demand?

I saw some CMS load the translations on the fly, but while we don't have this capability we could install them using dotnet CLI

@sebastienros
Copy link
Member

@Skrypt I could check ... but as I understand it the "All" is for "all languages", not "all modules" ? If so then the change is fine.

@Skrypt
Copy link
Contributor Author

Skrypt commented Nov 25, 2024

@sebastienros It is both all languages and all modules. We don't make a distinction yet between Core and Modules. I think that's maybe the enhancement we could have. This way we could just use Core translations when we want to use OC as a framework so that we can get the DataAnnotation translations as an example.

But in the mean time I think this PR is adequate because you at least want all translations if you don't have a choice.

At the same time here we are talking about a Nuget package that is 8.3 MB so it is maybe neglectable for most people?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OrchardCore.Translations.All is not referenced by OrchardCore.Application.Cms.Core.Targets
4 participants