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

[4.0] Fix Untranslated tip #27174

Merged
merged 11 commits into from
Dec 4, 2019

Conversation

infograf768
Copy link
Member

Summary of Changes

As title says. The tip displays in the articles Manager when an article is Unpublished.
It concerns the COM_CONTENT_CHANGE_STAGE lang string

Testing Instructions

Create an article as Unpublished.
make sure publish up and down are empty
Hover the Status icon.

Before patch

Screen Shot 2019-11-28 at 11 06 52

After patch

Screen Shot 2019-11-28 at 11 05 09

About this patch

It is the simplest solution but we have another solution by passing through Text the string in both
articles/default.php and featured/default.php

													->addState(ContentComponent::CONDITION_PUBLISHED, '', 'publish', 'COM_CONTENT_CHANGE_STAGE', ['tip_title' => 'JPUBLISHED'])
													->addState(ContentComponent::CONDITION_UNPUBLISHED, '', 'unpublish', 'COM_CONTENT_CHANGE_STAGE', ['tip_title' => 'JUNPUBLISHED'])
													->addState(ContentComponent::CONDITION_ARCHIVED, '', 'archive', 'COM_CONTENT_CHANGE_STAGE', ['tip_title' => 'JARCHIVED'])
													->addState(ContentComponent::CONDITION_TRASHED, '', 'trash', 'COM_CONTENT_CHANGE_STAGE', ['tip_title' => 'JTRASHED'])
													

I can change the PR if judged necessary.

@SharkyKZ
Copy link
Contributor

another solution by passing through Text the string in both
articles/default.php and featured/default.php

That's better. But I wonder whether passed strings should be translated or not. Here they're not translated:

$this->addState(1, 'unpublish', 'publish', 'JLIB_HTML_UNPUBLISH_ITEM', ['tip_title' => 'JPUBLISHED']);
$this->addState(0, 'publish', 'unpublish', 'JLIB_HTML_PUBLISH_ITEM', ['tip_title' => 'JUNPUBLISHED']);
$this->addState(2, 'unpublish', 'archive', 'JLIB_HTML_UNPUBLISH_ITEM', ['tip_title' => 'JARCHIVED']);
$this->addState(-2, 'publish', 'trash', 'JLIB_HTML_PUBLISH_ITEM', ['tip_title' => 'JTRASHED']);

So maybe translation should be performed in Joomla\CMS\Button\PublishedButton::render() instead.

@infograf768
Copy link
Member Author

Afaik that part of PublishButton is not used for articles but may be for other managers and it is translated somewhere else. The articles manager loads a new PublishButton with specific states because of workflow. Then it is displayed by the transition-button where the textip (data content) depends of the state of the article obtained by Publish button when the article is published.
When it is unpublished, we need to use this pr or add Text:: in the default.php.
So we have to decide which solution to use OR refactor the whole stuff.

@SharkyKZ
Copy link
Contributor

It's not translated as far as I can tell. After removing custom com_content states, I get JLIB_HTML_PUBLISH_ITEM in the tooltip which is what other components will get with default button.

@infograf768
Copy link
Member Author

infograf768 commented Nov 29, 2019

It's not translated as far as I can tell.

Indeed. In fact it looks like it is never used in core... What is used elsewhere is JGrid.
Only occurence of new PublishedButton are in both articles/default.php and featured/default.php

Looks like it "could" be used by 3rd party in the future.

Therefore I think it is safer to add the Text:: both in the default.php files and PublishedButton itself. On it.

@infograf768
Copy link
Member Author

Please test again.

@SharkyKZ
Copy link
Contributor

Grrrr... Now it looks weird because title has to be translated but tip_title doesn't 🤷‍♂.

@infograf768
Copy link
Member Author

@SharkyKZ
I found where the original issue comes from.
It was my patch #27078 ;)
I had modified the transition-button there to prevent double translation when we had "Start Date etc.)
Debug Lang was marking it this way:
Screen Shot 2019-11-30 at 12 22 40

I patched this way

- data-content="<?php echo HTMLHelper::_('tooltipText', Text::_($title), '', 0); ?>"
+ data-content="<?php echo HTMLHelper::_('tooltipText', $title, '', 0); ?>"

Therefore the $title is untranslated when the article is unpublished.

