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

Content Localization Part #3380

Merged
merged 36 commits into from May 7, 2019

Conversation

Projects
None yet
5 participants
@jptissot
Copy link
Contributor

commented Mar 23, 2019

Fixes #2081

Creating this PR for feedback.
Content linking via LocalizationSet seems to work.
There are probably lots of edge cases I missed.
The LocalizationPart UI need some love.

@agriffard

This comment has been minimized.

Copy link
Member

commented Mar 24, 2019

I started from a blank site, enable the feature and added the LocalizationPart on a Page.

Here are a few remarks after testing the Content Localization features:

First, I tested without any cultures so the System culture is the default one in the general Settings.
When I create a new page, an empty Content Culture list is displayed.
Then, I added the English culture.
Should we display it (or a warning) if no culture is set or only one?
if French is added and set as default, English is still displayed first, because it is sorted alphabetically I guess.

In Edit, the list is displayed as disabled.
Shouldn't we display a label instead or hide it, as it will appear in the list below?

The culture code and the English culture name are displayed.
What should description we display?
We could display it as Bootstrap list group, with a special class for the currently selected culture and may be in a collapsible panel.

In the Content items list:
Should we display the culture in each content summary that has a LocalizationPart?
If the feature is enabled, should we display a Culture Filter?

@agriffard

This comment has been minimized.

Copy link
Member

commented Mar 24, 2019

Have you planned to add a Culture Layer rule to be able to only display some widgets depending on the current culture?

As an example, I tried to add a Widget stereotype to the Menu content type.
We need to think about it because when people will want to display a different navigation based on the culture, they will probably do like this (and find a way to remove the menu if it has been added manually in the Layout view).

It seems that an error occurs when you add a Widget.
The first time, this Exception appears:

NullReferenceException: Object reference not set to an instance of an object.
OrchardCore.ContentLocalization.Records.LocalizedContentItemIndexProvider+<>c.<Describe>b__0_0(ContentItem contentItem) in LocalizedContentItemIndex.cs, line 26

NullReferenceException: Object reference not set to an instance of an object.
                    if (string.IsNullOrEmpty(localizationPart.LocalizationSet))

and then, it is impossible to add a Widget anymore in a Layer.

@jptissot

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

I have been super busy lately but I plan on working on this sometime this week.

jptissot added some commits Apr 6, 2019

LocalizationPart improvements
- Message when no Clutures with link to add cultures.
- Better handle case where culture is deleted
- Rudimentary display of culture in SummaryAdmin
- Fix NRE with Index

@jptissot jptissot force-pushed the jptissot:localization branch from 2b53c33 to 807ec09 Apr 6, 2019

@jptissot

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2019

Antoine, Thanks for your comments! Did a bit more work on this tonight.

I started from a blank site, enable the feature and added the LocalizationPart on a Page.

Here are a few remarks after testing the Content Localization features:

First, I tested without any cultures so the System culture is the default one in the general Settings.
When I create a new page, an empty Content Culture list is displayed.

Fixed, now displaying System culture with link to create cultures.

Then, I added the English culture.
Should we display it (or a warning) if no culture is set or only one?

Fixed I think

In the Content items list:
Should we display the culture in each content summary that has a LocalizationPart?

Added a driver for the SummaryAdmin. Needs some placement/ styling.

I also fixed the exception with the index when attaching a Widget

@jptissot

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2019

We should probably also group all localized ContentItems in Admin/Contents/ContentItems view with a link to each localization.

@agriffard

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

Thank you @jptissot .
There aren't errors anymore.

The display of the summary admin and the list in details can still be improved.

The Deleted label that appears next to a culture that has been removed is a little disturbing.

The link to the culture settings can be handy but once the cultures are set, it won't be used anymore so is it really necessary?.

As explained, have you considered to add a Culture Layer rule that you can use in the layers?

@jptissot

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

@agriffard Thanks!

Yeah, I am mostly focused on functionality right now so expect ugly UI ;)

What do you recommend we do about deleted cultures that have localized items? That is what the Deleted label is meant to represent. I agree with you though it makes it seem like the ContentItem was deleted. I am not sure it is relevant to display this information.

The link to the culture settings can be handy but once the cultures are set, it won't be used anymore so is it really necessary?.

I can remove that link.

Do you find the Dropdown that allows selecting Culture of the Current Item useful ?

How would you style the list of links to other cultures ?

I did not consider Culture Layer rule yet. Will investigate what those are as I never used them. I am going to look to develop the CulturePicker soon.

@jptissot

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

@agriffard I made some UX improvements and a couple bugfixes (if a localized content item was deleted for example, the index behaved strangly) Let me know what you think ! Onto the CulturePicker next!

@agriffard

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

Do we really want to allow to change the culture in Edit by providing a list of the cultures not already localized?

If we add some css (for the accordion), it will be better to have it in a distinct file, not in the view.

About the CulturePicker, will it be a Widget than you can add in the layer you want?

@Skrypt

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

I pushed my changes on a different branch please review them. I merged latest dev on that branch.
https://github.com/OrchardCMS/OrchardCore/tree/skrypt/content-localization

The issues I fixed are related to supporting content items that have no LocalizationPart set. Those content items are now set to InvariantCulture. On the UI I've added the option to set a content item to InvariantCulture because we need to be able to fallback to that option too.

