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

Fix OrderingFilter label translation #546

Merged

Conversation

rpkilby
Copy link
Collaborator

@rpkilby rpkilby commented Nov 4, 2016

Supersedes and resolves #543.

This also adds descending options to OrderingFilter.field_labels.

@rpkilby
Copy link
Collaborator Author

rpkilby commented Nov 4, 2016

Thanks @ad-m!

@@ -656,7 +656,7 @@ def normalize_fields(cls, fields):

def build_choices(self, fields, labels):
ascending = [
(param, labels.get(field, pretty_name(param)))
(param, labels.get(field, _(pretty_name(param))))
for field, param in fields.items()
]
descending = [
Copy link
Contributor

@ad-m ad-m Nov 4, 2016

Choose a reason for hiding this comment

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

Use:

        descending = [
            ('-%s' % pair[0], labels.get('-%s' % pair[0], self.descending_fmt % pair[1]))
            for pair in ascending
        ]

to support translation of label for descending options (eg, '-username'). There is no reason to not support them. In some cases it can improve UX experience by naming orders in more natural ways.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's not a bad point. It's also a fairly trivial thing to add. Currently updating the PR to include this & your corresponding test.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ad-m - see the latest commit.

@rpkilby rpkilby force-pushed the fix-orderingfilter-translation branch from bca402d to 6955fd2 Compare November 4, 2016 22:51
@rpkilby rpkilby force-pushed the fix-orderingfilter-translation branch from 6955fd2 to b63d222 Compare November 4, 2016 22:52
@ad-m
Copy link
Contributor

ad-m commented Nov 5, 2016

LGTM! 👍

@carltongibson carltongibson added this to the 1.0 milestone Nov 5, 2016
@carltongibson carltongibson merged commit 470b89c into carltongibson:develop Nov 8, 2016
@rpkilby rpkilby deleted the fix-orderingfilter-translation branch November 8, 2016 15:54
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.

3 participants