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.

Sign up for GitHub

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

Jump to bottom

Add default count parameter on transchoice #185

Merged
merged 1 commit into from
Dec 16, 2016
Merged

Add default count parameter on transchoice #185

merged 1 commit into from
Dec 16, 2016

Conversation

pyrech
Copy link
Contributor

@pyrech pyrech commented Nov 18, 2016 •
edited
Loading

The idea is to have a similar behavior between Symfony translator (TranslationExtension and symfony/symfony#19795) and the js translator.

@pyrech pyrech force-pushed the feature/transchoice-count-parameter branch 5 times, most recently from b259a6f to e2f62d6 Compare November 18, 2016 13:52
@pyrech
Copy link
Contributor Author

pyrech commented Nov 25, 2016

Hi @monteiro and @willdurand :) Will you have a look at this PR soon?

stof
stof reviewed Nov 25, 2016
View reviewed changes
Resources/js/translator.js
@@ -149,6 +149,11 @@
);

var _number = parseInt(number, 10);
parameters = parameters || {};

if (!parameters.count) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is wrong. You need to check whether it is defined or no. 0 should not be replaced if it is passed explicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, fixed now

Resources/js/translator.js
@@ -149,6 +149,11 @@
);

var _number = parseInt(number, 10);
parameters = parameters || {};

if (!parameters.count) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please also add tests covering the case where the count is provided explicitly too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@pyrech pyrech force-pushed the feature/transchoice-count-parameter branch from e2f62d6 to 13b5044 Compare November 25, 2016 10:04
@pyrech pyrech closed this Nov 30, 2016
@pyrech pyrech reopened this Nov 30, 2016
@willdurand
Copy link
Owner

looks good to me!

@monteiro
Copy link
Collaborator

monteiro commented Dec 4, 2016

@pyrech thanks for the pr! The tests seems to be failing :( can you fix it? So we can merge this.

@pyrech
Copy link
Contributor Author

pyrech commented Dec 4, 2016 •
edited
Loading

@monteiro Tests were working before I rebased the PR. The error is Asset support cannot be enabled as the Asset component is not installed. The main difference I see is that Circle now installs Symfony 3.2 so I guess it looks like the test suite is not compatible with the latest release of Symfony

@monteiro
Copy link
Collaborator

monteiro commented Dec 6, 2016 •
edited
Loading

I see what you mean @pyrech

Let me fix it.

@monteiro
Copy link
Collaborator

monteiro commented Dec 7, 2016 •
edited
Loading

@pyrech we are already fixing the tests problem in PR #186 After this is merged we make the tests green again.

@monteiro
Copy link
Collaborator

@pyrech Can you rebase with master, so we can merge this PR with tests passed? Thanks a lot!

@pyrech pyrech force-pushed the feature/transchoice-count-parameter branch from 13b5044 to c7a67f3 Compare December 16, 2016 16:12
@pyrech pyrech force-pushed the feature/transchoice-count-parameter branch from c7a67f3 to b36144c Compare December 16, 2016 16:13
@pyrech
Copy link
Contributor Author

pyrech commented Dec 16, 2016

Cool 👍 Just rebased the PR on top of master 😉

@monteiro monteiro merged commit 575e838 into willdurand:master Dec 16, 2016
@monteiro
Copy link
Collaborator

Thanks a lot!

@pyrech pyrech deleted the feature/transchoice-count-parameter branch December 16, 2016 16:38
@pyrech
Copy link
Contributor Author

pyrech commented Dec 16, 2016

Awesome, you're welcome 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pyrech @willdurand @monteiro @stof