Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
avoid rendering theChoiceType separator if allchoices arepreferred_choices#1787
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
fabpot commentedJul 24, 2011
The same change should be made to the PHP template. |
fabpot commentedJul 25, 2011
I forgot to ask you to add some unit tests too. Thanks. |
craue commentedJul 25, 2011
Are you asking for PHPUnit tests? If so, unfortunately, I'm not able to add those because I haven't used PHPUnit yet. ;) |
lenar commentedJul 25, 2011
I would prefer |
lenar commentedJul 25, 2011
@fabpot: Since is unclickable in browser (by HTML spec) this really doesn't change anything (something not there is as unclickable) except the the look when rendered. I have hard time to imagine what could become unit-testable here by this change. |
stof commentedJul 25, 2011
@lenar unit testing is not about what the browser could do. What should be unit-tested is that an example will only preferred choices does not output the separator, which is exactly what this PR is about |
1 similar comment
stof commentedJul 25, 2011
@lenar unit testing is not about what the browser could do. What should be unit-tested is that an example will only preferred choices does not output the separator, which is exactly what this PR is about |
lenar commentedJul 25, 2011
@stof: ok, put this way you are definitely right. |
craue commentedJul 25, 2011
@lenar: You're right about the spaces. I'm using them in my projects but will remove them here for the sake of consistency. |
stloyd commentedJul 25, 2011
@craue I will write today/tomorrow test to cover your code and send you PR. |
craue commentedJul 25, 2011
@stloyd: That would be nice. But I'm still not that familiar with Git(Hub). Is there anything I have to take care of? Also, I'd like to squash my three commits into one ... if this is possible for an open PR and if I find out how to do that easily. :D |
fabpot commentedJul 26, 2011
@craue: yes, you should squash your commits into one and use |
Commits-------c558b78 avoid rendering the `ChoiceType` separator if all `choices` are `preferred_choices`Discussion----------avoid rendering the `ChoiceType` separator if all `choices` are `preferred_choices`---------------------------------------------------------------------------by fabpot at 2011/07/24 00:51:21 -0700The same change should be made to the PHP template.---------------------------------------------------------------------------by fabpot at 2011/07/25 00:31:39 -0700I forgot to ask you to add some unit tests too. Thanks.---------------------------------------------------------------------------by craue at 2011/07/25 10:23:34 -0700Are you asking for PHPUnit tests? If so, unfortunately, I'm not able to add those because I haven't used PHPUnit yet. ;)---------------------------------------------------------------------------by lenar at 2011/07/25 12:47:51 -0700I would prefer ```choises | length``` without spaces as everywhere else.---------------------------------------------------------------------------by lenar at 2011/07/25 12:50:32 -0700@fabpot: Since <option disabled> is unclickable in browser (by HTML spec) this really doesn't change anything (something not there is as unclickable) except the the look when rendered. I have hard time to imagine what could become unit-testable here by this change.---------------------------------------------------------------------------by stof at 2011/07/25 13:03:47 -0700@lenar unit testing is not about what the browser could do. What should be unit-tested is that an example will only preferred choices does not output the separator, which is exactly what this PR is about---------------------------------------------------------------------------by stof at 2011/07/25 13:04:03 -0700@lenar unit testing is not about what the browser could do. What should be unit-tested is that an example will only preferred choices does not output the separator, which is exactly what this PR is about---------------------------------------------------------------------------by lenar at 2011/07/25 13:08:33 -0700@stof: ok, put this way you are definitely right.---------------------------------------------------------------------------by craue at 2011/07/25 13:37:50 -0700@lenar: You're right about the spaces. I'm using them in my projects but will remove them here for the sake of consistency.---------------------------------------------------------------------------by stloyd at 2011/07/25 13:40:40 -0700@craue I will write today/tomorrow test to cover your code and send you PR.---------------------------------------------------------------------------by craue at 2011/07/25 14:00:26 -0700@stloyd: That would be nice. But I'm still not that familiar with Git(Hub). Is there anything I have to take care of?Also, I'd like to squash my three commits into one ... if this is possible for an open PR and if I find out how to do that easily. :D---------------------------------------------------------------------------by fabpot at 2011/07/26 00:18:22 -0700@craue: yes, you should squash your commits into one and use `--force` when you push (the PR will automatically be updated accordingly).
No description provided.