Simple blog view and template

Simple blog view and template

[ThomasJWaldmann, 2012-06-12 17:07:18]

http://codereview.appspot.com/6307082/diff/1/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

http://codereview.appspot.com/6307082/diff/1/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:474: #TODO: move to BlogItem class
# TODO: ...

http://codereview.appspot.com/6307082/diff/1/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:484: revs = flaskg.storage.search(query,
sortedby=[PTIME], reverse=True, limit=None)
maybe also add a debug print here to see what you get from whoosh

http://codereview.appspot.com/6307082/diff/1/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:485: blog_post_items =
[Item.create(rev.meta[NAME]) for rev in revs]
you don't really make sure that the item uses the rev you have found, it will
likely use the latest rev present there.

[Reimar Bauer, 2012-06-13 06:00:00]

answered

22:31 < spy> dreimark: do you have any ideas on

http://codereview.appspot.com/6307082/diff/1/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

http://codereview.appspot.com/6307082/diff/1/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:486: for item in blog_post_items:
You may want to look into

http://hg.moinmo.in/moin/2.0/file/452b341325eb/MoinMoin/apps/frontend/views.p...

[ThomasJWaldmann, 2012-06-17 22:42:38]

some more comments

http://codereview.appspot.com/6307082/diff/14001/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

http://codereview.appspot.com/6307082/diff/14001/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:485: # We only want the sub-item part of the
blog post item names.
how does this ^^ comment fit here?

http://codereview.appspot.com/6307082/diff/14001/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:486: blog_posts = [(post_item,
Markup(post_item._render_data())) for post_item in blog_post_items]
maybe you could omit this line if you just give the list of blog_post_items to
the template and call _render_data() from there.

http://codereview.appspot.com/6307082/diff/14001/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:489:
header_rendered=Markup(item._render_data()),
same for this one

http://codereview.appspot.com/6307082/diff/14001/MoinMoin/templates/blog.html
File MoinMoin/templates/blog.html (right):

http://codereview.appspot.com/6307082/diff/14001/MoinMoin/templates/blog.html...
MoinMoin/templates/blog.html:17: {% if newtab %}target="_blank"{% endif %}>{{
title }}</a></h1>
isn't that target=... deprecated?

we removed such stuff from moin long time ago. if user wants a new tab/window HE
controls that by using middle mouse button (or similar means).

http://codereview.appspot.com/6307082/diff/14001/MoinMoin/themes/modernized/s...
File MoinMoin/themes/modernized/static/css/common.css (right):

http://codereview.appspot.com/6307082/diff/14001/MoinMoin/themes/modernized/s...
MoinMoin/themes/modernized/static/css/common.css:292: .moin-post-info {
font-size: 12px; color: #888888; }
ehrm, please check other places setting font-size. iirc we usually use em.

[spy, 2012-06-17 22:53:01]

http://codereview.appspot.com/6307082/diff/14001/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

http://codereview.appspot.com/6307082/diff/14001/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:485: # We only want the sub-item part of the
blog post item names.
On 2012/06/17 22:42:38, ThomasJWaldmann wrote:
> how does this ^^ comment fit here?
deleted, sorry

http://codereview.appspot.com/6307082/diff/14001/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:486: blog_posts = [(post_item,
Markup(post_item._render_data())) for post_item in blog_post_items]
On 2012/06/17 22:42:38, ThomasJWaldmann wrote:
> maybe you could omit this line if you just give the list of blog_post_items to
> the template and call _render_data() from there.

okay

http://codereview.appspot.com/6307082/diff/14001/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:489:
header_rendered=Markup(item._render_data()),
On 2012/06/17 22:42:38, ThomasJWaldmann wrote:
> same for this one

okay

http://codereview.appspot.com/6307082/diff/14001/MoinMoin/templates/blog.html
File MoinMoin/templates/blog.html (right):

http://codereview.appspot.com/6307082/diff/14001/MoinMoin/templates/blog.html...
MoinMoin/templates/blog.html:17: {% if newtab %}target="_blank"{% endif %}>{{
title }}</a></h1>
On 2012/06/17 22:42:38, ThomasJWaldmann wrote:
> isn't that target=... deprecated?
> 
> we removed such stuff from moin long time ago. if user wants a new tab/window
HE
> controls that by using middle mouse button (or similar means).

was just copy-pasted from utils.html
clean it up also there?

[Reimar Bauer, 2012-06-19 06:07:23]

a comment

http://codereview.appspot.com/6307082/diff/10004/MoinMoin/templates/blog.html
File MoinMoin/templates/blog.html (right):

http://codereview.appspot.com/6307082/diff/10004/MoinMoin/templates/blog.html...
MoinMoin/templates/blog.html:25: <ul class="moin-post-tags">
why is for div and ul the same class name used?

[Reimar Bauer, 2012-06-19 10:51:44]

another comment

http://codereview.appspot.com/6307082/diff/10004/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

http://codereview.appspot.com/6307082/diff/10004/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:465:
@frontend.route('/+blog/<itemname:item_name>', defaults=dict(rev=CURRENT))
which methods ?

[spy, 2012-06-19 19:50:32]

http://codereview.appspot.com/6307082/diff/10004/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

http://codereview.appspot.com/6307082/diff/10004/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:465:
@frontend.route('/+blog/<itemname:item_name>', defaults=dict(rev=CURRENT))
On 2012/06/19 10:51:45, Reimar Bauer wrote:
> which methods ?

Sorry, did not understand what you mean...

http://codereview.appspot.com/6307082/diff/10004/MoinMoin/templates/blog.html
File MoinMoin/templates/blog.html (right):

http://codereview.appspot.com/6307082/diff/10004/MoinMoin/templates/blog.html...
MoinMoin/templates/blog.html:25: <ul class="moin-post-tags">
On 2012/06/19 06:07:23, Reimar Bauer wrote:
> why is for div and ul the same class name used?

I'm not a css guru, is it a bad practice?
It seems that div and ul are related to the same object (tags). Their styles
could be edited by using ul.moin-post-tags and div.moin-post-tags respectively.

[Reimar Bauer, 2012-06-19 20:49:45]

answered

http://codereview.appspot.com/6307082/diff/10004/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

http://codereview.appspot.com/6307082/diff/10004/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:465:
@frontend.route('/+blog/<itemname:item_name>', defaults=dict(rev=CURRENT))
On 2012/06/19 19:50:32, spy wrote:
> On 2012/06/19 10:51:45, Reimar Bauer wrote:
> > which methods ?
> 
> Sorry, did not understand what you mean...

Is it GET or POST or both together.

You can have a keyword methods for the route, look at others

[spy, 2012-06-19 21:09:44]

Thanks, fixed.

http://codereview.appspot.com/6307082/diff/10004/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

http://codereview.appspot.com/6307082/diff/10004/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:465:
@frontend.route('/+blog/<itemname:item_name>', defaults=dict(rev=CURRENT))
On 2012/06/19 20:49:46, Reimar Bauer wrote:
> On 2012/06/19 19:50:32, spy wrote:
> > On 2012/06/19 10:51:45, Reimar Bauer wrote:
> > > which methods ?
> > 
> > Sorry, did not understand what you mean...
> 
> Is it GET or POST or both together.
> 
> You can have a keyword methods for the route, look at others
> 
> 

Currently we need only GET method, thus added methods=['GET'] parameter.

[ThomasJWaldmann, 2012-06-23 11:23:04]

http://codereview.appspot.com/6307082/diff/17002/MoinMoin/templates/blog.html
File MoinMoin/templates/blog.html (right):

http://codereview.appspot.com/6307082/diff/17002/MoinMoin/templates/blog.html...
MoinMoin/templates/blog.html:22: {% set categories = item.meta['supertags'] %}
you could reverse these 2 lines, somehow like:
set categories = ...
if categories:

http://codereview.appspot.com/6307082/diff/17002/MoinMoin/templates/blog.html...
MoinMoin/templates/blog.html:39: {% if post_item.meta['tags'] %} {# display
post's tags as a list if they existed #}
i don't think comments about the rather obvious are needed.
please rather comment if you are doing something non-obvious.

http://codereview.appspot.com/6307082/diff/17002/MoinMoin/templates/blog.html...
MoinMoin/templates/blog.html:40: {% set tags = post_item.meta['tags']|sort %}
see last comment

[Reimar Bauer, 2012-07-04 04:53:19]

some comments

http://codereview.appspot.com/6307082/diff/23001/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

http://codereview.appspot.com/6307082/diff/23001/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:486: title_name=item.name,
where is title_name used in the template

What is the reason to rename item_name?

http://codereview.appspot.com/6307082/diff/23001/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:508: title_name=item.name,
Why not item_name, I think we have that in all other templates used. So the name
can be constant?

[spy, 2012-07-04 17:29:07]

Reply on comments about 'title_name'

http://codereview.appspot.com/6307082/diff/23001/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

http://codereview.appspot.com/6307082/diff/23001/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:486: title_name=item.name,
On 2012/07/04 04:53:19, Reimar Bauer wrote:
> where is title_name used in the template
> 
> What is the reason to rename item_name?

title_name is used in the base.html template as a title of a page.

item=item, item_name=item.name  is used in almost all other templates where we
deal with only one item. But if you look at those templates such as +search,
+history, +index, etc where there are many items to represent on a page you can
find that title_name is used to specify the title of a page. In my views +post
and +blog there are many items used to represent on a page. That is why I wrote
my code in such a way.

http://codereview.appspot.com/6307082/diff/23001/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:508: title_name=item.name,
On 2012/07/04 04:53:19, Reimar Bauer wrote:
> Why not item_name, I think we have that in all other templates used. So the
name
> can be constant?

Look at the previous comment ^

[ThomasJWaldmann, 2012-07-05 15:11:04]

http://codereview.appspot.com/6307082/diff/17002/MoinMoin/themes/modernized/s...
File MoinMoin/themes/modernized/static/css/common.css (right):

http://codereview.appspot.com/6307082/diff/17002/MoinMoin/themes/modernized/s...
MoinMoin/themes/modernized/static/css/common.css:305: .moin-box-heading {
font-weight: bold; margin: 0; padding: 0; }
why is this removed? did you add it originally?

http://codereview.appspot.com/6307082/diff/23001/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

http://codereview.appspot.com/6307082/diff/23001/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:493: def show_post(item_name, rev):
maybe the "post" in show_post is not descriptive enough.
also it is a http method and other stuff (not blog) could also have "posts". so
I suggest to rename this.
show_blog_entry, show_blog_index, ...

also rename the template accordingly.

http://codereview.appspot.com/6307082/diff/23001/MoinMoin/templates/blog_layo...
File MoinMoin/templates/blog_layout.html (right):

http://codereview.appspot.com/6307082/diff/23001/MoinMoin/templates/blog_layo...
MoinMoin/templates/blog_layout.html:13: {{ _("Posted on") }} {{
post_item.meta['ptime']|datetimeformat }}
Hmm, "posted on" is not quite the truth, this is rather the publication date.
The post may have happened at another date.
Maybe "published on"?

http://codereview.appspot.com/6307082/diff/23001/MoinMoin/templates/blog_layo...
MoinMoin/templates/blog_layout.html:21: {% if tags %} {# display post's tags as
a list if they existed #}
... if it is not empty

http://codereview.appspot.com/6307082/diff/23001/MoinMoin/templates/blog_layo...
MoinMoin/templates/blog_layout.html:32: {% set discussion_item_name =
'{0}/{1}'.format(post_item.name, 'Discussion') %}
hmm, do we have a cfg attribute for "Discussion"?

http://codereview.appspot.com/6307082/diff/23001/MoinMoin/themes/modernized/s...
File MoinMoin/themes/modernized/static/css/common.css (right):

http://codereview.appspot.com/6307082/diff/23001/MoinMoin/themes/modernized/s...
MoinMoin/themes/modernized/static/css/common.css:259: .moin-contenttypes-wrapper
form { position: absolute; top: 1.9em; left: 0; display: none; margin: 0.3em 0;
border-top: 1px solid #E5E5E5; z-index: 2; background: #FFFFFF; border: 1px
solid #E5E5E5; }
why is this changed? where is it used?

[spy, 2012-07-05 17:33:22]

http://codereview.appspot.com/6307082/diff/23001/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

http://codereview.appspot.com/6307082/diff/23001/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:493: def show_post(item_name, rev):
On 2012/07/05 15:11:05, ThomasJWaldmann wrote:
> maybe the "post" in show_post is not descriptive enough.
> also it is a http method and other stuff (not blog) could also have "posts".
so
> I suggest to rename this.
> show_blog_entry, show_blog_index, ...
> 
> also rename the template accordingly.

Done.

http://codereview.appspot.com/6307082/diff/23001/MoinMoin/templates/blog_layo...
File MoinMoin/templates/blog_layout.html (right):

http://codereview.appspot.com/6307082/diff/23001/MoinMoin/templates/blog_layo...
MoinMoin/templates/blog_layout.html:13: {{ _("Posted on") }} {{
post_item.meta['ptime']|datetimeformat }}
On 2012/07/05 15:11:05, ThomasJWaldmann wrote:
> Hmm, "posted on" is not quite the truth, this is rather the publication date.
> The post may have happened at another date.
> Maybe "published on"?

Done.

http://codereview.appspot.com/6307082/diff/23001/MoinMoin/templates/blog_layo...
MoinMoin/templates/blog_layout.html:21: {% if tags %} {# display post's tags as
a list if they existed #}
On 2012/07/05 15:11:05, ThomasJWaldmann wrote:
> ... if it is not empty

Done.

http://codereview.appspot.com/6307082/diff/23001/MoinMoin/templates/blog_layo...
MoinMoin/templates/blog_layout.html:32: {% set discussion_item_name =
'{0}/{1}'.format(post_item.name, 'Discussion') %}
On 2012/07/05 15:11:05, ThomasJWaldmann wrote:
> hmm, do we have a cfg attribute for "Discussion"?

we have a cfg.supplementation_item_names = [u'Discussion', ] it is used in the
itemviews listing. How could I use this list of names in our case? I guess there
may be multiple names in this list.

http://codereview.appspot.com/6307082/diff/23001/MoinMoin/themes/modernized/s...
File MoinMoin/themes/modernized/static/css/common.css (right):

http://codereview.appspot.com/6307082/diff/23001/MoinMoin/themes/modernized/s...
MoinMoin/themes/modernized/static/css/common.css:259: .moin-contenttypes-wrapper
form { position: absolute; top: 1.9em; left: 0; display: none; margin: 0.3em 0;
border-top: 1px solid #E5E5E5; z-index: 2; background: #FFFFFF; border: 1px
solid #E5E5E5; }
On 2012/07/05 15:11:05, ThomasJWaldmann wrote:
> why is this changed? where is it used?

It was my mistake. I accidentally replaced it.

[ThomasJWaldmann, 2012-07-06 20:53:24]

> we have a cfg.supplementation_item_names = [u'Discussion', ] it is used in the
> itemviews listing. How could I use this list of names in our case? I guess
there
> may be multiple names in this list.

Use cfg.supplementation_item_names[0] for now. I guess we need something better
there, but [0] is still better than hardcoding it likely.