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

gedmo: allow to switch content language on list and filter by language #84

Closed
wants to merge 1 commit into from

Conversation

dbu
Copy link
Contributor

@dbu dbu commented Mar 30, 2016

fix #81, fix #83

continuation of #83

this can be merged after 1.0.2 has been tagged - its a new feature.

@dbu
Copy link
Contributor Author

dbu commented Mar 30, 2016

@mcrinquand something with rebasing seems to have gone wrong. i took your branch and cleaned it up. i also added that this will bump the version to 1.1 as its a new feature. i wonder if we might need an option to disable this feature... but well, if somebody needs to disable it, they could do a PR to add that option...

i will wait with merging until i tagged 1.0.2 which hopefully will happen soon.

}
}

if ($aliasAlreadyExists === false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

$aliasAlreadyExists is a boolean, I think you can use it directly in the condition, which is what booleans are made for, so if (!$aliasAlreadyExists), which sounds closer to plain english

@dbu dbu modified the milestone: 1.1 Mar 30, 2016

{% if (admin.class is translatable) %}
{% for extension in admin.extensions %}
{% if(extension.translatableLocale is defined) %}
Copy link
Contributor

Choose a reason for hiding this comment

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

missing space after the if

*
* @return bool|null
*/
public static function translationFieldFilter(ProxyQuery $queryBuilder, $alias, $field, $value)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

looking at the code this seems to never be used. if the user needs to do something to activate the filter, can you please add some documentation for this @mcrinquand ?

@mcrinquand
Copy link
Contributor

Yes @dbu I'll update the documentation.

@dbu
Copy link
Contributor Author

dbu commented Mar 30, 2016

thanks. can you take this branch translate-gedmo-lists and do the pull request against it instead of master? that way we can keep the changes together and i can already merge the doc update.

@dbu
Copy link
Contributor Author

dbu commented Apr 19, 2016

ping @mcrinquand

@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

@soullivaneuh
Copy link
Member

@dbu The PR should be re-opened against master.

Could you also please do your work from a fork? Working directly on the repo would be a problem with StyleCI sending auto fix PR.

I'm closing this one.

Thanks.

@dbu
Copy link
Contributor Author

dbu commented Jun 7, 2016

@mcrinquand any chance you can pick this up and finalize it?

public function configureQuery(AdminInterface $admin, ProxyQueryInterface $query, $context = 'list')
{
$this->getTranslatableListener($admin)->setTranslatableLocale($this->getTranslatableLocale($admin));
$this->getTranslatableListener($admin)->setTranslationFallback('');
Copy link
Member

Choose a reason for hiding this comment

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

From Scrutinizer:

'' is of type string, but the function expects a boolean.

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.

None yet

5 participants