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

Localize Page Template Module Titles #1259

Merged
merged 4 commits into from
Apr 30, 2021
Merged

Conversation

hishamco
Copy link
Contributor

No description provided.

@hishamco
Copy link
Contributor Author

@sbwalker there's two tiny PRs that need to merge

@sbwalker
Copy link
Member

@hishamco I am trying to understand the intent behind this PR. Basically it will perform a one-time localization of module titles when the site is initially created. However if you then add another language to your site, the module titles will not be localized to the new language - they will remain in the original language. This is not the way localization is supposed to function? It appears you are trying to incorporate "content localization" into Oqtane - which is not in scope for the framework.

@hishamco
Copy link
Contributor Author

it will perform a one-time localization of module titles

You are right because module titles will be saved into the database, seems such data will not localized like the normal localized strings. Please don't merge this PR, I need to do some few tests, seem we need sort of Data Localization to make this work

Reminder for myself and all the only data will be localized are the STATIC one

It appears you are trying to incorporate "content localization" into Oqtane - which is not in scope for the framework.

No, Content Localization is another feature that may or might not be included in Oqtane

@hishamco
Copy link
Contributor Author

@sbwalker while I did some testing I notice a strange behavior in Lanaguage Management though your PR #1244. Did you remove the ability to add the languages from the admin panel? This breaks what I did previously, AFAIK the set of languages should be managed from the admin dashboard, once the resources for a particular language has been added into the binaries folder the translation for that language should work

The current behavior, the user is not able to add any language, no languages will be shown because the lanaguage management page is using ILanguageService not ILocalizationService

Please let me your intend, so I can change what's required to make this functional again, that why I insist to add unit and functional tests to avoid break things

Thanks

@sbwalker
Copy link
Member

sbwalker commented Apr 21, 2021

I have a different understanding of how this capability should work. My perspective has always been that only the cultures which actually exist in the installation should be the "source of truth". This feature should never have relied upon an appsettings.json entry which is specified manually and has no relationship to the resources which are actually installed. It does not make sense to me why you would want to allow Administrators to enable cultures in a site when the resources for those cultures may not exist. Most users would consider this to be a bug... as after enabling the culture they would see no change to the UI and assume the framework is broken.

@hishamco
Copy link
Contributor Author

When we start to add a supported cultures the appsettings.json was the solution that we started with, then you ask to make the admin to decide which languages are supported which is fine IMHO I see similar thing in many CMSs I think the thing that we miss is to show a warning message in case the added language doesn't have resources in the bin directory, this will remove all the confusion. The current language management is useless. I think we should bring it back if you don't mind with showing alert message

@sbwalker
Copy link
Member

I tried to explain the expected behavior for languages previously... but I must not have done a very good job...

Translations are installed at the framework level - and they use satellite assemblies so they are organized by culture subfolders under the /bin folder. The framework is aware of the satellite assemblies so if it needs to retrieve a list of supported cultures, it should do so dynamically based on what is actually installed. The appsettings.json approach was never the preferred solution - I always viewed it as a short-term hack until a proper solution could be implemented. The goal in Oqtane should be to keep appsettings options to a minimum - and especially avoid including any settings which could be determined dynamically.

Oqtane is multi-tenant. Therefore the list of supported cultures that can be retrieved at the framework level is not necessarily applicable to every tenant/site. As a result, there was a need for an individual site to be able to specify the languages it supports. However, it should only be able to select from the list of previously installed cultures. And the Language Management serves this purpose just fine. If I have multiple translations installed, it allows me to choose the cultures from the list in Add Language - which is exactly what I would expect. If there are no translations installed, the list is empty. In this case, I am fine with adding a message to the Add Language UI which says "Translations must be previously installed in order to enable them for this site". This would make the expected behavior more clear.

@hishamco
Copy link
Contributor Author

And the Language Management serves this purpose just fine. If I have multiple translations installed, it allows me to choose the cultures from the list in Add Language - which is exactly what I would expect. If there are no translations installed, the list is empty. In this case, I am fine with adding a message to the Add Language UI which says "Translations must be previously installed in order to enable them for this site". This would make the expected behavior more clear.

Go it, I will revise the Lanaguage Management, then check if it's working based on what you said as the following:

  • If the supported cultures are zero a message should be showen to add the translations before adding the language
  • If the supported cultures are greater than zero the admin allowed to add language(s) from the supported languages
  • The Languages switcher should list what it configured by the language management

I just need to make a quick demo to make sure nothing breaks, then test this PR

Thanks

@hishamco
Copy link
Contributor Author

Backing to this PR after few test the localization works great

Basically it will perform a one-time localization of module titles when the site is initially created

To make everything clear, the _localizer[""] that Introduced in SiteRepository to make the Oqtane extractor tool able to figure out the localization resources in there, but it will be not used for localization, so English translation will be saved all the time, nothing but this will let the translators to use such keys for translations, but the actual localization will be done in the UI itself

I will do one last commit to show you how it should work

@hishamco
Copy link
Contributor Author

AdminDashboard

To remove the confusion between static & dynamic translation, the module titles are dynamic data, fortunetly it will not updated and will be saved once the installtion is done, it becomes to me as static resource, so the localization should work again

@hishamco
Copy link
Contributor Author

Hope it's clear now why the localization work in this case while the data is dynamic

@sbwalker
Copy link
Member

sbwalker commented Apr 23, 2021

@hishamco the screen shot you posted above is the Admin Dashboard. The Admin Dashboard renders a list of Pages which are children of the "Admin" page. So the names which are rendered in that screen shot are actually Page names - not Module titles.

