On Fri, Apr 20, 2012 at 9:54 PM, Danny Tamez <email address hidden> wrote:
> Danny Tamez has proposed merging lp:~zematynnad/ubuntu-webcatalog/screenshot_enlarge_845911 into lp:ubuntu-webcatalog.
>
> Requested reviews:
>  Canonical Consumer Applications Hackers (canonical-ca-hackers)
> Related bugs:
>  Bug #845911 in Ubuntu Apps Directory: "WC should ape USC screenshot behaviour"
>  https://bugs.launchpad.net/ubuntu-webcatalog/+bug/845911
>
> For more details, see:
> https://code.launchpad.net/~zematynnad/ubuntu-webcatalog/screenshot_enlarge_845911/+merge/102926
>
> Overview
> ========
> This branch adds a panel that displays an enlarged image for the application screenshots.
>
> Details
> ========
> If there are no screnshots and none are loaded via ajax then no panel is needed and no panel is loaded.
> If there is a single image or a carousel for multiple screenshots then the panel will be displayed when the user clicks on a thumbnail.
>
> Pic of one screenshot and then the enlarged image:
> http://simplest-image-hosting.net/png-0-rz2643
> http://simplest-image-hosting.net/png-0-en2643
> Pic of carousel and then the enlarged image:
> http://simplest-image-hosting.net/png-0-jp2643
> http://simplest-image-hosting.net/png-0-md2643

Cool - thanks for the screenshots! It looks great!

Just summarising the comments below:
 1) It'd be great if the new JS code that you're adding could be
stripped of any django template code and moved to an external js file
(or YUI module file). The template pages are getting way too much
inline JS IMO (which usually translates to non-reusable code and hard
to understand templates). This would mean the page simply does
something like: new Y.uwc.ScreenshotsPanel({...config options});

Another option is that it could be built-in to the carousel, or a
subclass of the carousel? This would obviously be easier if we updated
the carousel to handle just one image properly rather than having if
statements that only include the carousel when there's > 1 image.

2) If you wanted to and were keen, the same could be done for the
other JS code there, but otherwise I'll create a bug or do it as a
fly-by on another branch at some point.

It may be worth checking with Anthony, as he might have a different
opinion and think it's not worthwhile - if so, I'm happy to +1 this as
is - it just scares me seeing more and more non-reusable JS with
template tags intertwined going directly into our templates.

Let me know what you think!

>
> To Test
> ========
> $fab bootstrap test
> --
> https://code.launchpad.net/~zematynnad/ubuntu-webcatalog/screenshot_enlarge_845911/+merge/102926
> Your team Canonical Consumer Applications Hackers is requested to review the proposed merge of lp:~zematynnad/ubuntu-webcatalog/screenshot_enlarge_845911 into lp:ubuntu-webcatalog.
>
> === modified file 'src/webcatalog/static/css/carousel.css'
> --- src/webcatalog/static/css/carousel.css      2012-04-19 23:05:19 +0000
> +++ src/webcatalog/static/css/carousel.css      2012-04-20 19:53:20 +0000
> @@ -219,3 +219,21 @@
>     top: -15px;
>     clear: both;
>  }
> +.yui3-panel {
> +    outline: none;
> +}
> +#viewerPanel {
> +    -webkit-box-shadow: 0px 0px 2px black;
> +    -moz-box-shadow: 0px 0px 2px black;
> +    box-shadow: 0px 0px 2px black;
> +}
> +#viewerPanel .yui3-widget-hd {
> +    font-weight: bold;
> +    background:url("/assets/light/images/header_bg.png") repeat-x scroll left top #DD4814;
> +}
> +#viewerPanel .yui3-widget-bd {
> +    padding: 40px 30px 40px;
> +}
> +#viewerPanel .yui3-widget-ft {
> +    background-color: white;

I've not checked, but do we have other .yui3-widgets atm? if not,
should the above just be general rules applying to any
.yui-widget-ft/bd/hd etc?

