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

[WIP] - Fix title translation #167

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

[WIP] - Fix title translation #167

wants to merge 2 commits into from

Conversation

cekk
Copy link
Member

@cekk cekk commented Jan 14, 2020

No description provided.

@mister-roboto
Copy link

@cekk thanks for creating this Pull Request and help improve Plone!

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass.

Whenever you feel that the pull request is ready to be tested, either start all jenkins jobs pull requests by yourself, or simply add a comment in this pull request stating:

@jenkins-plone-org please run jobs

With this simple comment all the jobs will be started automatically.

Happy hacking!

@cekk
Copy link
Member Author

cekk commented Jan 14, 2020

Title were always create with the default label and not translated into current language.

Should i have to create an upgrade-step to update already created comments?

@tisto tisto self-requested a review February 20, 2020 15:04
@tisto
Copy link
Member

tisto commented Feb 20, 2020

@cekk an upgrade step would be nice if you have the time for it.

@mauritsvanrees
Copy link
Member

Is this still Work In Progress?
Just from looking at the changes, it seems good and complete, and should not need an upgrade step. I have not tried it out yet.

@mauritsvanrees
Copy link
Member

Actually, I am not sure how to test this. With plone.app.discussion master in a Dutch site I enabled comments on Page, added an anonymous comment without any name, and the name shown was "Anoniem", the Dutch translation of "Anonymous". So this part is working, but maybe the Title is something else.
Or does this need a multilingual site to properly test?

Ah, I see: the way to test this, is to go to the search control panel and allow searching comments. Then I see in the search results a title like "Anonymous on Page Title", where "Anonymous on" would be translated in Dutch with your fix. And an upgrade step would then be needed to make this active.
That is a corner case, but I guess some people do use it this way.

There are various spots in the same comment.py file where translate is called without context, so those should be fixed too, preferably.
Specifically, a few lines down from your fix, the next translate call needs this, otherwise I see in Dutch "Anoniem on Page Title", where "on" is not translated into the Dutch "over".

Actually, when I add the context to this call, and rebuild the catalog, the search results show "${creator} over Page Title". So this literally contains ${creator}.

Okay, thanks for starting this, but this indeed needs more work, so the "WIP" in the title is correct. :-)
BTW, you can revert the PR into draft status if you want. There is a link below the reviewers.

Copy link
Member

@mauritsvanrees mauritsvanrees left a comment

Choose a reason for hiding this comment

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

Changes requested, see my latest comment.

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.

4 participants