Code

Opened 3 years ago

Last modified 12 months ago

#15220 new Cleanup/optimization

replace SelectFilter2.js with a jQuery plugin

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

Description

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: http://groups.google.com/group/django-developers/browse_thread/thread/ff460085a3bfb12e/2c4e6f5a9807c19b?lnk=gst&q=jquery#2c4e6f5a9807c19b

Attachments (3)

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

Download all attachments as: .zip

Change History (18)

Changed 3 years ago by slurms

comment:1 Changed 3 years ago by anonymous

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

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

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

comment:2 Changed 3 years ago by russellm

Oops - that was me.

comment:3 Changed 3 years ago by slurms

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 3 years ago by slurms

comment:4 Changed 3 years ago by slurms

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 3 years ago by lrekucki

  • Severity set to Normal
  • Type set to Cleanup/optimization

comment:6 Changed 3 years ago by julien

  • UI/UX set

comment:7 Changed 3 years ago by dbunskoek

  • Easy pickings unset
  • Owner changed from slurms to dbunskoek

Changed 3 years ago by dbunskoek

comment:8 Changed 3 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 3 years ago by idangazit

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 2 years ago by jezdez

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

comment:11 Changed 2 years ago by julien

Quick note: some tests were added in r17579.

comment:12 Changed 2 years ago by julien

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 2 years ago by julien

  • Patch needs improvement set

comment:14 Changed 2 years ago by julien

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

comment:15 Changed 12 months ago by timo

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from dbunskoek to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.