Merge lp:~zematynnad/ubuntu-webcatalog/screenshot_enlarge_845911 into lp:ubuntu-webcatalog

[Danny Tamez]

Adds a panel for enlarged application screenshot images.

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

To Test
========
$fab bootstrap test

[lp:~zematynnad/ubuntu-webcatalog/screenshot_enlarge_845911]

109. By Danny Tamez on 2012-04-20

Merge from trunk

110. By Danny Tamez on 2012-04-20

Handle case where the viewer is needed for only 1 screenshot returned via ajax.

111. By Danny Tamez on 2012-04-20

Removing unneeded assignment.

[Michael Nelson (michael.nelson)]

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 ...

[lp:~zematynnad/ubuntu-webcatalog/screenshot_enlarge_845911]

112. By Danny Tamez on 2012-05-08

Moved javascript into a reusable function in a separate js file.

113. By Danny Tamez on 2012-05-08

Adding js file for screenshots.

114. By Danny Tamez on 2012-05-18

Merge from trunk

115. By Danny Tamez on 2012-05-18

Removing transitions from screenshot enlarger.

116. By Danny Tamez on 2012-05-18

Make the height variable to accomodate different screenshot aspect ratios.

[Danny Tamez (zematynnad)]

Naty,

I've tried to reproduce the problem you're experiencing with the header in 4 different browsers with no luck. Could you please let me know the browser and version you are using to see if I can perhaps duplicate it using that same browser/version?

[Natalia Bidart (nataliabidart)]

I'm running Firefox 12.0.

[Natalia Bidart (nataliabidart)]

IRL tested. It works pretty good, there is an issue in a weird, hard-to-reproduce, corner case, that I'm not sure is worth delaying this branch any longer (you can see the video showing this at http://ubuntuone.com/3O3eXt5SH8ZD2yutoZVuNj).

So, Danny and I agreed that:

(04:55:53 PM) zematynnad: do you want to run it by achuni or just leave a note and approve?
(04:56:34 PM) nessita: I can approve, and perhaps you can run this by achuni to get a final sign off? mostly because I'm such an ignorant when it comes to javascript
(04:56:57 PM) zematynnad: sure thing

review: Approve

[Danny Tamez (zematynnad)]

adding small css for pointer: http://simplest-image-hosting.net/png-0-qn2454%0A

[lp:~zematynnad/ubuntu-webcatalog/screenshot_enlarge_845911]

117. By Danny Tamez on 2012-05-23

Adding cursor pointer when over the screenshot.

[Anthony Lenton (elachuni)]

review: Approve

=== 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-05-23 22:59:16 +0000
@@ -204,7 +204,8 @@
 }
 div.screenshot .carousel-wrapper .slide img {
     margin: 0 auto;
-    display: block
+    display: block;
+    cursor: pointer;
 }
 div.screenshot .carousel .pagination li a.active {
     background-color: #DD4814;
@@ -219,3 +220,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;
+}

=== 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-05-23 22:59:16 +0000
@@ -158,7 +158,7 @@
                 this.caroAnim.run();
                 this.caroAnim.set("duration", origDuration);
             }
-        },
+        }
     }, {
         NAME: "carousel",
         ATTRS: {
@@ -187,5 +187,5 @@
     });
     Y.uwc.Carousel = Carousel;
 }, '0.0.1', {
-    requires: ['base', 'node', 'anim', 'event']
+    requires: ['base', 'node', 'anim', 'event','panel', 'transition']
 });

