Opened 17 months ago

Last modified 5 months ago

#22298 new Cleanup/optimization

Rename Media to Static

Reported by: aaugustin Owned by:
Component: Forms Version: master
Severity: Normal Keywords:
Cc: loic@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The confusion between media and static files is almost cleared, except in the Widget API which still sports a Media class.

Change History (14)

comment:1 Changed 17 months ago by timo

  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Bug to Cleanup/optimization

comment:2 Changed 16 months ago by vlal

  • Owner changed from nobody to vlal
  • Status changed from new to assigned

comment:3 follow-up: Changed 16 months ago by russellm

Just throwing this out there on the stoop to see if the cat licks it up: is there any evidence that Media objects are actually being used?

I added this feature years ago, but it's not one of my proudest API moments - it didn't really solve the problem I was trying to fix.

Rather than go through a complicated rename process, should we just be deprecating it altogether?

comment:4 in reply to: ↑ 3 Changed 16 months ago by charettes

Replying to russellm:

Just throwing this out there on the stoop to see if the cat licks it up: is there any evidence that Media objects are actually being used?

I added this feature years ago, but it's not one of my proudest API moments - it didn't really solve the problem I was trying to fix.

Rather than go through a complicated rename process, should we just be deprecating it altogether?

I think it's pretty common to define a widget with a Media class to reference static files (a JavaScript file enhancing the rendered input for instance).

It's also quite convenient way for third party apps to provide a fully bootstrapped object.

comment:5 Changed 16 months ago by russellm

Ok - if you're convinced people are actually using it, consider my request withdrawn.

comment:6 Changed 16 months ago by loic84

When I spotted this ticket, I thought the same thing as @russellm, why not just deprecate it, although I didn't dare mentioning it.

My biggest issue with it is that it's a very naive approach to a complicated problem. It doesn't really fit with today's development practices where a project can easily grow to hundreds of static assets, where javascript offers concepts like AMD, where CSS gained preprocessors like LESS or SASS and where files are concatenated for performance.

Having each input widget pull it's own assets can also be a performance footgun, and IMO it's hardy something we should recommend.

comment:7 Changed 16 months ago by aaugustin

Well we can put this on hold until we have a superior replacement. Or maybe just drop the feature and tell people to include their JS globally.

comment:8 Changed 16 months ago by loic84

  • Cc loic@… added

Telling people to include JS globally or to use a third party app akin to django-compressor and django-pipeline is IMO a viable option. It fixes the issue that random assets are loaded into the DOM by forcing users to acknowledge static dependencies.

A superior replacement would be to add one of the apps above to the contrib apps.

comment:9 Changed 16 months ago by vlal

  • Owner vlal deleted
  • Status changed from assigned to new

comment:10 Changed 16 months ago by timo

I'd be in favor of deprecating it since I've never used it myself and it doesn't seem to attract much interest from core developers as far as I can tell. An alternative suggested (now nearly two years ago) in #18455 of starting "fresh" and creating a Static class that integrates with contrib.staticfiles may be interesting, although I tend to agree with Loic that this strategy may not really be consistent with modern practices and simply including JS globally may be easiest.

There are just two uses of this in Django itself, both in GeoDjango: OpenLayersWidget and OSMWidget.

Here are some related tickets:
#21987 - Allow Media objects to have their own MEDIA_TYPES
#18455 - Added hooks to Media for staticfiles app
#13978 - Allow inline js/css in forms.Media
#12265 - Media (js/css) collection strategy in Forms has no order dependence concept
#12264 - calendar.js depends on jsi18n but date widgets using it do not specify as required media
#9357 - Unable to subclass form Media class

I created a django-dev thread to get feedback from the community.

comment:11 Changed 15 months ago by luismasuelli@…

It'd be good to give the user the final choice. First of all, having a Static class instead of a Media class would be a good deprecation (and not a replacement until later versions).

Second: allow to have the currently supported behavior AND the alternative to load them as global -compressed- content.

Currently I developed a widget (a fully autocomplete-based app which contrasts with database entries | https://github.com/luismasuelli/jackfrost) which uses jquery ui by including static files via Media, and think that media objects, since they're application or widget-dependent, should not be deleted.

comment:12 follow-up: Changed 8 months ago by collinanderson

As a data point, I use it a bunch. As Mark said on django-developers, it's especially helpful for the admin. I'd be fine if we moved all support for it there instead of forms itself.

Again, because it's the admin, I personally don't need pipeline optimizations.

Overriding templates in the admin doesn't work very well in my experience. It's hard to override specific things and the templates are pretty undocumented so we don't maintain backwards compatibility very well. It's a little MVC-punching, but it's pretty nice to simply add a WYSIWYG field on your model and be able to get a nice widget in the admin automatically.

comment:13 in reply to: ↑ 12 Changed 6 months ago by awol

Replying to collinanderson:

As a data point, I use it a bunch. As Mark said on django-developers, it's especially helpful for the admin. I'd be fine if we moved all support for it there instead of forms itself.

I've just run into a situation where I am using CSS to brand the admin and the media class is not being inserted in the base templates. I would rather not have to override the templates since all I want to do is just add a custom CSS file to every page. I interpreted the documentation of the custom AdminSite feature to suggest that defining a Media class in my custom site would achieve this. At the moment, not so much.

Effectively adding {{ media.css }} into the extrastyles block of base.html addresses much of the issue. Unfortunately login and the delete_*confirm templates (at least) would need additional modification since they derive from base_site.html rather than base.html.

Ideally adding 'media': self.media to the each_context function would also be required, but I can already override that in my custom AdminSite.

I don't have a full understanding of the distinction between these inheritance choices to know what the right change would be in that context. I wouldn't want to dive in and change the inheritance without knowing why they are base_site vs base templates.

Even if this functionality was moved into a Static class rather than Media, for me the feature is desirable, it's a very convenient way to brand the admin, but it's implementation at the moment is not consistent, from what I am seeing.

I was going to add a ticket for this consistency issue, but if this is a better ticket on which to hang the issue I'm happy.

comment:14 Changed 5 months ago by sthzg

I also think that it is valuable to have the functionality in Django Admin. Like mentioned above, it allows to add enhancements to certain views easily while not having to override the admin templates. I think that there are many small projects that are maintained through Django Admin and being able to drop in some Javascript for certain views is essential, at least for me. I would miss this functionality if it was gone completely (instead of keeping it or deprecating it in favor of something more elaborate).

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