Merge lp:~deadlight/ubuntu-cdimage/ubuntu-cdimage into lp:ubuntu-cdimage

[Karl Williams]

Fix for the nesting of <div>s in the heading of listing pages.

Fix for the nesting of <div>s in the heading of listing pages.

[Steve Langasek (vorlon)]

Could you please run ./run-tests from the tree before pushing?

................................................................................................../lib/cdimage/tree.py:1340:25: E129 visually indented line with same indent as next logical line
./lib/cdimage/tree.py:1340:60: E202 whitespace before ')'
./lib/cdimage/tree.py:1341:80: E501 line too long (93 > 79 characters)
./lib/cdimage/tree.py:1495:59: E261 at least two spaces before inline comment
./lib/cdimage/tree.py:1495:60: E262 inline comment should start with '# '
F.................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
======================================================================
FAIL: test_pep8_clean (cdimage.tests.test_static.TestStatic)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/vorlon/devel/canonical/cdimage/lib/cdimage/tests/test_static.py", line 63, in test_pep8_clean
    self.assertEqual(0, len(output))
AssertionError: 0 != 5

----------------------------------------------------------------------
Ran 580 tests in 9.908s

FAILED (failures=1)

review: Needs Fixing

[lp:~deadlight/ubuntu-cdimage/ubuntu-cdimage]

1756. By Karl Williams <email address hidden> on 2018-11-14

Remove comment and reformat long line

[Karl Williams (deadlight)]

Fixed, sorry about that

[Steve Langasek (vorlon)]

I don't think the fix for the nesting worked as intended: http://releases.ubuntu.com/.bionic-vanilla/

[Steve Langasek (vorlon)]

I don't think the fix for the nesting worked as intended: http://releases.ubuntu.com/.bionic-vanilla/

[Karl Williams (deadlight)]

Hmm, I moved something to the wrong level of the loop.

What parameters are you running the generator with?

On Wed, 14 Nov 2018, 17:26 Steve Langasek, <email address hidden>
wrote:

> I don't think the fix for the nesting worked as intended:
> http://releases.ubuntu.com/.bionic-vanilla/
> --
>
> https://code.launchpad.net/~deadlight/ubuntu-cdimage/ubuntu-cdimage/+merge/358783
> You are the owner of lp:~deadlight/ubuntu-cdimage/ubuntu-cdimage.
>
> Launchpad-Message-Rationale: Owner
> Launchpad-Message-For: deadlight
> Launchpad-Notification-Type: code-review
> Launchpad-Branch: ~deadlight/ubuntu-cdimage/ubuntu-cdimage
> Launchpad-Project: ubuntu-cdimage
>

[Steve Langasek (vorlon)]

On Wed, Nov 14, 2018 at 05:39:24PM -0000, Karl Williams wrote:
> What parameters are you running the generator with?

This was:

DIST=bionic make-web-indices .bionic-vanilla/ ubuntu-18.04.1 release

[Karl Williams (deadlight)]

Great, thanks. I'll create a similar directory and use those params to test.

[lp:~deadlight/ubuntu-cdimage/ubuntu-cdimage]

1757. By Karl Williams <email address hidden> on 2018-11-15

Remove rogue HTML

1758. By Karl Williams <email address hidden> on 2018-11-15

Code style improvement

[Karl Williams (deadlight)]

Ok, I think that *THIS* fixes the HTML nesting.

[Steve Langasek (vorlon)]

Thanks, http://releases.ubuntu.com/.bionic-vanilla/ looks good now, but see http://cdimage.ubuntu.com/ubuntu-server/daily/20181115.vanilla/ which doesn't.

review: Needs Fixing

[Karl Williams (deadlight)]

Thanks Steve...

Multiple options for images needs something.

[lp:~deadlight/ubuntu-cdimage/ubuntu-cdimage]

1759. By Karl Williams <email address hidden> on 2018-11-15

Fix indent dor closing </div> and remove unneeded content

[Karl Williams (deadlight)]

I've tested for a single-image build and one with multiple images per release. It all looks good to me.

[Steve Langasek (vorlon)]

Yes, http://releases.ubuntu.com/.bionic-vanilla/ and http://cdimage.ubuntu.com/ubuntu-server/daily/20181115.vanilla/ both now look good to me. I've asked Dimitri to double-check for anything I've missed.

review: Approve

[Karl Williams (deadlight)]

Dimitri: How's this looking? Can we consider it fixed?

[Karl Waghorn-Moyce (karlwaghorn-moyce)]

Dimitri: Did you get the chance to check Karl's updates?

[Dimitri John Ledkov (xnox)]

This is ok.

There is a further improvement that I would want to see:

* icons should be vertically centered on the left

