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

Add SuffixedMultiWidget #681

Merged
merged 4 commits into from
Aug 31, 2017

Conversation

rpkilby
Copy link
Collaborator

@rpkilby rpkilby commented Apr 8, 2017

Resolves #425. Not sure if this would be considered for a major/minor release given the API changes.

Changes:

  • Add SuffixedMultiWidget
  • Change RangeWidget to use min/max suffixes
  • Add DateRangeWidget that uses after/before suffixes (could remove and just use RangeWidget`)
  • Change LookupTypeWidget to just the field name and a lookup suffix. So, ?price=15&price_lookup=lt

TODO:

  • docs.
  • tests?

@rpkilby rpkilby force-pushed the suffixed-multiwidget branch 4 times, most recently from 50f475b to b134d36 Compare April 8, 2017 22:27
@codecov-io
Copy link

codecov-io commented Apr 8, 2017

Codecov Report

Merging #681 into develop will decrease coverage by 0.12%.
The diff coverage is 93.54%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #681      +/-   ##
===========================================
- Coverage    98.11%   97.99%   -0.13%     
===========================================
  Files           15       15              
  Lines         1117     1148      +31     
===========================================
+ Hits          1096     1125      +29     
- Misses          21       23       +2
Impacted Files Coverage Δ
django_filters/widgets.py 98.7% <93.54%> (-1.3%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 645f24a...8c15b6a. Read the comment docs.

@carltongibson
Copy link
Owner

Looks good. I had thought that the new widget probably could not be the default, simply because it's contrary to the standard API...

@rpkilby
Copy link
Collaborator Author

rpkilby commented Apr 9, 2017

I had thought that the new widget probably could not be the default, simply because it's contrary to the standard API...

Makes sense.

Eventually (next major version?) should the default change to using SuffixedMultiWidget?

@carltongibson
Copy link
Owner

Eventually (next major version?) should the default change to using SuffixedMultiWidget?

Not sure at this point. 🙂

@rpkilby rpkilby force-pushed the suffixed-multiwidget branch 5 times, most recently from cb47d16 to cdbeab7 Compare April 11, 2017 18:00
@rpkilby
Copy link
Collaborator Author

rpkilby commented Apr 11, 2017

Okay - the breaking changes have been split off into a separate commit, and I'll submit a new PR for consideration once this has been reviewed/accepted.

@carltongibson
Copy link
Owner

Eventually (next major version?) ...

I'm coming round to a 2.0, dropping Python 2 support.

@rpkilby
Copy link
Collaborator Author

rpkilby commented May 16, 2017

Pushed the breaking commit to suffixed-multiwidget-default so I don't lose track of it.

@rpkilby rpkilby force-pushed the suffixed-multiwidget branch 2 times, most recently from 799ded1 to 8c15b6a Compare August 22, 2017 22:09
@rpkilby
Copy link
Collaborator Author

rpkilby commented Aug 22, 2017

rebased

Copy link
Owner

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

👍

@carltongibson carltongibson merged commit d760aa6 into carltongibson:develop Aug 31, 2017
@rpkilby rpkilby deleted the suffixed-multiwidget branch August 31, 2017 08:35
carltongibson pushed a commit that referenced this pull request Oct 19, 2017
* Add SuffixedMultiWidget

* Add SuffixedMultiWidget support for django < 1.11

* Add SuffixedMultiWidget tests

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

Successfully merging this pull request may close these issues.

3 participants