-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Minor fixes for localizations #8726
Conversation
# Conflicts: # src/Orchard.Web/Modules/Orchard.Blogs/BlogsLocalizationExtensions/Handlers/BlogPostPartHandler.cs
public ActionResult GetTaxonomy(string contentTypeName, string taxonomyFieldName, int contentId, string culture) { | ||
public ActionResult GetTaxonomy(string contentTypeName, string taxonomyFieldName, int contentId, string culture, bool isAdmin = false) { | ||
|
||
if (isAdmin) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a front-end controller that you want to use in the admin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a controller that we were using already to dynamically (i.e. via an ajax call) replace the available terms in a TaxonomyField when the culture of the content that's being edited changes. It does so by returning the rendered shape for the field with the localized terms.
Without an admin filter, it would render the shape using alternates from frontend themes, if there were any (we have a few cases were we have some "edit" functionalities in the frontend).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't do that. Or we would have to do it any time someone wants to start rendering front-end results in the admin (or the other way around).
You create a dedicated controller or method for the admin or front-end.
And it's the same for the Views. A view is for admin or front-end, not both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The situation before this PR is that the action from this controller gets called by what's rendered in the TaxonomyFieldList EditorTemplate.
If the current applied theme (for frontend) has an alternate for the EditorTemplates for TaxonomyFields, this action returns the html rendered out of that alternate. This causes a "misalignment" in how things are rendered in the backend.
If I understand you correctly, what you are proposing is to have this controller actually be 2 controllers, which basically do the same thing: one for the admin, and one for the times we need to render editor stuff out on the frontend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer. This way a change in the admin can't break things in the front-end, and the front-end view should be very limited. The front-end controller should also be very limited in terms of features.
…out looking for its localization. # Conflicts: # src/Orchard.Web/Modules/Orchard.Taxonomies/Controllers/LocalizedTaxonomyController.cs
…taxonomies back office calls. # Conflicts: # src/Orchard.Web/Modules/Orchard.Taxonomies/Controllers/LocalizedTaxonomyController.cs # src/Orchard.Web/Modules/Orchard.Taxonomies/Views/EditorTemplates/Fields/TaxonomyFieldList.cshtml
As suggested by @sebastienros , I duplicated the controller in order to have both a front-end and a admin controller. The latter inherits from the former to avoid having the GetTaxonomy function in both classes with the same code. |
We found and fixed a few issues with some of the "automations" related to localizations for TaxonomyFields and BlogPosts.
TaxonomyFields would correctly update their editor UI when selecting a new culture, because the controller action wasn't called when it should have been. Moreover, we added an
isAdmin
parameter so we can correctly always use shapes from an admin theme when appropriate.In BlogPosts, we were comparing Culture records rather than values: as a consequence, when creating a new localization, since the comparison ended up being between different instances of the same culture, the process failed to correctly "move" the translated BlogPost into its correct container.
This PR contains fixes for both.