* Name column must not be wrapped

* and on mobile, table should be `pannable` (such that it takes more than the width of the page, which one can smudge left/right with thumb - no idea how that is called properly) or, like can we force a vertical layout of rows on mobiles? https://css-tricks.com/responsive-data-tables/

review: Approve

[Karl Williams (deadlight)]

Dimitri: Thanks for the feedback.
1. I'll override apache's default top alignment here
2. I'll make sure that the name column gets enough space
3. I can, initially, give the table a min-width to enable the side-scrolling behaviour that you described. A vertical layout might work but is a little more complex, especially when we don't have complete control over how the HTML is generated.

[lp:~deadlight/ubuntu-cdimage/ubuntu-cdimage]

1760. By Karl Williams <email address hidden> on 2018-11-28

Fix widths of table elements to better fit content

1761. By Karl Williams <email address hidden> on 2018-11-28

Make table scroll in the X axis when the content doesn't fit in the viewport

[Karl Williams (deadlight)]

Dimitri: I've pushed some commits to this branch to make the above improvements.

=== modified file 'lib/cdimage/tree.py'
--- lib/cdimage/tree.py	2018-11-12 19:19:26 +0000
+++ lib/cdimage/tree.py	2018-11-28 13:21:16 +0000
@@ -1195,7 +1195,12 @@
             for css in self.cssincludes():
                 print("  @import url(%s);" % css, file=header)
             print(dedent("""\
+                    .p-table-wrapper {
+                        overflow-x: scroll;
+                    }
+
                     table {
+                        min-width: 984px;
                         width: 100%;
                     }
 
@@ -1206,8 +1211,28 @@
 
                     th:first-child,
                     td:first-child {
+                        vertical-align: inherit;
                         width: 5%;
                     }
+
+                    th:nth-of-type(2),
+                    td:nth-of-type(2) {
+                        width: 20em;
+                    }
+
+                    th:nth-of-type(3),
+                    td:nth-of-type(3) {
+                        width: 12em;
+                    }
+
+                    th:nth-of-type(4),
+                    td:nth-of-type(4) {
+                        width: 6em;
+                    }
+
+                    th:nth-of-type(5),
+                    td:nth-of-type(5) {
+                    }
                 </style>
                 """), file=header)
             if self.project == "kubuntu":
@@ -1326,8 +1351,6 @@
                 "https://help.ubuntu.com/community/BitTorrent", "BitTorrent")
 
             for prefix in prefixes:
-                print('<div class="row p-divider"><div class="p-card">',
-                      file=header)
                 for publish_type in all_publish_types:
                     if not self.find_images(directory, prefix, publish_type):
                         continue
@@ -1365,6 +1388,8 @@
                         if not paths:
                             continue
 
+                        print('<div class="row p-divider"><div class="p-card">',
+                            file=header)
                         cdtypestr = self.cdtypestr(publish_type, image_format)
                         print('<div class="col-6 p-divider__block">',
                               file=header)
@@ -1376,20 +1401,6 @@
                             print(tag, file=header)
                             print(file=header)
 
-                        if len(paths) == 1:
-                            print(
-                                "<p>There is one image available:</p>",
-                                file=header)
-                        elif publish_type == "src":
-                            print(
-                                "<p>There are %s images available:</p>" %
-                                self.numbers[len(paths)], file=header)
-                        else:
-                            print(
-                                "<p>There are %s images available, each for a "
-                                "different type of computer:</p>" %
-                                self.numbers[len(paths)], file=header)
-
                         print(file=header)
 
                         print('</div>', file=header)
@@ -1424,7 +1435,6 @@
                                     self.titlecase(cdtypestr), archstr)
                                 archdesc = self.archdesc(arch, publish_type)
 
-                            print("<div>", file=header)
                             if os.path.exists(path):
                                 print(
                                     "<a href=\"%s\">%s</a>" %
@@ -1492,9 +1502,9 @@
                                         htaccessimagestr, extstr,
                                         os.path.basename(extpath)),
                                     file=htaccess)
-                            print("</div>", file=header)
                         print('</div>', file=header)
-                print('</div></div>', file=header)
+                        print('</div></div>', file=header)
+
             published_ec2_path = os.path.join(
                 directory, "published-ec2-%s.txt" % status)
             if os.path.exists(published_ec2_path):
@@ -1616,9 +1626,11 @@
                     print(tag, file=header)
                 print(file=header)
 
+            print("<div class='p-table-wrapper'>", file=header)
+
             print(
                 dedent("""\
-         </div></div></div>
+         </div></div></div></div>
          <footer class="p-footer">
            <div class="row">
              <p><small>&copy; 2018 Canonical Ltd. Ubuntu and Canonical