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

Translation texts with namespaces #82

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

KuceraMartin
Copy link
Contributor

@KuceraMartin KuceraMartin commented Jul 3, 2017

This change is Reviewable

@KuceraMartin
Copy link
Contributor Author

ping @JanTvrdik

@@ -256,10 +262,9 @@ public function getTranslator()

public function translate($s, $count = null)
Copy link
Member

Choose a reason for hiding this comment

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

is this method still needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it is still used at line 115

src/Datagrid.php Outdated
public function __construct()
{
parent::__construct();
$this->translator = new DefaultTranslator(DefaultTranslator::LANG_EN);
Copy link
Member

Choose a reason for hiding this comment

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

what about making this lazy by getTranslator()

src/Datagrid.php Outdated
if ($this->translator) {
$form->setTranslator($this->translator);
}
$form->setTranslator($this->translator);
Copy link
Member

Choose a reason for hiding this comment

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

if the translator would be lazy, we would probably set it only for render, eg. in render method.

const LANG_CS = 'cs';

const TRANSLATIONS = [
self::LANG_EN => [
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I want support more langs :|

Copy link
Member

Choose a reason for hiding this comment

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

Well, czech is quite popular language, I've heard =)

@@ -0,0 +1,20 @@
<?php
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO it's more appropriate to split these into single files

@simPod
Copy link
Contributor

simPod commented Aug 21, 2017

Just a question, do we have a roadmap for this PR? :) Are there any edits needed to be made or is this necessary to be reworked completely?

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