Skip to content

Conversation

KuceraMartin
Copy link
Contributor

@KuceraMartin KuceraMartin commented Jul 3, 2017

This change is Reviewable

@KuceraMartin
Copy link
Contributor Author

ping @JanTvrdik

}


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