#30263 closed Cleanup/optimization (fixed)
Mention the Media.merge change in the release notes
Reported by: | Matthias Kestenholz | Owned by: | Carlton Gibson |
---|---|---|---|
Component: | Forms | Version: | 2.2 |
Severity: | Release blocker | Keywords: | |
Cc: | Johannes Maron | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
The Media merge change (#30179, #30153) has some potential of breaking libraries which worked before. It would be nice to give a heads-up to developers that they have to check their Media
definitions.
The necessary change to django-content-editor was really small: https://github.com/matthiask/django-content-editor/commit/ed662db302fd74033ce62031bb11b6f8beddf249
Here's a draft for the release notes (I hope it helps -- I'm finding it hard to keep it short while still providing an useful example)
--
Merging of media classes
The Media
class merging algorithm has been changed to produce sensible results when pairwise merging is insufficient. However, CSS and JavaScript files which do not properly declare their dependencies may now be reordered before their dependencies. This only ever worked by accident.
The recommendation is to check all Media
declarations for dependencies and make those explicit. Just as an example, widgets depending on django.jQuery
should specify js=["admin/js/jquery.init.js", ...]
Change History (11)
comment:1 by , 6 years ago
Severity: | Normal → Release blocker |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 6 years ago
Hey, that was me :)
I'd appreciate it if you did the PR. I just thought I'd cc Johannes because of his involvement.
comment:3 by , 6 years ago
Hi @Matthias,
thanks for letting us know about this. I believe it's a good idea to add some documentation here. Since other might rely on implicit dependencies especially with the old merging warning.
Since you encountered the issue, what would you suggest as documentation. What would have helped or pointed you into the right direction?
Best,
Joe
comment:5 by , 6 years ago
Thanks, both of you.
I'd add the sentence in the ticket description under the "forms" heading and also mention the change under the "admin" heading, coupled with the recommendation of adding jquery.init.js
to the js
list.
(It didn't confuse me this time since I authored the last change to Media.merge
. A hint with what-to-do / upgrading instructions would be very helpful though. Upgrading instructions for breaking changes is a thing I missed in the past, e.g. when get_query_set
was changed to get_queryset
etc.)
comment:6 by , 6 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:9 by , 6 years ago
Has patch: | set |
---|
comment:10 by , 6 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Hey Joe. Yep, fine. Do you want to make that PR? (I can do it quickly if you say no.)