Code review comment for lp://staging/~zematynnad/ubuntu-webcatalog/buttons__938167

Hey Danny,

I've just moved your replies inline so I can see what the context was...

On Mon, Mar 12, 2012 at 11:07 AM, Michael Nelson
<email address hidden> wrote:
> On Fri, Mar 9, 2012 at 10:31 PM, Danny Tamez <email address hidden> wrote:
>> Overview
>> =========
>> This branch adds fancy download buttons for apps an also has some drive by fixes for 2 related bugs.
>>
>> Details
>> =========
>> For bug 938167 - new buttons are being used and replace the old grey buttons.  Here are some screenshots:
>> Purchase button: http://simplest-image-hosting.net/png-0-ac2075
>> Free button: http://simplest-image-hosting.net/png-0-gr2075
>> Download ubuntu: http://simplest-image-hosting.net/png-0-wg2075
>
> Cool - I like the  purchase and free buttons. I'm not sure about the
> download ubuntu button there though? It seems a bit out of place there
> next to the app, and would perhaps belong elsewhere on the page (ie.
> where there's space to say "Haven't tried Ubuntu? [[Download Ubuntu
> graphic/link]]"). But if it's something you've already discussed in a
> pre-imp. with someone, great.
>
> I've put that comment on the bug so if there is a discussion, it can
> happen there. I realise that's not something you'd changed (it's an
> option to the template tag), but it may or may not be worth
> discussing.

On Tue, Mar 13, 2012 at 4:02 AM, Danny Tamez <email address hidden> wrote:
> We discussed the placement of the download/install button and decided it would probably look better beneath the description to the left of the screenshot or just underneath the screenshot.  I left it underneath the description and here are screenies of each:
> button below description: http://simplest-image-hosting.net/png-0-hj2158
> button below screenshot: http://simplest-image-hosting.net/png-0-tn2158

If Anthony is happy with that placement, great. My comment above
wasn't about placement, in fact I thought the purchase and free
buttons looked great where they were (but I'm no designer). My comment
was just specifically about the "Download *Ubuntu*" button - it's not
something you've changed in this branch, just something I noticed that
it seems to lack any context (that is, as a non-Ubuntu user, I'd
expect something like "{{ app_name }} is available for Ubuntu. Haven't
tried Ubuntu? [[ Download Ubuntu graphic/link ]]"), which obviously
wouldn't fit up the top. But anyway, it's not a change of yours, just
something I thought while looking at the screenshots, so feel free to
ignore.

>
>>
>> For bug 861593 - the issue with bad wrapping and button display has been alleviated. Screenshots:
>> Chrome button: http://simplest-image-hosting.net/png-0-io2075
>> Chrome also available: http://simplest-image-hosting.net/png-0-jv2086
>> Since this anchor was really a link to the download page and not a download itself I left this as a link and not a button.
>
> Yikes - yes, +1 for that being a link. The link looks much more
> natural. I've put a few comments with the inline changes related to
> this.
>
>>
>> For bug 944254 - the text about the app not being available for your operating system has been removed.
>
> I've added a comment on that bug too (we should use {% trans %} for
> the title of the images).
>
> Other comments inline below.
>
>>
>> To Test
>> ========
>> $fab bootstrap test
>>
>> --
>> https://code.launchpad.net/~zematynnad/ubuntu-webcatalog/buttons__938167/+merge/96836
>> You are subscribed to branch lp:ubuntu-webcatalog.
>>
>> === modified file 'src/webcatalog/templates/webcatalog/install_options_snippet.html'
>> --- src/webcatalog/templates/webcatalog/install_options_snippet.html    2011-09-19 21:33:34 +0000
>> +++ src/webcatalog/templates/webcatalog/install_options_snippet.html    2012-03-09 21:30:35 +0000
>> @@ -1,16 +1,20 @@
>>  {% load i18n %}
>>  {% if message_text %}
>> -  {{ message_text }}
>> -{% endif %}{% if display_install_button %}
>> -  <a href="apt://{{ application.package_name }}" class="install-button">
>> +    <div id="message_text">
>> +        {{ message_text }}
>> +    </div>
>> +{% endif %}
>> +{% if display_install_button %}
>> +  <a href="apt://{{ application.package_name }}">
>>   {% if application.for_purchase %}
>> -  {% blocktrans with application.name as application_name %}Purchase {{ application_name }}{% endblocktrans %}
>> +<img class="fancy-install" width="200" height="60" alt="" src="{{ STATIC_URL }}images/scbutton-non-free-200px.png" title="Get it from the Ubuntu Software Centre!">
>
> We should use {% trans %} here for the title. And should the title
> text say "Buy it.."?

On Tue, Mar 13, 2012 at 4:02 AM, Danny Tamez <email address hidden> wrote:
> Changed the tooltip on the non-free button to say 'Buy it' instead of 'Get it'.

Great

> As for the {{ trans }} issues: none of the application is translated and so we decided there's really no point in doing that.

Are you sure you guys aren't getting confused with actual translations
for application data? That we're not supporting yet, sure, but AFAIK
we are currently ensuring that all the static text strings in our UI
(such as the title attribute above) are translateable? You can see
this in the above with the {% blocktrans %} that you've removed - the
"Purchase {{ applicatiion_name }}" was marked for translation, but
you've replaced it with an image title "Get it from the Ubuntu
Software Center!" which is not marked for translation.

Anyway, if that's all stuff you guys were aware of when you decided,
then that's fine. Maybe it's also worth chatting about after a
standup, as it's news to me and may be to others too.

>
>>   {% else %}
>> -  {% blocktrans with application.name as application_name %}Install {{ application_name }}{% endblocktrans %}
>> +<img class="fancy-install" width="200" height="60" alt="" src="{{ STATIC_URL }}images/scbutton-free-200px.png" title="Get it from the Ubuntu Software Centre!">
>
> Just {% trans %}
>
>>   {% endif %}
>>   </a>
>>  {% endif %}
>>  {% if display_ubuntu_download %}
>> -<a href="http://www.ubuntu.com/download" class="awesome">{% trans "Download Ubuntu" %}</a>
>> +<a href="http://www.ubuntu.com/download" class="awesome">
>> +<img class="fancy-install" width="200" height="60" alt="" src="{{ STATIC_URL }}images/download_ubuntu.png" title="Get it from the Ubuntu Software Centre!">
>
> {% trans %} and does this title make sense here?

I think you missed this question? (at least, I don't see it answered
or updated in your branch). That is, it's a "Download Ubuntu" button,
but the title text says "Get it from the Ubuntu Software Centre!"

>
>> +</a>
>>  {% endif %}
>> -
>>
>> === modified file 'src/webcatalog/templatetags/webcatalog.py'
>> --- src/webcatalog/templatetags/webcatalog.py   2012-03-01 21:38:31 +0000
>> +++ src/webcatalog/templatetags/webcatalog.py   2012-03-09 21:30:35 +0000
>> @@ -48,7 +48,8 @@
>>     """Evaluate the installation options for the application in context."""
>>     useragent = UserAgentString(context['user_agent'])
>>     template_context = dict(application=application, message_text='',
>> -        display_install_button=False, display_ubuntu_download=False)
>> +        display_install_button=False, display_ubuntu_download=False,
>> +                            STATIC_URL=settings.STATIC_URL)
>
> It shouldn't be necessary to pass STATIC_URL in the context at all...
> it smells like we're not using the static context processor? If that's
> the case, let's update to use that instead.
>
> https://docs.djangoproject.com/en/1.3/ref/contrib/staticfiles/#the-static-context-processor
>

On Tue, Mar 13, 2012 at 4:02 AM, Danny Tamez <email address hidden> wrote:
> Fixed the {{STATIC_URL}} issue.  We were already using the static context processor but the
> templatetag was overwriting the RequestContext and thus the context processor values were no longer
> available.  I changed the template tag to instead modify the RequestContext with the new values instead
> of creating an entirely new context.
>

Great.

>>
>>     if useragent.distroseries == application.distroseries.code_name:
>>         # If both the distroseries and architecture matches, install away,
>> @@ -104,9 +105,7 @@
>>         template_context['display_install_button'] = True
>>         return template_context
>>     else:
>> -        template_context['message_text'] = _(
>> -            '{application_name} is not available for your operating '
>> -            'system.').format(application_name=application.name)
>> +        template_context['message_text'] = ''
>
> See comment on bug about this.

Let me know what you think about this. The comment on the bug will
give more context at:

https://bugs.launchpad.net/ubuntu-webcatalog/+bug/944254/comments/3

If you've already discussed it with someone, made a decision, and just
not updated the bug, no probs, otherwise let me know.

>
>>         template_context['display_ubuntu_download'] = True
>>         return template_context
>>
>>