Opened 3 years ago

Last modified 13 months ago

#22298 new Cleanup/optimization

Rename Form Media to Static (or get rid of Form Media completely?)

Reported by: Aymeric Augustin 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 (15)

comment:1 Changed 3 years ago by Tim Graham

Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

comment:2 Changed 3 years ago by Vishal Lal

Owner: changed from nobody to Vishal Lal
Status: newassigned

comment:3 Changed 3 years ago by Russell Keith-Magee

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

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 3 years ago by Russell Keith-Magee

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

comment:6 Changed 3 years 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 3 years ago by Aymeric Augustin

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

Owner: Vishal Lal deleted
Status: assignednew

comment:10 Changed 3 years ago by Tim Graham

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 3 years 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 Changed 2 years ago by Collin Anderson

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 23 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 22 months ago by Stephan Herzog

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).

comment:15 Changed 13 months ago by Collin Anderson

Summary: Rename Media to StaticRename Form Media to Static (or get rid of Form Media completely?)

James Aylett (on stage at duth)'s solution "that will solve everything" (he says it came to him at 1:30 in the morning and he hasn't fully thought through it :):

{% load assets %}
<html>
  <head>
    {% asset_tag css %}
    {% asset css "site-chrome" %}
    {% asset css "article-page" %}
  </head>
  <body>
    <h1>My heading</h1>
    {% asset css "search-box" %}
    <form action='{% url "search" %}'>
      <!-- ... -->
    </form>
  </body>
</html>
Note: See TracTickets for help on using tickets.
Back to Top