Skip to content

improved description of choice_list option of Choice form type #5052

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

Closed
wants to merge 1 commit into from

Conversation

adamziel
Copy link

@adamziel adamziel commented Mar 2, 2015

Current example could be a bit confusing since rendered HTML would look like this:

<select>
    <option value="0">Full</option>
    <option value="1">Half</option>
</select>

Which could easily be confued with truncated floats. In other words this is very similar to what would happen if choices option was set to array(1=>'Full', 0.5=>'Half'). This pull request is an attempt to give a clue "a-ha, it's an index and not a float representation".

@dupuchba
Copy link

dupuchba commented Mar 2, 2015

👍

@wouterj
Copy link
Member

wouterj commented Mar 25, 2015

👍

<option value="1">Half</option>
<option value="2">Almost empty</option>
</select>

Copy link
Member

Choose a reason for hiding this comment

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

This is a nice addition, because as I was reading this, I was in fact confused as you described in the PR description :). Can we add one more line after this? I'd like something that basically says: "But don't be confused! If Full is selected (value 0 in HTML), 1 will be returned in your form. If Almost empty is selected (value 2 in HTML), 2 will be returned".

@xabbuh
Copy link
Member

xabbuh commented May 23, 2015

@adamziel Will you be able to make the change requested by @weaverryan?

@javiereguiluz
Copy link
Member

I'm finishing this PR in #5428.

@adamziel don't worry about not having finished your PR. This happens sometimes. I've taken your work and finished in another PR. I've also reused all your original commits so you get the credit for your work. Thanks.

@weaverryan
Copy link
Member

Great - thank you @javiereguiluz! The commit from @adamziel will be merged in with #5428 :)

@weaverryan weaverryan closed this Jun 23, 2015
@adamziel
Copy link
Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants