Issue 18: No more mobile version of the site

Templates now take mobile flag into account and provides a better experience
for those browsing through m.jaiku.com. Also provides meta viewport declaration
to better serve iPhone users.

Patch by: Jonas Nockert

Committed: r70

[lemonad]

http://rietku.appspot.com/10001/diff/1/35
File css/mobile.css (left):

http://rietku.appspot.com/10001/diff/1/35#oldcode1
Line 1: body {
Unfortunately this file was saved with DOS line endings so the diff needs to be
done with the appropriate flags excluding line ending styles.

[lemonad]

The above should have been "this file was _initially_ saved with..." (the
uploaded file has unix line endings).

[termie]

whole bunch of comments, it seems in general we may need to start moving towards
a more centralized approach to url generation

http://rietku.appspot.com/10001/diff/1041/41
File actor/templates/contacts.html (right):

http://rietku.appspot.com/10001/diff/1041/41#newcode7
Line 7: <h1><a href="{{view|get_url:request.mobile}}/contacts">People</a> /
{{whose}} contacts</h1>
seems like we should be taking request rather than request.mobile, request is
sort of where we can store various characteristics about the request and is
therefore a bit more useful

additionally we should probably make this into a tag, my suggestion is something
like:

{% url_for view request %}

which looks something like (in format.py):

def url_for(entity, request):
  return entity.url(mobile=request.mobile)

register.simple_tag(url_for)


which will require making a some of the models accept extra args to their url
method

http://rietku.appspot.com/10001/diff/1041/45
File actor/templates/item.html (right):

http://rietku.appspot.com/10001/diff/1041/45#newcode8
Line 8: {{entry.actor_ref|actor_link:request.mobile}} posted to
{{entry.owner_ref|actor_link:request.mobile}}:
may want to do request instead of request.mobile and have actor_link backend its
url generation on url_for

http://rietku.appspot.com/10001/diff/1041/50
File actor/views.py (right):

http://rietku.appspot.com/10001/diff/1041/50#newcode450
Line 450: if request.mobile:
not really necessary... see below

http://rietku.appspot.com/10001/diff/1041/50#newcode459
Line 459: {'entry_add_comment': entry_ref_url,
entry_ref.url(mobile=request.mobile)

we could even realistically pass request=request instead and make all of the url
functions decide things based on request so we have some more versatility with
fewer args in the future

http://rietku.appspot.com/10001/diff/1041/78
File channel/templates/channel_tiles.html (right):

http://rietku.appspot.com/10001/diff/1041/78#newcode5
Line 5: {% if request.mobile %}{{channel|linked_avatar_mobile:"t"}}{% else
%}{{channel|linked_avatar:"t"}}{% endif %}
these might do well to be turned into simple_tag implementations as well

http://rietku.appspot.com/10001/diff/1041/85
File channel/views.py (right):

http://rietku.appspot.com/10001/diff/1041/85#newcode297
Line 297: entry_ref_url = entry_ref.mobile_url()
as with the others

http://rietku.appspot.com/10001/diff/1041/51
File common/mail.py (right):

http://rietku.appspot.com/10001/diff/1041/51#newcode127
Line 127: activation_mobile_url = 'http://m.%s/confirm/email/%s' %
(settings.DOMAIN,
this breaks down a little in the no-subdomains situation, which is mostly okay
for now since it is pretty easy to add the two subdomains, but a version that
can be DOMAIN/m/whatever is probably in the future roadmap

http://rietku.appspot.com/10001/diff/1041/52
File common/models.py (right):

http://rietku.appspot.com/10001/diff/1041/52#newcode234
Line 234: def actor_url(nick, actor_type, path='', prefix=''):
i think mobile=None or request=None is more appropriate

http://rietku.appspot.com/10001/diff/1041/52#newcode295
Line 295: def mobile_url(self, path=""):
i think the mobile url should be an argument to the url method

http://rietku.appspot.com/10001/diff/1041/55
File common/templates/base_navbar.html (right):

http://rietku.appspot.com/10001/diff/1041/55#newcode6
Line 6: {% if not request.mobile %}
how does a mobile user log in?

http://rietku.appspot.com/10001/diff/1041/62
File common/templates/email/email_comment.txt (right):

http://rietku.appspot.com/10001/diff/1041/62#newcode14
Line 14: click on one of the following links (or copy and paste it to your
browser):
after a quick chat around the office it seems like it might be a better idea to
simply send one link but try to do mobile browser detection and redirect when
appropriate

http://rietku.appspot.com/10001/diff/1041/69
File common/templates/footnav.html (right):

http://rietku.appspot.com/10001/diff/1041/69#newcode17
Line 17: <li><a
href="http://m.{{DOMAIN}}/login?redirect_to=http://{{request.META.HTTP_HOST}}{{request.META.PATH_INFO}}">Sign
In</a></li>
again, these have m. hardcoded which may be tricky for no-subdomainers, not sure
if that is going to be a problem

also, i see now the answer to my "where do they log in question"

http://rietku.appspot.com/10001/diff/1041/75
File common/templatetags/avatar.py (right):

http://rietku.appspot.com/10001/diff/1041/75#newcode99
Line 99: def linked_avatar(value, args):
these may end up better as simple tags that use some common url generation code

[lemonad]

http://rietku.appspot.com/10001/diff/1041/41
File actor/templates/contacts.html (right):

http://rietku.appspot.com/10001/diff/1041/41#newcode7
Line 7: <h1><a href="{{view|get_url:request.mobile}}/contacts">People</a> /
{{whose}} contacts</h1>
On 2009/04/21 21:41:02, termie wrote:
> seems like we should be taking request rather than request.mobile, request is
> sort of where we can store various characteristics about the request and is
> therefore a bit more useful
> 
> additionally we should probably make this into a tag, my suggestion is
something
> like:
> 
> {% url_for view request %}
> 
> which looks something like (in format.py):
> 
> def url_for(entity, request):
>   return entity.url(mobile=request.mobile)
> 
> register.simple_tag(url_for)
> 
> 
> which will require making a some of the models accept extra args to their url
> method
> 
> 

Done.

http://rietku.appspot.com/10001/diff/1041/45
File actor/templates/item.html (right):

http://rietku.appspot.com/10001/diff/1041/45#newcode8
Line 8: {{entry.actor_ref|actor_link:request.mobile}} posted to
{{entry.owner_ref|actor_link:request.mobile}}:
On 2009/04/21 21:41:02, termie wrote:
> may want to do request instead of request.mobile and have actor_link backend
its
> url generation on url_for

I created an actor_link tag. Seems like using variables as filter arguments is
somewhat problematic as they have to be defined or the template throws an
exception. Tests were failing because of request not being defined.

http://rietku.appspot.com/10001/diff/1041/50
File actor/views.py (right):

http://rietku.appspot.com/10001/diff/1041/50#newcode459
Line 459: {'entry_add_comment': entry_ref_url,
On 2009/04/21 21:41:02, termie wrote:
> entry_ref.url(mobile=request.mobile)
> 
> we could even realistically pass request=request instead and make all of the
url
> functions decide things based on request so we have some more versatility with
> fewer args in the future

Done.

http://rietku.appspot.com/10001/diff/1041/78
File channel/templates/channel_tiles.html (right):

http://rietku.appspot.com/10001/diff/1041/78#newcode5
Line 5: {% if request.mobile %}{{channel|linked_avatar_mobile:"t"}}{% else
%}{{channel|linked_avatar:"t"}}{% endif %}
On 2009/04/21 21:41:02, termie wrote:
> these might do well to be turned into simple_tag implementations as well

Done.

http://rietku.appspot.com/10001/diff/1041/85
File channel/views.py (right):

http://rietku.appspot.com/10001/diff/1041/85#newcode297
Line 297: entry_ref_url = entry_ref.mobile_url()
On 2009/04/21 21:41:02, termie wrote:
> as with the others

Done.

http://rietku.appspot.com/10001/diff/1041/51
File common/mail.py (right):

http://rietku.appspot.com/10001/diff/1041/51#newcode127
Line 127: activation_mobile_url = 'http://m.%s/confirm/email/%s' %
(settings.DOMAIN,
On 2009/04/21 21:41:02, termie wrote:
> this breaks down a little in the no-subdomains situation, which is mostly okay
> for now since it is pretty easy to add the two subdomains, but a version that
> can be DOMAIN/m/whatever is probably in the future roadmap

I agree. As "m." is hardcoded in several places outside of this patch, I would
prefer solving that later as to avoid extending the scope of this patch even
more. Would that be okay with you?

http://rietku.appspot.com/10001/diff/1041/52
File common/models.py (right):

http://rietku.appspot.com/10001/diff/1041/52#newcode234
Line 234: def actor_url(nick, actor_type, path='', prefix=''):
On 2009/04/21 21:41:02, termie wrote:
> i think mobile=None or request=None is more appropriate

Unfortunately had to go with both of the above as email templates do not have
access to "request".

http://rietku.appspot.com/10001/diff/1041/52#newcode295
Line 295: def mobile_url(self, path=""):
On 2009/04/21 21:41:02, termie wrote:
> i think the mobile url should be an argument to the url method

Had to go with both mobile and request here too.

http://rietku.appspot.com/10001/diff/1041/62
File common/templates/email/email_comment.txt (right):

http://rietku.appspot.com/10001/diff/1041/62#newcode14
Line 14: click on one of the following links (or copy and paste it to your
browser):
On 2009/04/21 21:41:02, termie wrote:
> after a quick chat around the office it seems like it might be a better idea
to
> simply send one link but try to do mobile browser detection and redirect when
> appropriate

@Jezlyn posted some good arguments against mobile browser detection on
#jaikuengine. I think it will be a difficult choice to decide whether the iPhone
or N800 should be rendered as mobile or not. I'm all for not sending out two
links though.

By the way, are we currently sending out mostly html emails? The dual links
aren't as bad there.

http://rietku.appspot.com/10001/diff/1041/75
File common/templatetags/avatar.py (right):

http://rietku.appspot.com/10001/diff/1041/75#newcode99
Line 99: def linked_avatar(value, args):
On 2009/04/21 21:41:02, termie wrote:
> these may end up better as simple tags that use some common url generation
code

Done.

[termie]

woot! okay just a couple small things and I'm ready to start trying to merge
this in :)

http://rietku.appspot.com/10001/diff/7015/8086
File common/templates/footnav.html (right):

http://rietku.appspot.com/10001/diff/7015/8086#newcode9
Line 9: <li><a href="http://m.{{DOMAIN}}/channel" accesskey="3">Channels</a>
[3]</li>
can these use that same {% url_for DOMAIN request %} syntax?

http://rietku.appspot.com/10001/diff/7015/8087
File common/templates/item_comments.html (right):

http://rietku.appspot.com/10001/diff/7015/8087#newcode12
Line 12: <p>{% if request %}{{comment|format_comment:request}}{% else
%}{{comment|format_comment}}{% endif %}</p>
kind of ugly but I guess I'll live

http://rietku.appspot.com/10001/diff/7015/8094
File common/templatetags/avatar.py (right):

http://rietku.appspot.com/10001/diff/7015/8094#newcode96
Line 96: class LinkedAvatarNode(template.Node):
this looks fine, just wondering whether register.simple_tag would for it, it
provides a much more readable api imho

http://rietku.appspot.com/10001/diff/7015/8095
File common/templatetags/format.py (right):

http://rietku.appspot.com/10001/diff/7015/8095#newcode86
Line 86: lambda match: '<a href="%s">@%s</a>' % (
@ -> #

[lemonad]

Thanks for the review!

http://rietku.appspot.com/10001/diff/7015/8086
File common/templates/footnav.html (right):

http://rietku.appspot.com/10001/diff/7015/8086#newcode9
Line 9: <li><a href="http://m.{{DOMAIN}}/channel" accesskey="3">Channels</a>
[3]</li>
On 2009/05/14 19:12:07, termie wrote:
> can these use that same {% url_for DOMAIN request %} syntax?

Sure! I'll fix that!

FYI, in order to bring down the number of template modifications, I haven't made
any changes if the mobile site isn't using a particular piece of a template.
I've tried to verify that all links in the mobile interface are indeed pointing
to m. pages but it's possible that I've missed something.

I'm not sure about that decision being especially wise except for avoiding an
even more massive change to the templates. What do you think, should all links
go through the new custom tags?

http://rietku.appspot.com/10001/diff/7015/8087
File common/templates/item_comments.html (right):

http://rietku.appspot.com/10001/diff/7015/8087#newcode12
Line 12: <p>{% if request %}{{comment|format_comment:request}}{% else
%}{{comment|format_comment}}{% endif %}</p>
On 2009/05/14 19:12:07, termie wrote:
> kind of ugly but I guess I'll live

I agree. I wasn't sure if I should make another custom tag out of it or not
since it's used in so few places. What do you think?

The if statement is needed since custom_filter:variable throws an exception if
the variable is not defined. I'm leaning toward it being a Django bug since
custom tags do not exhibit the same behavior.

http://rietku.appspot.com/10001/diff/7015/8094
File common/templatetags/avatar.py (right):

http://rietku.appspot.com/10001/diff/7015/8094#newcode96
Line 96: class LinkedAvatarNode(template.Node):
On 2009/05/14 19:12:07, termie wrote:
> this looks fine, just wondering whether register.simple_tag would for it, it
> provides a much more readable api imho

I don't think I can since it uses resolve(context) but I'd be happy to be wrong
here since I feel the same way as you about simple_tag being much more readable.

http://rietku.appspot.com/10001/diff/7015/8095
File common/templatetags/format.py (right):

http://rietku.appspot.com/10001/diff/7015/8095#newcode86
Line 86: lambda match: '<a href="%s">@%s</a>' % (
On 2009/05/14 19:12:07, termie wrote:
> @ -> #

Oops! Thanks for catching that :)

[lemonad]

For some strange reason change set 5 lists lots of changes compared to change
set 4... I only changed a couple of files.

Sorry about that.