-
-
Notifications
You must be signed in to change notification settings - Fork 145
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
base: master
Are you sure you want to change the base?
Conversation
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 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
It's more like a method call on attrs. I wouldn't implement this generically but more of a magic "lookup" for A variant (1b) on this would read Variant 2
That would introduce a new keyword 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 Class names In Variant 1a:
In Variant 2:
What do you think about that? |
I don't like https://github.com/kmike/django-widget-tweaks has a similar feature, they use a syntax Another alternative might be just to say
|
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.