Opened 9 years ago
Last modified 9 years ago
#26300 new Cleanup/optimization
Convert contrib.flatpages views to class-based views
Reported by: | Tom Carrick | Owned by: | Tom Carrick |
---|---|---|---|
Component: | contrib.flatpages | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Someday/Maybe | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
I'm intending on applying for GSoC 2016 and the wiki mentions it would be nice if I've done some work in the area - so this is my attempt to do so.
Summary speaks for itself. We should look at using a CBV for the flatpages view. I notice there is an extra internal view and I'll look into seeing if there's a nice way of integrating it into the CBV rather than just calling it from within it.
Change History (5)
comment:1 by , 9 years ago
comment:2 by , 9 years ago
I don't disagree that CBVs are not always better, but I think they may be here. Personally, I think there are two things here.
Firstly, maybe you disagree, but I think that having two views and several lines of comments to explain why there are two is kind of messy. I think that's valuable cleanup in itself.
Secondly, yeah, it may not be much simpler than writing a custom view, but I think it's certainly cleaner.
Quick example:
You have a contact page with some copy above it. You want to be able to change the copy without changing the code. This is a very common scenario I've dealt with a number of times. There are several solutions here.
- Make your own
Page
model, and a view for it, then useDetailView
andFormMixin
here. Not bad, but what if flatpages are suitable for you otherwise, it makes more sense to leverage them right?
- Use a regular
FormView
and something like django-flatblocks to put the copy in. Fine, but if you don't need flatblocks for anything else, it's kind of annoying.
- Use flatpages, have a separate view for the contact form, use a custom flatpage template and put your form in the template manually. Kind of awful in my view.
- Use flatpages model, but make your own view for it. Okay, kinda works, but do you really want to copy all the code from the flatblocks views for this? Violates DRY for me.
Let me know if there's a good solution I'm missing.
Proposed solution:
Inherit FlatPageDetailView
(or whatever) and mix in FormMixin
. Then just provide either a custom template with the form, or have your default flatpage template check for a form and render it if it exists. Seems like a nice solution in my view. It might make a good example for the docs, too.
comment:3 by , 9 years ago
Summary: | Dogfood CBV in contrib.flatpages → Convert contrib.flatpages views to class-based views |
---|---|
Triage Stage: | Unreviewed → Someday/Maybe |
Thanks, I guess I'll have to see the final code to say for sure but it's good to see you've thought about it.
comment:4 by , 9 years ago
Thanks Tim. Where can I go from here, should I work on an implementation and submit a PR or do we need discussion on the mailing list or anything?
comment:5 by , 9 years ago
I think seeing the code changes will help evaluate whether or not this will be useful.
Could you motivate the reason why they should become class-based views? Is it a common need to customize them? If so, how? As the views are only a handful of lines each, will a class-based approach be any simpler than writing a custom view?
Class-based views are not always better/simpler than function-based ones.