Newforms media (#4418)
Russell Keith-Magee
I recently raised ticket #4418 to add media descriptors to newforms.
It was discussed on the following thread:
To my reading, Jacob and Malcolm were +1 to the change (correct me if
I am misrepresenting here)
Any objections to my committing this to trunk?
Yours,
Russ Magee %-)
Malcolm Tredinnick
I hadn't actually read the patch and I think I lost consciousness at
some point in the middle of the thread (last week was a busy week on
this list, it seemed). The concept was slowly growing on me, though.
Just had a quick read through and one bug that jumped out is the
absolute_path() method messes up "https://..." URLs.
There's also no way to set the media attribute for CSS (screen/print
being a particularly useful distinction).
I realise you're trying to tidy things up before fleeing to remote parts
of the planet, but I'd like to read this through a bit more (unless one
of the other guys are hugely in favour, in which case I defer to their,
and your, superior judgement). I'll try to patch any non-controversial
problems I can see to save some time.
Cheers,
Malcolm
Russell Keith-Magee
>
> On Sat, 2007-06-02 at 10:02 +0800, Russell Keith-Magee wrote:
> > Hi All,
> >
>
> I hadn't actually read the patch and I think I lost consciousness at
> some point in the middle of the thread (last week was a busy week on
> this list, it seemed). The concept was slowly growing on me, though.
>
> Just had a quick read through and one bug that jumped out is the
> absolute_path() method messes up "https://..." URLs.
Valid point; easily fixed for the https case.
> There's also no way to set the media attribute for CSS (screen/print
> being a particularly useful distinction).
True. The easiest solution I can think of here is:
class MyWidget(Widget):
...
class Media:
css = {
'print': (...),
'screen': (...)
}
js = (...)
If the value of css is a list/tuple, it renders as currently
implemented; if you provide a dict, it renders multiple links, one for
each media type. How does that sound?
> I realise you're trying to tidy things up before fleeing to remote parts
> of the planet, but I'd like to read this through a bit more (unless one
> of the other guys are hugely in favour, in which case I defer to their,
> and your, superior judgement). I'll try to patch any non-controversial
> problems I can see to save some time.
No worries. If it doesn't happen before I depart, it will still be
there when I return. I just wanted to tie up lose ends before I leave.
Yours,
Russ Magee %-)
Malcolm Tredinnick
> On 6/2/07, Malcolm Tredinnick <mal...@pointy-stick.com> wrote:
> >
> > On Sat, 2007-06-02 at 10:02 +0800, Russell Keith-Magee wrote:
> > > Hi All,
> > >
> > > Any objections to my committing this to trunk?
> >
> > I hadn't actually read the patch and I think I lost consciousness at
> > some point in the middle of the thread (last week was a busy week on
> > this list, it seemed). The concept was slowly growing on me, though.
> >
> > Just had a quick read through and one bug that jumped out is the
> > absolute_path() method messes up "https://..." URLs.
>
> Valid point; easily fixed for the https case.
>
> > There's also no way to set the media attribute for CSS (screen/print
> > being a particularly useful distinction).
>
> True. The easiest solution I can think of here is:
>
> class MyWidget(Widget):
> ...
> class Media:
> css = {
> 'print': (...),
> 'screen': (...)
> }
> js = (...)
>
> If the value of css is a list/tuple, it renders as currently
> implemented; if you provide a dict, it renders multiple links, one for
> each media type. How does that sound?
Not really great (sorry). :-(
You don't really want to list all known media types, because there are a
lot of them (10 in CSS2, although support for many is limited). Also,
some will be combined and some company style guides may prefer to use
"@media" designators inside the stylesheets themselves and so use
media="all". We can probably omit the "combined" case -- I'm thinking of
things like media="handheld, screen" -- since putting that in as two
links won't hurt; the data will only be loaded once. We can also omit
the case of leaving off the media attribute. It defaults to "screen", so
specifying that explicitly won't hurt if that makes things easier.
There's probably a balance point between infinite flexibility and
pragmatism here, but it's hard to see where. This is kind of what my
original concern about this whole scheme was: the easiest and
generically simple solution is just specifying the information yourself.
However, I think if we're going to do this, we should try to at least
accommodate the standard spec functionality and rendering for different
media is something fundamental enough that even I've done it.
Is the simple solution to make "css" always a dictionary (and we don't
validate the keys, so as to be extensible)? CSS users have to think
about media anyway, so this isn't really going to be something out of
left field for them to specify.
Regards,
Malcolm
John D'Agostino
On Jun 2, 12:02 pm, "Russell Keith-Magee" <freakboy3...@gmail.com>
wrote:
> Hi All,
>
> I recently raised ticket #4418 to add media descriptors to newforms.
> It was discussed on the following thread:
>
> http://groups.google.com/group/django-developers/browse_thread/thread...
>
> To my reading, Jacob and Malcolm were +1 to the change (correct me if
> I am misrepresenting here)
>
> Any objections to my committing this to trunk?
>
a small bug that i noticed after reading over the patch;
self closing <script> tags will fail on older browsers (ie6). So
render_js should be;
def render_js(self, path):
return u'<script type="text/javascript" src="%s"></script>' %
self.absolute_path(path)
Malcolm Tredinnick
I re-read this a bit later an I think I misunderstood it originally. I'm
not completely wild about 'css' being able to take on two fairly
different data structure forms, but I guess that's a reasonable
compromise. We would need to be explicit that the tuple means "screen",
so that union of Media classes works as expected.
Yeah.. I'll stop being difficult now. You're right; that would work.
Regards,
Malcolm