Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allowing attrs to be set via template on FormFieldNode #131

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ghinch
Copy link

@ghinch ghinch commented Nov 17, 2014

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 the formrow template tag, and have it pass through to the rendered node.

@gregmuellegger
Copy link
Collaborator

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?

@ghinch
Copy link
Author

ghinch commented Dec 16, 2014

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.

@ghinch
Copy link
Author

ghinch commented May 1, 2015

Any chance this can come in at the next release?

@melinath
Copy link
Collaborator

melinath commented May 2, 2015

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

@ghinch
Copy link
Author

ghinch commented May 2, 2015

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.

@melinath
Copy link
Collaborator

melinath commented May 3, 2015

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

@gregmuellegger
Copy link
Collaborator

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?

@dan-passaro
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants