-
Notifications
You must be signed in to change notification settings - Fork 537
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
Create abstract base class for common fields #47
Conversation
Hey wow, I had actually done half of this work last night, was going to finish today, but you beat me to it. Thanks! Code looks great, and I pulled it down and it migrated and works fine. My only question is: Shouldn't every page of the site have a large hero image and intro? (both index and detail pages)? If so, then this should also be applied to BlogPage, BreadPage, and LocationPage, not just the index pages. If you agree, can you do a 2nd commit on this PR to add those? Or do you disagree? |
Apologies! I was going under the notion that if a ticket was unassigned, it
was free for the taking.
I'll review the other pages; you are likely correct!
|
Of course you're right that unassigned tickets are up for grabs - I should have thought to assign it to myself. All yours now :) |
@shacker I would like some feedback as to how to best approach BlogPage This model is currently differentiated by the addition of a
BreadPage
How would you suggest that we reconcile the above with the |
For the BlogPage, I don't think we need both subtitle and introduction. Intro is important for smart truncation of blog posts on index pages so keep that, but subtitle isn't actually something I've seen on blogs before. I'd ditch that. For the content panels of BlogPage, I'd show the inherited ones first, then the specific ones (so introduction after the two inherited ones). On the BreadPage, I don't see the point of a heading CharBlock since we already have the bread's title. With that gone, we could just use body from elsewhere, rather than introduction (to maximize consistency). Agree/disagree? |
@shacker when you have a change, if you could take another look at this PR; I implemented your suggestions. |
@shacker and @daaray the subtitle came from the design, and is the text that's appearing visually under the main title on the BlogPage. If it's going to make lives difficult then I'd say ditch it, but it does help with the design by giving an extra element to play with. (Not perhaps relevant but On UK news websites it's also a pattern that's used regularly to have title, standfirst then introduction) The rest looks great! |
@heymonkeyriot @shacker I restored subtitle as it is a design requirement. It also allows us to illustrate the malleability of the
This should be ready for final review. |
That's a very interesting technique, using a list comprehension to manipulate the content_panels dynamically. |
Fixes #41
Introduces
base.models.BasePageFieldsMixin
which includesimage
andintroduction
fields.Currently utilized in:
base.models.GalleryPage
blog.models.BlogIndexPage
breads.models.BreadsIndexPage
locations.models.LocationsIndexPage
Review other pages for use of the mixin: