Skip to content

Add support for grouped choices. #3108

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
Closed
wants to merge 2 commits into from
Closed

Add support for grouped choices. #3108

wants to merge 2 commits into from

Conversation

maxpeterson
Copy link
Contributor

Closes #1533.

This also adds support for mixing single and paired choices (which may not be desirable):

[
    ('poor', 'Poor quality'),
    'medium',
    ('good', 'Good quality'),
]

@maxpeterson
Add support for grouped choices.
56b3f19 
This also adds support for mixing single and paired choices:
```
[
    ('poor', 'Poor quality'),
    'medium',
    ('good', 'Good quality'),
]
```
@jpadilla
Copy link
Member

jpadilla commented Jul 3, 2015

Looking good, there's just a few tests failing for Django 1.4 / Python 2.6.

Other than that, I think we could with flatten_choice being a utility method instead of adding it as a public method to that class. @tomchristie thoughts?

@maxpeterson
Copy link
Contributor Author

A utility makes more sense, I will amend it and look at the 1.4 tests.

tomchristie
tomchristie reviewed Jul 3, 2015
View reviewed changes
rest_framework/fields.py
else:
self.choices = OrderedDict([(item, item) for item in choices])
flat_choices = [flatten_choice(c) for c in choices]
self.choices = OrderedDict(list(itertools.chain(*flat_choices)))
Copy link
Member

Choose a reason for hiding this comment

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

Should we be leaving the .choices attribute alone, but adding a .flatchoices attribute?
What's the right way for us to access the grouping information, if we need to (eg for displaying a grouped select in the browsable API)

Copy link
Member

Choose a reason for hiding this comment

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

Irrespective of my comment above - this would be an improvement either way, so it's possible we should accept this, and consider any further enhancements seperately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, it seems likely to an issue that we have lost the group information. I suspect that keeping the .choices flat will reduce further refactoring. How about about adding a new .choices_list property that stores the choices list, that was originally passed in?

@maxpeterson
Copy link
Contributor Author

Do you wan't me to make any changes to this?

I looked into using .flatchoices for the flat version of the choices, in a similar way to django.db.models.fields. This would imply that the .choices would not always be flat which seems like a significant change.

For the following grouped choices:

[
     (
         'Category',
         [
             ('value1', 'name1'),
             ('value2', 'name2'),
         ],
     ),
     (
         'Category2',
         [
             ('value3', 'name3'),
         ],
     ),
]

We could have something like:

.choices =  [
            {
                'display_name': 'Category',
                'choices': [
                    {
                        'value': 'value1',
                        'display_name': 'name1'
                    },
                    {
                        'value': 'value2',
                        'display_name': 'name2'
                    },
                ],
            },
            {
                'display_name': 'Category2',
                'choices': [
                    {
                        'value': 'value2',
                        'display_name': 'name2'
                    },
                ],
            },
]

.flatchoices = [
    {
        'value': 'value1',
        'display_name': 'name1'
    },
    {
        'value': 'value2',
        'display_name': 'name2'
    },
    {
        'value': 'value2',
        'display_name': 'name2'
    },
]

For non-grouped choices the .flatchoices and .choices would be the identical.

My concern is that most places (including the templates) would need to be updated to use .flatchoices or updated to output grouped choices, radios, ....

Since the grouped choices appears to be a bit broken currently, I guess this will not break existing usage.

@tomchristie
Copy link
Member

Thanks for all your work on this @maxpeterson!

I've picked this up, and completed the work required for rendering these in templates too. Now in #3225 and to be released as part of 3.2.0.

Nice work 😄

@tomchristie tomchristie closed this Aug 6, 2015
@tomchristie tomchristie mentioned this pull request Aug 6, 2015
Merged
@maxpeterson
Copy link
Contributor Author

@tomchristie thanks, glad it was of use.

@maxpeterson maxpeterson mentioned this pull request Sep 23, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Needs further review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@maxpeterson @jpadilla @tomchristie