Bread styling #71
Bread styling #71
Conversation
hminnovation
changed the title
hminnovation
force-pushed
the
57-styling-bread
branch
from
4757965
to
b9f4661
Compare
…template. Works on #57
bakerydemo/breads/models.py
Outdated
| page = request.GET.get('page') | ||
| paginator = Paginator(self.breads, 2) |
There was a problem hiding this comment.
I've put 2 just for the moment to make testing easier so you only need to create three bread objects to test pagination.
|
Looking good; a few comments:
|
bakerydemo/breads/models.py
Outdated
|
|
||
| paginator = Paginator(all_resources, 5) # Show 5 resources per page | ||
| @property | ||
| def breads(self): |
There was a problem hiding this comment.
To conform to convention, as @shacker pointed out in the Blog model, this should be renamed get_breads and not be a property.
There was a problem hiding this comment.
No problem to change the name to get_breads. But removing the @property raises an error object of type 'method' has no len() on the pagintor. I'm not sure why that's happening. Would welcome suggestions on how to resolve.
|
@heymonkeyriot I pushed a change for; it looks as though when you changed from the property to a method, you weren't invoking the method:
|
|
|
… 57-styling-bread. Workson #57.
|
Screenshots for desktop and mobile for the BreadIndexPage and BreadPage |
| @@ -10,7 +10,7 @@ | |||
| <h1>{{ page.title }}</h1> | |||
|
|
|||
| <figure class="hidden-md-up"> | |||
| {% image self.image width-600 %} | |||
| {% image self.image width-500 %} | |||
There was a problem hiding this comment.
This means we only need to load a single image asset for the mobile viewport and desktop, which makes me feel a little better about using the visibility class.
| {% image self.image width-500 %} | ||
| </figure> | ||
| <ul class="bread-meta"> | ||
| {% if page.origin %} |
There was a problem hiding this comment.
I think it's slightly confusing to use self.image and page.origin in the same template. Would these both work with page.foo ?
There was a problem hiding this comment.
Good spot, and absolutely correct that we should keep them consistent. Will amend to page.foo throughout and push up.
|
This is looking great. Side question (not quite about this PR): I notice you put some thought into punny bread titles - they're very good. I'm already working with Tumi's original breads and have them in the "master" content copy. Were you wanting to go with funny title instead, or was that just placeholder content? |
|
Travis is not happy with Docker. |
I was sort of doing them for my own amusement, and was just placeholder content. I'm not sure how well they'll travel. I'd suggest we keep it fairly neutral in tone, though perhaps it wouldn't hurt to hide one or two puns around the place? |
|
Roger dodger, we'll stick with wikipedia content with maybe a pun here or there. I had parents in town this weekend and wasn't able to put in time, but back on the case tonight hopefully. |
Ready for review