Open ghinch opened 10 years ago
Hi @ghinch, sorry for the quite late response. Indeed that seems useful, so thank you for the suggestion!
However in order to merge them, this needs a bit more improvement. I think some documentation would be really helpful so people get to know that they can use your enhancement. Also we would need tests for this to make sure that future changes does not break this.
Would you be willing to work on this?
Okay sure I can work on this probably in the next couple weeks, as work is a bit manic over the last couple weeks leading up to Xmas.
Any chance this can come in at the next release?
This feels very "magical" to me... there's a secret key that you can pass in which, although it looks like everything else, behaves completely differently. It also moves the responsibility for defining the display of the form input out of the template, which seems antithetical to the goal of the project (as far as I understand it.)
That being said, it does fill a niche that's missing. Right now, to get this effect, you'd have to copy the widget template and tweak it manually, or create a custom widget class, which are both more invasive than a simple, commonly-desired change like this should warrant. And other options would involve making the templatetags even more convoluted.
So... yeah. :-p
I take your point @melinath, and it's valid. As might be inferred from this and my other pull request (#137), on the two large projects I've used floppyforms on, it's been great in allowing me to move the form logic further away from the presentation (in that case into an installable module), and push the presentation almost entirely to the template layer. The team members who do the HTML and front end love this, but without a change like I have here, they don't quite have full control to set things like placeholders on inputs.
The only real way to address the issue you've raise I can think of is the explicitly name all of the field attributes on the widget, and then pass those through the row template on to the widget. There are a finite number of tag attributes in the HTML spec, albeit they change not inconsistently (and that's nothing to say of the data-
attributes everyone uses for JS these days). The fact that there is a general "attrs" parameter at all seems to be the root of this, which I think stems from how Django has handled these in the built-in forms.
I'm open to suggestions, but I think the fact that floppyforms pushes so much control of the rendering onto the template is fantastic, which leaves this one current exception feeling a bit odd.
@ghinch Just to clarify, I would say that right now I'm a -0 on this. I don't have a better suggestion; I think this may be the best option that doesn't require reworking large portions of the library. Plus you've already put the work into writing this PR and you're apparently using it successfully in production.
Re: your actual question, I don't know exactly what the plans are for the next release, so I can't say whether this will be in there.
Hey, I also feel the oddity that @melinath is expressing. However the feature is very helpful, too helpful to be dropped in total. What do you think about having a syntax that makes it more explicit on how to change the attrs?
A brainstorm:
Variant 1a
{% formfield myform.myfield with attrs:update=myattrs %}
It's more like a method call on attrs. I wouldn't implement this generically but more of a magic "lookup" for :update
.
A variant (1b) on this would read {% formfield ... with attrs[]=myattrs %}
. Maybe that's more obvious on that it's something special. However I haven't seen another example of those special syntaxes in Django apps so far. So I'm not too keen on using that.
Variant 2
{% formfield myform.myfield with whatever=1 setattrs myattrs %}
{% formfield myform.myfield with whatever=1 setattrs placeholder="My Placeholder" moremyattrs %}
That would introduce a new keyword setattrs
. It would take dicts (like myattrs
and moremyattrs
in the examples) that are merged into the attrs of the widget template. It also takes keyword arguments that are put in the attrs.
That variant is very explicit, easy to grasp and flexible to be used with existing dicts and hardcoded attr keys. We would need to test this against dash-separated keys like data-...
attributes.
Class names There is also the #99 issue still open which wants to implement an easy way to add class names to a widget's input field. Classes are a special attribute as it's more a list of classes than a single attribute. We need to support that as well somehow. In the context of the above variants that could look like:
In Variant 1a:
{% formfield myform.myfield with attrs:update=myattrs classes:append="extraclass" %}
In Variant 2:
{% formfield myform.myfield with whatever=1 setattrs myattrs setclass "extraclass" myclasslist %}
What do you think about that?
I don't like setclass
as it sounds like it will override classes provided by the Widget
instance.
https://github.com/kmike/django-widget-tweaks has a similar feature, they use a syntax class+="extra-classes-here"
. This looks foreign in the world of Django templates but it at least corresponds to the python +=
operator.
Another alternative might be just to say extra_class
or class:append
; although this sounds dumb it has the benefit of exactly matching the actual HTML attribute it's modifying.
classes:append
works too imo.
It's useful to be able to set extra attributes on the rendered
FormFieldNode
via the template (e.g. where you want template authors to be able to set class names on the rendered fields). This change allows you to set a "attrs" value on theformrow
template tag, and have it pass through to the rendered node.