> +}
>
> === modified file 'src/webcatalog/static/js/carousel.js'
> --- src/webcatalog/static/js/carousel.js        2012-04-18 21:27:46 +0000
> +++ src/webcatalog/static/js/carousel.js        2012-04-20 19:53:20 +0000
> @@ -158,7 +158,7 @@
> @@ -187,5 +187,5 @@
>     });
>     Y.uwc.Carousel = Carousel;
>  }, '0.0.1', {
> -    requires: ['base', 'node', 'anim', 'event']
> +    requires: ['base', 'node', 'anim', 'event','panel', 'transition']

Just a missing space after the comma there.

>  });
>
> === modified file 'src/webcatalog/templates/light/index.1col.html'
> --- src/webcatalog/templates/light/index.1col.html      2012-04-17 13:52:41 +0000
> +++ src/webcatalog/templates/light/index.1col.html      2012-04-20 19:53:20 +0000
> @@ -8,7 +8,7 @@
>         <link rel="stylesheet" type="text/css" href="{% url wc-combo %}?light/css/reset.css&light/css/styles.css&light/css/forms.css"/>
>         {% endblock %}
>     </head>
> -    <body>
> +    <body {% block body_extra %}{% endblock %}>

It'd be worth chatting with maris or matt or whoever is owning the
light theme to see if you can land this there too - or have we given
up trying to minimise our divergence from the light theme? I'm not
sure - perhaps Anthony will know.

