readthedocs / readthedocs.org

The source code that powers readthedocs.org
https://readthedocs.org/
MIT License
8.06k stars 3.59k forks source link

Forms: avoid Crispy layout API? #11042

Open agjohnson opened 10 months ago

agjohnson commented 10 months ago

In the work around the new dashboard and handling forms, I found a few forms where we decided to use the Crispy layout API instead of a basic Django form. This was usually to add fieldsets, but sometimes it was to add HTML content to the form. I'd like to discuss the merits of this pattern, and whether it's worth continuing or perhaps removing and settling on a single pattern.

The issues that I have with this pattern:

So, I personally would rather author complex forms in templates rather than in HTML in Python, as it is easier to maintain and author.

However, one point I'll give a pattern like the layout API is that we can get closer to rendering all forms with the same template logic: {% crispy form %}. This doesn't outweigh the negatives for me though.

Are there more benefits to using the Crispy layout API that I'm not acknowledging?

humitos commented 10 months ago

The two things I like about using Layout/FieldSet on Python are:

As I haven't touched the new templates too much, I don't have any benefits to highlight about not using the Crispy Layout pattern, tho. However, I'd like to know what would be the pattern you are suggesting to create something similar to Layout/FieldSet from the templates?

The next, or maybe even a previous question would be, "do we need these Layout/FieldSet at all?". It seems we were only using it for:

So, I'm 85% convinced we don't need this Layout/FieldSet pattern, but I'd like to know what is the replacement your are proposing in case we need it.

agjohnson commented 10 months ago

it's a technology we all know and feel comfortable with

You noted this too, but I might describe this pattern as less standardized than that. We've used it, but only in a few views. But also, we haven't absorbed doing complex FUI/SUI with the layout API yet either, which might feel a bit less comfortable :smile:

it requires changing the code only in one place

Yeah, I can see how from just the form/application side of things this would be nicer. I feel the opposite of this actually though, as working display side of things it's not as nice editing a template and coinciding application form simultaneously for the same HTML output.

However, I'd like to know what would be the pattern you are suggesting to create something similar to Layout/FieldSet from the templates?

There isn't a great answer here. The problem I struggle with is a form that is more complex than a very vanilla Django form (just a simple {{ form | crispy }}) requires you to manipulate the form instance somewhere -- so far this has been in template code.

This is the case for the following changes:

I think we're talking the same thing overall, and the examples you pointed out are on their way to removing the API already, but I also don't have a slam dunk replacement in templates. At very least, keeping all of the HTML in one place feels way more maintainable and easier to author though.

humitos commented 10 months ago

I understand the frustration that you are describing when working with forms and requiring specific HTML/JS code 😄

I don't have too much to add here, but I just wanted to say that I'm not in love with any of the patterns you liked to in the previous comment 😅 . Using custom alter_field template tag looks pretty similar to manage it by Python code in the form itself, since the alter_field is a Python function in the end. Also, it's a non-standard way to handle forms in Django, which makes me some noise.

On the other hand, don't rendering the form at all using {{ form|crispy }} and writing the whole HTML code is not necessarily bad, and I've seen it multiple times. However, it's pretty hard to maintain over time, get updates and parity with the rest of the forms, and I would avoid this pattern as much as possible.

agjohnson commented 10 months ago

I don't have too much to add here, but I just wanted to say that I'm not in love with any of the patterns you liked to in the previous comment

Yeah, neither pattern is great, but I do feel it is far better than having to add the HTML structure or HTML attributes like Knockout bindings in Python code. At least using template tags separates concerns and keeps all of the HTML changes in HTML/templates. I don't feel it matters too much that the logic ultimately comes from Python inside template code, that is almost unavoidable.

For some background, this was the same concept as: https://github.com/jazzband/django-widget-tweaks

writing the whole HTML code is not necessarily bad

I'd say it's not bad when there is reason to restructure the whole form, but to customize a single field this would be a bad pattern to follow. I share concerns around the form class and template drifting apart as well, and would only use this pattern when we really want a custom form.


Long term, I think this is maybe a point for web components. Web components are all JS driven, instead of using data binding attributes to dictate how elements behave. This could help reduce some of the concerns, though I don't really know what this pattern looks like yet either.

For now though, I still feel altering the fields in HTML is the best overall solution when we need to add CSS classes/data bindings to fields. I feel the Crispy API is nice in theory, but we don't have a strong reason to use it heavily, and the areas we are using it -- adding commentary in between fields and splitting up complex forms into multiple sections -- are patterns we should avoid in themselves.

But, again, these patterns are just compromises overall. I have not found a pattern for form HTML that I am excited to work with, just patterns I hate less.