From a higher level, Page names and Module titles are dynamic content ( ie. they are stored in the database and can be modified by the user ). However, for practical purposes they are "static" content because every site includes these Admin pages/modules. So I believe they should be supported through static localization.

In order to do that, I believe Oqtane needs a global/shared resources area where common terms can be defined ( as @leigh-pointer mentioned previously ) - and the static names of the Admin pages/modules can be defined there as well. For example, "Site Settings" would be a key/entry in the shared/global resource class - which would allow translators to override the value in their culture specific translations. Oqtane would then use standard localization to translate the "Site Settings" value for page names and module titles ( ie. treating it as static text even though it is actually dynamic ).

If we follow this approach, then the Admin Dashboard ( Oqtane.Client\Modules\Admin\Dashboard\Index.razor ) could be modified to reference @localizer[p.Name] rather than @p.Name ( similar to the changes you made to Module Title ).

This way the solution handles any language and is not dependent on the default language used during a site installation ( and on that note, I believe the changes in this PR related to SiteTemplates are not the proper solution and should not be merged ).

@hishamco hishamco changed the title Localize modules title Localize Page Template Module Titles Apr 25, 2021
@hishamco
Copy link
Contributor Author

@hishamco the screen shot you posted above is the Admin Dashboard. The Admin Dashboard renders a list of Pages which are children of the "Admin" page. So the names which are rendered in that screen shot are actually Page names - not Module titles.

Sorry, I fixed the PR title

From a higher level, Page names and Module titles are dynamic content ( ie. they are stored in the database and can be modified by the user ). However, for practical purposes they are "static" content because every site includes these Admin pages/modules. So I believe they should be supported through static localization.

If we follow this approach, then the Admin Dashboard ( Oqtane.Client\Modules\Admin\Dashboard\Index.razor ) could be modified to reference @localizer[p.Name] rather than @p.Name ( similar to the changes you made to Module Title ).

I think what I did in this PR is to use @Localizer[p.Name] not @p.Name

@hishamco
Copy link
Contributor Author

hishamco commented Apr 25, 2021

At the end I can close this PR or add the possibility to use Shared Resources if it's possible

@sbwalker
Copy link
Member

Yes @hishamco the changes to Oqtane.Client\Modules\Admin\Dashboard\Index.razor ( ie. using @localizer[p.Name] rather than @p.Name ) are fine. The changes to SiteRepository.cs should be removed from this PR and we should use a static Shared Resources approach to localize admin page names and module titles.

@hishamco
Copy link
Contributor Author

we should use a static Shared Resources approach to localize admin page names and module titles

Perhaps this is easy but I'm thinking on how do we extract them

1 similar comment
@hishamco

This comment has been minimized.

@sbwalker
Copy link
Member

I am only thinking of the needs of the core framework and module developers - where Shared Resources are a very common requirement. I was not thinking about the complications of how to extract them using an automated tool. This is precisely the challenge with having a dependency on an external tool - and why the Oqtane philosophy explicitly tries to minimize dependencies - as I would not want the framework to be restricted or blocked by the capability of an external tool. As much as possible the framework needs the freedom to provide the best solution to users without having to worry about external tools. I obviously understand that managing resx files is complicated and tedious - so your utility is very useful... but the utility needs to serve the needs of the framework - not vice versa. I hope this makes sense.

@hishamco
Copy link
Contributor Author

This is precisely the challenge with having a dependency on an external tool - and why the Oqtane philosophy explicitly tries to minimize dependencies - as I would not want the framework to be restricted or blocked by the capability of an external too

Totally agree, so this my challenge to make the tool extract the Shared resources, I will give it a try and come back to you

FYI I will start to create a shared resources for page titles at the beginning, then we can extract the common localized strings from various components

@sbwalker
Copy link
Member

@hishamco for this PR just remove the changes to SiteRepository.cs and I will merge it

@hishamco
Copy link
Contributor Author

If I remove the changes in SiteRepository the strings will not be localized, I prefer to use shared resources with what I added

@sbwalker
Copy link
Member

Ok, I will close this PR then as saving localized strings to the database during installation is not the correct solution.

@hishamco
Copy link
Contributor Author

No need to close the PR, I revert the changes in SiteRepository then I will add a shared resources

@hishamco
Copy link
Contributor Author

I spent few hours struggling to make the SharedResouces work from the Oqtane.Shared, at the end I realized it will be better to add the shared resources to Oqtane.Client instead of splitting the resx files, but in case we need every project has it's own resx files I need to add few changes in Oqtane.Shared project. Your thought @sbwalker

@sbwalker
Copy link
Member

@hishamco I think its fine to add them to Oqtane.Client rather than Oqtane.Shared. One question I have is if a component needs to use both its own resources as well as SharedResources - how would they declare the Localizer? Is it possible to have 2 Localizers in the same component?

@sbwalker
Copy link
Member

sbwalker commented Apr 29, 2021

Actually my question has already been answered here oqtane/oqtane.translations#16 by @leigh-pointer

@Inject IStringLocalizer Localizer
@Inject IStringLocalizer SharedResources

@sbwalker sbwalker merged commit 2244fcc into oqtane:dev Apr 30, 2021
@hishamco hishamco deleted the more-localization branch May 1, 2021 18:23
@hishamco
Copy link
Contributor Author

hishamco commented May 1, 2021

@leigh-pointer please your feedback in case you already used the shared resources

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.

None yet

2 participants