>         <div id="container">
>             <div id="container-inner">
>                 <div id="header">
>
> === modified file 'src/webcatalog/templates/webcatalog/application_detail.html'
> --- src/webcatalog/templates/webcatalog/application_detail.html 2012-04-20 18:22:42 +0000
> +++ src/webcatalog/templates/webcatalog/application_detail.html 2012-04-20 19:53:20 +0000
> @@ -39,7 +39,7 @@
>  <meta property="og:title" content="{{ application.name }}" />
>  <meta property="og:image" content="{{ application.icon_url_or_default }}" />
>  {% endblock %}
> -
> +{% block body_extra %}class="yui3-skin-sam"{% endblock %}
>  {% block content %}
>     {% if messages %}
>         {% for message in messages %}
> @@ -133,4 +133,6 @@
>     </div>
>   </div>
>  </div>
> +<div id='viewerPanel'>
> +</div>
>  {% endblock %}
>
> === modified file 'src/webcatalog/templates/webcatalog/screenshot_widget.html'
> --- src/webcatalog/templates/webcatalog/screenshot_widget.html  2012-04-18 21:27:46 +0000
> +++ src/webcatalog/templates/webcatalog/screenshot_widget.html  2012-04-20 19:53:20 +0000
> @@ -18,10 +18,69 @@
>  we could retrieve multiple screenshots via AJAX.  So only skip this section
>  if the app has exactly 1 screenshot.
>  {% endcomment %}
> +<script type="text/javascript">
> +YUI({combine: true, comboBase: '{% url 'wc-combo' %}?', root: 'yui/3.4.0/build/'}).use('io-base', 'uwc-carousel', function(Y) {
> +
> +    Y.on('domready', setupViewerPanel, false);
> +
> +    // viewer needs to be setup even if there is only one screenshot and no carousel is used
> +    function setupViewerPanel(addedMoreViaAjax) {
> +        // don't set up if there are no screenshots
> +        {% if application.screenshots|length_is:0 %}
> +            // but do set up if we get called a second time when adding screenshots via ajax
> +            if (!addedMoreViaAjax) {
> +                return;
> +            }
> +        {% endif %}

Rather than mixing in django template code here in the JS, couldn't it
simply query the DOM to find this out? That would also mean that this
javascript could be moved to a separate file to be loaded by the combo
loader too I think? (as it's quite long...).

> +        var appName = '{{ application.name }}';

Ditto here - I'm assuming we can grab that from the dom.

> +        var panel = new Y.Panel({
> +            width: 860,
> +            height: 580,
> +            centered: true,
> +            zIndex: 50,
> +            modal: true,
> +            visible: false,
> +            render: false,
> +            buttons: [
> +                {
> +                    value: 'Close',
> +                    section: 'footer',
> +                    action: function(e) {
> +                        e.preventDefault();
> +                        bb.transition({
> +                            easing: 'ease-out',
> +                            opacity: {
> +                                duration: 1.25,
> +                                value: 0
> +                            }
> +                        }, function() {
> +                            panel.hide();
> +                        });
> +                    }
> +                }
> +            ]
> +        });
> +        var bb = panel.get('boundingBox');
> +        var slides = Y.all('.slide img');
> +        slides.on("click", function(e){
> +                var slide = e.currentTarget;
> +                var url = slide.getAttribute('src');
> +                heading = appName + ' - screeshot ';
> +                panel.setStdModContent(Y.WidgetStdMod.HEADER, heading);
> +                panel.setStdModContent(Y.WidgetStdMod.BODY, '<img width="800" height="440" src="' + url +'"/>');
> +                panel.render('#viewerPanel');
> +                panel.show();
> +                bb.transition({
> +                    easing: 'ease-in',
> +                    opacity: {
> +                        duration: 1,
> +                        value: 1
> +                    }
> +                });
> +            }, this);
> +    }

 - end of stuff for external file.

> +
>  {% if not application.screenshots|length_is:1 %}

Here too - it wasn't a change in this branch, but this is a simple dom
operation.

> -  <script type="text/javascript">
> -  YUI({combine: true, comboBase: '{% url 'wc-combo' %}?', root: 'yui/3.4.0/build/'}).use('io-base', 'uwc-carousel', function(Y) {
> -
>     function setup_carousel() {
>         var caro = new Y.uwc.Carousel({
>             nodeContainer: "#screenshots-carousel",
> @@ -32,41 +91,42 @@
>         Y.all('.slide').removeClass('disabled');
>     }
>
> -{% if application.screenshots|length_is:0 %}
> -    var uri = "{% url 'wc-package-screenshots' application.package_name %}";
> -    function complete(id, obj) {
> -        var id = id; // Transaction ID.
> -        var json = JSON.parse(obj.responseText);
> -        if (json.length > 1) {
> -            var content_div = document.createElement('div');
> -            content_div.className = 'carousel-wrapper';
> -            var carousel_div = document.createElement('div');
> -            carousel_div.id = 'screenshots-carousel';
> -            carousel_div.className = 'carousel';
> -            content_div.appendChild(carousel_div)
> -            var ol = document.createElement('ol')
> -            ol.className = 'carouselol';
> -            carousel_div.appendChild(ol);
> -            for (var i in json) {
> -                var url = json[i];
> -                var li = document.createElement('li');
> -                li.className = 'slide';
> -                ol.appendChild(li);
> -                var img = document.createElement('img');
> -                img.src = url;
> -                li.appendChild(img);
> +    {% if application.screenshots|length_is:0 %}

This snippet is also very long - it looks like it's receiving the
results of an ajax call and dynamically creating the carousel markup
with the resulting images? I wonder why we didn't put it at least in a
descriptive function when adding it. It's not part of this branch
(you've just indented it right?), but I think we should be cleaning
this stuff up. Ideally it shouldn't have any django template code and
should be able to be moved to an external file too.

> +        var uri = "{% url 'wc-package-screenshots' application.package_name %}";
> +        function complete(id, obj) {
> +            var id = id; // Transaction ID.
> +            var json = JSON.parse(obj.responseText);
> +            if (json.length > 1) {
> +                var content_div = document.createElement('div');
> +                content_div.className = 'carousel-wrapper';
> +                var carousel_div = document.createElement('div');
> +                carousel_div.id = 'screenshots-carousel';
> +                carousel_div.className = 'carousel';
> +                content_div.appendChild(carousel_div)
> +                var ol = document.createElement('ol')
> +                ol.className = 'carouselol';
> +                carousel_div.appendChild(ol);
> +                for (var i in json) {
> +                    var url = json[i];
> +                    var li = document.createElement('li');
> +                    li.className = 'slide';
> +                    ol.appendChild(li);
> +                    var img = document.createElement('img');
> +                    img.src = url;
> +                    li.appendChild(img);
> +                }
> +                var screenshots_div = Y.one('#screenshots_placeholder');
> +                screenshots_div.get('childNodes').remove();
> +                screenshots_div.appendChild(content_div);
> +                setup_carousel();
> +                setupViewerPanel(true);
>             }
> -            var screenshots_div = Y.one('#screenshots_placeholder');
> -            screenshots_div.get('childNodes').remove();
> -            screenshots_div.appendChild(content_div);
> -            setup_carousel();
> -        }
> -    };
> -    Y.on('io:complete', complete, Y);
> -    var request = Y.io(uri);
> -{% else %}
> -    setup_carousel();
> -{% endif %}
> -    });
> -  </script>
> -{% endif %}
> +        };
> +        Y.on('io:complete', complete, Y);
> +        var request = Y.io(uri);
> +    {% else %}
> +        setup_carousel();
> +    {% endif %}
> +{% endif %}
> +});
> +</script>
>
>



-- 
Michael Nelson
Canonical Ltd.
+49 176 491 53481 (mob)
<email address hidden>
IRC nick: noodles (noodles775 on Freenode)
Ubuntu - Linux for human beings | www.ubuntu.com | www.canonical.com