We have to take a decision. Do we reinstate the Text there (and get a wrong debug lang) or modify as done in this PR?

@SharkyKZ
Copy link
Contributor

I just don't like that one string we pass has to be translated but the other doesn't. I see two options here:

  1. Remove translation from button layout and pass only translated strings to PublishedButton::addState().

  2. Remove translation from button layout and perform all translations in PublishedButton::render().

@infograf768
Copy link
Member Author

I'm afraid I can't do what you suggest.

@SharkyKZ
Copy link
Contributor

SharkyKZ commented Dec 2, 2019

You mean you don't know how to do it or you don't agree with proposed solutions?

@infograf768
Copy link
Member Author

Don’t know how. 😄

@SharkyKZ
Copy link
Contributor

SharkyKZ commented Dec 2, 2019

Remove Text::_() calls from layouts/joomla/button/transition-button.php and add them to tip_title constants like you already did for title, e.g. here

->addState(ContentComponent::CONDITION_PUBLISHED, '', 'publish', Text::_('COM_CONTENT_CHANGE_STAGE'), ['tip_title' => 'JPUBLISHED'])

This should also fix remaining double translations which aren't visible in core.

@infograf768 infograf768 force-pushed the 4.0_untranslated_stage_string branch from b264d75 to 24d59bc Compare December 3, 2019 08:22
@infograf768
Copy link
Member Author

@SharkyKZ
Done and it does work indeed (changes were also needed in PublishedButton).

BUT, remains a case.
See transition-button $only_icon = empty($options['transitions']);
https://github.com/joomla/joomla-cms/blob/4.0-dev/layouts/joomla/button/transition-button.php#L31-L40

I did not touch for the moment at the Text::_() stuff when $only_icon is set as I don't know how to test that part. The logic is to also modify there but not sure at all.

If you don't know, maybe @bembelimen knows the uses of that part.

@SharkyKZ
Copy link
Contributor

SharkyKZ commented Dec 3, 2019

Yes, remove those Text::_() calls too. It works the same way. For testing purposes you can just set $only_icon to true.

@infograf768
Copy link
Member Author

Done. Can be now tested. I fail to understand the use of $only_icon. Why would anyone lose the ability to modify via the icon is a mystery to me...

@SharkyKZ
Copy link
Contributor

SharkyKZ commented Dec 3, 2019

Remove use Joomla\CMS\Language\Text; from button layout please.

@infograf768
Copy link
Member Author

Done.

@infograf768
Copy link
Member Author

needs more work for featured.
On it

@infograf768 infograf768 force-pushed the 4.0_untranslated_stage_string branch from 09c7d6d to 18a1654 Compare December 3, 2019 13:05
@SharkyKZ
Copy link
Contributor

SharkyKZ commented Dec 3, 2019

Remove Text::_() here too please:

<span class="sr-only"><?php echo Text::_($title); ?></span>

And with that use Joomla\CMS\Language\Text; too.

@infograf768 infograf768 force-pushed the 4.0_untranslated_stage_string branch from 0f6751f to 4c89c16 Compare December 3, 2019 16:07
@infograf768
Copy link
Member Author

LOL
Hope it was the last one...

@SharkyKZ
Copy link
Contributor

SharkyKZ commented Dec 3, 2019

I have tested this item ✅ successfully on 0318b14


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/27174.

@Quy
Copy link
Contributor

Quy commented Dec 3, 2019

I have tested this item ✅ successfully on 0318b14

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/27174.

@Quy Quy added the RTC This Pull Request is Ready To Commit label Dec 3, 2019
@wilsonge wilsonge merged commit 3ae2484 into joomla:4.0-dev Dec 4, 2019
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Dec 4, 2019
@wilsonge
Copy link
Contributor

wilsonge commented Dec 4, 2019

This looks better to me translating the tips this way (sorry for not looking before JM!). Thanks!

@wilsonge wilsonge added this to the Joomla 4.0 milestone Dec 4, 2019
@infograf768 infograf768 deleted the 4.0_untranslated_stage_string branch December 4, 2019 11:18
@infograf768
Copy link
Member Author

Thanks to all who helped on this. Was a long road... ;)
I still have no idea why we offer the variable $only_icon though except for overrides of the admin templates.

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.

5 participants