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 wrote: > On Fri, Mar 9, 2012 at 10:31 PM, Danny Tamez 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 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 %} >> - >> +

>> + {{ message_text }} >> +

>> +{% endif %} >> +{% if display_install_button %} >> + >> {% if application.for_purchase %} >> - {% blocktrans with application.name as application_name %}Purchase {{ application_name }}{% endblocktrans %} >> +

> > 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 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 %} >> +

> > Just {% trans %} > >> {% endif %} >> >> {% endif %} >> {% if display_ubuntu_download %} >> -{% trans "Download Ubuntu" %} >> + >> +

> > {% 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!" > >> + >> {% 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 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 >> >>