I also found that when I was saving a content item it was always creating a new LocalizationSet id. This Id is now set directly in the DefaultContentLocalizationManager.LocalizeAsync method.

Content items that have InvariantCulture are now also part of the LocalizationSet without being saved in the LocalizedContentItemIndex table. The LocalizationSet is only set in the Document table to keep track of which set it belongs to.

@Skrypt

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

Will make a change here. Instead of using InvariantCulture we should maybe use the System Default Culture and not the Tenant default culture.

@Skrypt

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

After thinking about this we should never have a content item set to InvariantCulture since our frontend website is always displayed on a specific culture. The culture fallback mechanism that we use with the tenant culture setting should be replicated.

Content Item culture > culture site setting > system (OS) culture setting.

So, if someone has content items that have no LocalizationPart attached it should always be assigned to a default culture using this order. I will also had something on the tenant culture settings hint to inform people that they should specify a culture manually if they want to always get the same behavior from every OS the website is developped or served on.

Make sure that a content item culture is never null or InvariantCulture.
Now use the same fallback mechanism than the site culture settings which will fallback to system culture when none is set manually.
@Skrypt

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

Ok I pushed an update to https://github.com/OrchardCMS/OrchardCore/tree/skrypt/content-localization
Maybe we should now merge it on the PR here if you guys approve the changes.

@Skrypt Skrypt referenced this pull request Apr 27, 2019

Closed

Content Localization #3525

@Skrypt Skrypt closed this Apr 27, 2019

@hishamco

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

Note from DNN

You can’t change the default site language after enabling content localization, so make sure you have selected the correct default language before enabling the content localization.

I vote for this suggestion because it will save us from a lot of issues and scenarios if the user will be able to change the default culture after enabling content localization module

@hishamco

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

Few suggestion:

  • Localize the content when it's saved with the current default culture, no need for two steps, save and localize

  • Change the way of localizing the content, it's too wired from UI/UX perspective, I prefer shows flags or cultures in badge (I can share a picture for what it looks like my local version)

  • Manage Urls, if this is done with AutoRoute it will be good otherwise, we need sort of UrlRequestCultureProvider

  • Group the content items by culture, this will be great for site editors, they can see which content is not translated yet .. again with flags will be obvious

@hishamco

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

Is there a way to push some commits here? or I need to fork @jptissot repo and send a PR?

@hishamco

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

Localization1

Localization2

@sebastienros

This comment has been minimized.

Copy link
Member

commented May 5, 2019

Maybe DNN can't change their default culture because they use a default value like null for it. We talked about it during the meeting. We don't have this issue with the current design.

save us from a lot of issues and scenarios if the user will be able to change the default culture after enabling content localization module

Start by sharing "one" issue and we can talk about it ;)

@sebastienros

This comment has been minimized.

Copy link
Member

commented May 5, 2019

The design with the flags looks nice, an maybe we can switch to the drop down version only after a specific number of cultures. Usually sites should handle 2-3 languages and that would be better. I assume the flags are always displayed and grayed out when the content is not yet localized to this culture?

@hishamco

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

I assume the flags are always displayed and grayed out when the content is not yet localized to this culture?

Exactly

Start by sharing "one" issue and we can talk about it ;)

Me as a site administrator I expect when I add a content after enable content localization that the added content is localized to the default culture not null or empty string, I am not sure I need an extra step and choose my default culture?!!

@sebastienros

This comment has been minimized.

Copy link
Member

commented May 5, 2019

What you'd expect is what we do. So there are no issues and we can change the default culture if we need without issues.

@jptissot

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

I don't mind flags but what about when a country has two languages (en-ca, fr-ca) ?

@jptissot

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

How about I fix the issues Sébastien mentioned in his review and we can have a follow up ticket for ux?

@hishamco

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

what about when a country has two languages (en-ca, fr-ca) ?

Good point .. we should display the region which is Canada, but we can display the language on hover

BTW all the UI changes is already done but I don't know how can I participate on this PR

sebastienros added some commits May 6, 2019

@sebastienros

This comment has been minimized.

Copy link
Member

commented May 7, 2019

I am done with the changes. Would appreciate that you all try it and ensure I didn't break much of it ;)

Regarding flags, I was googling about it and found this site: http://www.flagsarenotlanguages.com/blog/
It has lots of posts looking at websites and how they represent locales. My overall conclusion is that flags are hard at conveying locales. But read them by yourself.

@jptissot

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

@sebastienros I tested the following use cases and they work:

  • New blog tenant, attach LocalizationPart to BlogPost, Save, Localize.
  • New BlogPost when no default clulture is assigned (using system culture)
  • Change default culture then try to create / edit currently localized Content Items

I made a couple changes, cleaned the code a little bit.

Is there a clean way to add a modal dialog saying that you will loose your changes when clicking on the Edit / Create localized content buttons ?

TODO: check in O1 if we should reassign the Owner with the current user. (on the clone event)

Did you still want to do this ?

@hishamco

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

Is there a clean way to add a modal dialog saying that you will loose your changes when clicking on the Edit / Create localized content buttons ?

Check the bootstrap modal that I created in the admin

@sebastienros sebastienros merged commit aa8448c into OrchardCMS:dev May 7, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details

@jptissot jptissot deleted the jptissot:localization branch May 7, 2019

@jptissot jptissot referenced this pull request May 8, 2019

Closed

Copying of content items? #2773

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.