[Django] #26357: 'related-lookup' and 'add-another' popups don't work for admin fields added via ajax
Django
via ajax
-------------------------------+-----------------------------------
Reporter: julianandrews | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Keywords: admin javascript ajax
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 1
-------------------------------+-----------------------------------
If a field with a `'related-lookup'` or `'add-another'` link is added
after page load (for instance via ajax), the javascript event handler
which causes these links to open a pop-up doesn't get attached to the new
links. Instead of opening a pop-up window to allow selection of the
related object, the links simply send the user to the list page for the
object.
As a result ajax generated admin fields don't work correctly. While this
doesn't break the Django admin itself, it does make extending the admin
with Ajax difficult.
An easy fix is to attach the `'related-lookup'` and `'add-another'`
`click` events to `document` instead of to the links themselves. This
would come at a small performance cost since the events will be triggered
on every click instead of just on link click, but with modern javascript
engines I think the cost would be unnoticeable, and given how unobtrusive
this change is I think the added flexibility justifies the cost.
I'll attach a PR for the proposed fix after I submit this report.
--
Ticket URL: <https://code.djangoproject.com/ticket/26357>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
via ajax
Reporter: julianandrews | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.admin | Version: master
Keywords: admin javascript | Triage Stage:
ajax | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 1
Changes (by julianandrews):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
I've created a patch here:
https://github.com/fusionbox/django/tree/ticket_26357
It's a fairly simple fix, but since I can see arguments against this
approach, I've held off on making a PR directly. If I get the go ahead,
I'll make a PR on this patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/26357#comment:1>
Django
via ajax
Reporter: julianandrews | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: admin javascript | Triage Stage: Accepted
ajax |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 1
Changes (by timgraham):
* type: Uncategorized => Cleanup/optimization
* stage: Unreviewed => Accepted
Comment:
The main concern I have is how to ensure the admin continues to support
such use cases in the future. I guess we can say "no tests = no official
support" and you can continue sending PRs in the future if things
inadvertently break? At least you might want to add a comment about it so
someone doesn't "cleanup" the code to the old version.
--
Ticket URL: <https://code.djangoproject.com/ticket/26357#comment:2>
Django
via ajax
Reporter: julianandrews | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: admin javascript | Triage Stage: Accepted
ajax |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 1
Comment (by julianandrews):
@timgraham: I've gone ahead and added comments.
I agree that maintaining future support for the use case would be nice,
but I'm not sure exactly what that would look like. Given how common ajax
and dynamic page content is now, it's probably a good practice to avoid
writing javascript that assumes a static page structure. Maybe a line to
that effect could be added to
https://docs.djangoproject.com/en/dev/internals/contributing/writing-
code/javascript/?
In any case, Fusionbox relies on ajax in the admin somewhat heavily so
we're likely to catch inadvertent breakage down the line and send PRs.
--
Ticket URL: <https://code.djangoproject.com/ticket/26357#comment:3>
Django
via ajax
Reporter: julianandrews | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: admin javascript | Triage Stage: Accepted
ajax |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 1
Comment (by timgraham):
Sure, some documented guidelines wouldn't hurt.
--
Ticket URL: <https://code.djangoproject.com/ticket/26357#comment:4>
Django
via ajax
Reporter: julianandrews | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: admin javascript | Triage Stage: Accepted
ajax |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 1
Comment (by julianandrews):
@timgraham I've added my best go at a documentation improvement.
I'm not 100% happy with it just because I'm not sure it's the best idea to
encourage binding events to the document, so take a look and I'll either
submit this as a PR as is, or rollback the documentation change and just
submit the first commit.
--
Ticket URL: <https://code.djangoproject.com/ticket/26357#comment:5>
Django
via ajax
Reporter: Julian Andrews | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: admin javascript | Triage Stage: Accepted
ajax |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 1
Comment (by Julian Andrews):
@timgraham I completely lost track of this issue, and just discovered it
again. I could rebase my changes, make sure it all still works, and submit
a PR. Would that be welcome?
--
Ticket URL: <https://code.djangoproject.com/ticket/26357#comment:6>
Django
via ajax
Reporter: Julian Andrews | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: admin javascript | Triage Stage: Accepted
ajax |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 1
Comment (by Tim Graham):
Yes, creating a pull request for review seems to be the next step.
--
Ticket URL: <https://code.djangoproject.com/ticket/26357#comment:7>
Django
via ajax
Reporter: Julian Andrews | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: admin javascript | Triage Stage: Accepted
ajax |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 1
Comment (by Julian Andrews):
I've added a PR here https://github.com/django/django/pull/7405.
--
Ticket URL: <https://code.djangoproject.com/ticket/26357#comment:8>
Django
via ajax
Reporter: Julian Andrews | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: admin javascript | Triage Stage: Accepted
ajax |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 1
Changes (by Tim Graham):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/26357#comment:9>
Django
via ajax
Reporter: Julian Andrews | Owner: nobody
Cleanup/optimization |
Component: contrib.admin | Version: master
Severity: Normal | Resolution: fixed
Keywords: admin javascript | Triage Stage: Accepted
ajax |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 1
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"adc93e85990b644194c679f528225deed9fdaa85" adc93e85]:
{{{
#!CommitTicketReference repository=""
revision="adc93e85990b644194c679f528225deed9fdaa85"
Fixed #26357 -- Allowed admin popups to work on links added after page
load.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26357#comment:10>