Opened 8 years ago

Last modified 21 months ago

#15220 new Cleanup/optimization

replace SelectFilter2.js with a jQuery plugin

Reported by: Nick Sandford Owned by: dbunskoek
Component: contrib.admin Version: master
Severity: Normal Keywords:
Cc: cmawebsite@…, ben@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: yes


I needed to extend the functionality of the select filter, so I rewrote it in jQuery. I noticed a discussion about replacing some of the admin javascript with jQuery, so this is a first attempt at replacing SelectFilter2.js with jquery.selectfilter.js.

Discussion is here:

Attachments (3)

selectfilter-widget-r15403.diff (8.4 KB) - added by Nick Sandford 8 years ago.
selectfilter-widget-2-r15426.diff (10.4 KB) - added by Nick Sandford 8 years ago.
15220-selectfilter-r16355.patch (10.9 KB) - added by dbunskoek 8 years ago.

Download all attachments as: .zip

Change History (22)

Changed 8 years ago by Nick Sandford

comment:1 Changed 8 years ago by anonymous

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

This needs to be reviewed by someone with better JS-foo than I have, but two initial impressions:

  1. The instantiation code ( looks a lot more complex -- is there any way to clean up that interface?
  2. The code also contains a TODO. If we're refactoring in this area, it's a prime opportunity to do that cleanup.

comment:2 Changed 8 years ago by Russell Keith-Magee

Oops - that was me.

comment:3 Changed 8 years ago by Nick Sandford

The instantiation code could probably be a bit cleaner without the anonymous function, but it won't change it too much. It was mostly to avoid typing django.jQuery(etc..) twice (once for the document.ready and a second time for invoking the plugin)..

I'll upload a patch to fix the TODO shortly. Cheers.

Changed 8 years ago by Nick Sandford

comment:4 Changed 8 years ago by Nick Sandford

I think BoundField has an auto_id property but I'm not sure how to go about using it, so I'm hoping the approach I took was ok (using attrs.get('id', 'id_%s' % name) ).

comment:5 Changed 8 years ago by Łukasz Rekucki

Severity: Normal
Type: Cleanup/optimization

comment:6 Changed 8 years ago by Julien Phalip

UI/UX: set

comment:7 Changed 8 years ago by dbunskoek

Easy pickings: unset
Owner: changed from Nick Sandford to dbunskoek

Changed 8 years ago by dbunskoek

comment:8 Changed 8 years ago by dbunskoek

Patch needs improvement: unset

I updated the original patch so it will apply cleanly to trunk, and also made some improvements:

Improved coding-style

  • moved from leading spaces to tabs
  • removed trailing comma's (fixes IE support)

Usability improvements

  • removed inconsistent 'wrap-around' functionality in selects
  • also allow moving back from 'selected' to 'choices'
  • only allow moving with right or left key when the layout is horizontal (it doesn't make sense when layout is vertical)
  • fixed keyboard shortcuts (didn't work at all in the trunk version)

I tested it all in IE7, IE8, FF4, Safari, Chrome, and it works like a charm.

comment:9 Changed 8 years ago by Idan Gazit

This looks good to me, but I also lack the JS-foo to vet every line of the JS.

This would be an excellent candidate for figuring out a more standardized means of testing. Could be QUnit, could be Selenium, could be both. I'd feel more comfortable with JS changes like these if I had a test suite to run on them.

comment:10 Changed 7 years ago by Jannis Leidel

FWIW, selenium tests have now landed in the admin, so this should be much easier to test.

comment:11 Changed 7 years ago by Julien Phalip

Quick note: some tests were added in r17579.

comment:12 Changed 7 years ago by Julien Phalip

FYI, while working on #13614, I've used the latest patch from here and brought it up to date with current trunk.

comment:13 Changed 7 years ago by Julien Phalip

Patch needs improvement: set

comment:14 Changed 7 years ago by Julien Phalip

We'll do this shortly after 1.4 gets released, based on the patch in #13614.

comment:15 Changed 6 years ago by Tim Graham

I closed #3202 as a duplicate which points out the inability of the current implementation to handle large lists. We should make sure new solution improves this.

comment:16 Changed 5 years ago by Collin Anderson

Cc: cmawebsite@… added

comment:17 Changed 4 years ago by Ben Kornrath

Cc: ben@… added

comment:18 Changed 4 years ago by Tim Graham

PR 4701 is the latest effort on this if someone is interested in updating it.

comment:19 Changed 21 months ago by Collin Anderson

This is basically fixed now that we can use select2 for many-to-many fields. Once it's been around for a while we could try changing UserAdmin to use select2 by default instead of SelectFilter2.js.

Note: See TracTickets for help on using tickets.
Back to Top