=== added file 'src/webcatalog/static/js/screenshots.js'
--- src/webcatalog/static/js/screenshots.js	1970-01-01 00:00:00 +0000
+++ src/webcatalog/static/js/screenshots.js	2012-05-23 22:59:16 +0000
@@ -0,0 +1,96 @@
+var setup_screenshots = function(num_screenshots, appName, combo_path, ajax_uri) {
+    YUI({combine: true, comboBase: combo_path, 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 (num_screenshots == 0) {
+                // but do set up if we get called a second time when adding screenshots via ajax
+                if (!addedMoreViaAjax) {
+                    return;
+                }
+            }
+            var panel = new Y.Panel({
+                width: 860,
+                centered: true,
+                zIndex: 50,
+                modal: true,
+                visible: false,
+                render: false,
+                buttons: [
+                    {
+                        value: 'Close',
+                        section: 'footer',
+                        action: function(e) {
+                            e.preventDefault();
+                            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" src="' + url +'"/>');
+                    panel.render('#viewerPanel');
+                    panel.show();
+                }, this);
+        }
+
+        if (num_screenshots > 1) {
+            function setup_carousel() {
+                var caro = new Y.uwc.Carousel({
+                    nodeContainer: "#screenshots-carousel",
+                    controlsContainer: "#nocontrols",
+                    autoPlay: false,
+                    slideAnimDuration: 0.6
+                });
+                Y.all('.slide').removeClass('disabled');
+            }
+        }
+
+        if (num_screenshots == 0) {
+            var uri = ajax_uri;
+            function complete(id, obj) {
+                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);
+                } else if (json.lenth == 1) {
+                    // in case we only got back 1 from ajax - still need the viewer
+                    setupViewerPanel(true);
+                }
+            };
+            Y.on('io:complete', complete, Y);
+            var request = Y.io(uri);
+        } else {
+            setup_carousel();
+        }
+    });
+}

=== 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-05-23 22:59:16 +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 %}>
         <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-05-04 10:20:34 +0000
+++ src/webcatalog/templates/webcatalog/application_detail.html	2012-05-23 22:59:16 +0000
@@ -6,7 +6,7 @@
 {% block header %}Details for {{ application.name }}{% endblock %}
 {% block head_extra %}
 <link rel="stylesheet" type="text/css" href="{% url wc-combo %}?light/css/reset.css&light/css/styles.css&css/webcatalog.css&light/css/forms.css&css/carousel.css"/>
-<script src="{% url wc-combo %}?yui/3.4.0/build/yui/yui-min.js&js/carousel.js"></script>
+<script src="{% url wc-combo %}?yui/3.4.0/build/yui/yui-min.js&js/carousel.js&js/screenshots.js"></script>
 <script>
 YUI({combine: true, comboBase: '{% url wc-combo %}?', root: 'yui/3.4.0/build/'}).use('io-base', 'node-base', function (Y) {
 
@@ -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 %}
@@ -141,4 +141,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-05-23 22:59:16 +0000
@@ -1,4 +1,5 @@
 {% load url from future %}
+<div id='console'></div>
 <div id="screenshots_placeholder">
 {% if application.screenshots|length_is:0 %}
     <img src="http://screenshots.ubuntu.com/thumbnail-with-version/{{ application.package_name }}/ignored" />
@@ -12,61 +13,6 @@
     </div>
 {% endif %}
 </div>
-
-{% comment %}
-If there are multiple screenshots, display a carousel.  If there are none,
-we could retrieve multiple screenshots via AJAX.  So only skip this section
-if the app has exactly 1 screenshot.
-{% endcomment %}
-{% if not application.screenshots|length_is:1 %}
-  <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",
-            controlsContainer: "#nocontrols",
-            autoPlay: false,
-            slideAnimDuration: 0.6
-        });
-        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);
-            }
-            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 %}
+<script type="text/javascript" charset="utf-8">
+    setup_screenshots({{ application.screenshots|length  }}, '{{ application.package_name }}', '{% url 'wc-combo' %}?', '{% url 'wc-package-screenshots' application.package_name %}');
+</script>

=== modified file 'src/webcatalog/tests/test_views.py'
--- src/webcatalog/tests/test_views.py	2012-05-23 22:28:39 +0000
+++ src/webcatalog/tests/test_views.py	2012-05-23 22:59:16 +0000
@@ -338,23 +338,6 @@
 
         self.assertNotContains(response, '<th>Version</th>')
 
-    def test_with_only_one_screenshot_there_is_no_carousel(self):
-        response, _ = self.get_app_and_response(
-            screenshot_url='http://example.com/screenshot.png')
-
-        self.assertNotContains(response, 'Y.uwc.Carousel')
-
-    def test_with_multiple_screenshots_there_is_js_carousel(self):
-        app = self.factory.make_application()
-        for i in range(3):
-            app.applicationmedia_set.create(
-                url='http://example.com/{0}.png'.format(i),
-                media_type='screenshot')
-
-        response = self.client.get(self.get_app_details_url(app))
-
-        self.assertContains(response, 'Y.uwc.Carousel', 1)
-
     def test_debtags_displayed(self):
         app = self.factory.make_application(debtags='["joystick"]')