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

Proposed by Karl Williams
Status: Merged
Merged at revision: 1763
Proposed branch: lp:~deadlight/ubuntu-cdimage/ubuntu-cdimage
Merge into: lp:ubuntu-cdimage
Diff against target: 117 lines (+32/-20)
1 file modified
lib/cdimage/tree.py (+32/-20)
To merge this branch: bzr merge lp:~deadlight/ubuntu-cdimage/ubuntu-cdimage
Reviewer Review Type Date Requested Status
Dimitri John Ledkov (community) Approve
Steve Langasek Approve
Review via email: mp+358783@code.launchpad.net

Commit message

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

Description of the change

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

To post a comment you must log in.
Revision history for this message
Steve Langasek (vorlon) wrote :

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
1756. By Karl Williams <email address hidden>

Remove comment and reformat long line

Revision history for this message
Karl Williams (deadlight) wrote :

Fixed, sorry about that

Revision history for this message
Steve Langasek (vorlon) wrote :

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

Revision history for this message
Steve Langasek (vorlon) wrote :

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

Revision history for this message
Karl Williams (deadlight) wrote :

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
>

Revision history for this message
Steve Langasek (vorlon) wrote :

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

Revision history for this message
Karl Williams (deadlight) wrote :

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

1757. By Karl Williams <email address hidden>

Remove rogue HTML

1758. By Karl Williams <email address hidden>

Code style improvement

Revision history for this message
Karl Williams (deadlight) wrote :

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

Revision history for this message
Steve Langasek (vorlon) wrote :
review: Needs Fixing
Revision history for this message
Karl Williams (deadlight) wrote :

Thanks Steve...

Multiple options for images needs something.

1759. By Karl Williams <email address hidden>

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

Revision history for this message
Karl Williams (deadlight) wrote :

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

Revision history for this message
Steve Langasek (vorlon) wrote :

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
Revision history for this message
Karl Williams (deadlight) wrote :

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

Revision history for this message
Karl Waghorn-Moyce (karlwaghorn-moyce) wrote :

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

Revision history for this message
Dimitri John Ledkov (xnox) wrote :

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
Revision history for this message
Karl Williams (deadlight) wrote :

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.

1760. By Karl Williams <email address hidden>

Fix widths of table elements to better fit content

1761. By Karl Williams <email address hidden>

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

Revision history for this message
Karl Williams (deadlight) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
1195 for css in self.cssincludes():1195 for css in self.cssincludes():
1196 print(" @import url(%s);" % css, file=header)1196 print(" @import url(%s);" % css, file=header)
1197 print(dedent("""\1197 print(dedent("""\
1198 .p-table-wrapper {
1199 overflow-x: scroll;
1200 }
1201
1198 table {1202 table {
1203 min-width: 984px;
1199 width: 100%;1204 width: 100%;
1200 }1205 }
12011206
@@ -1206,8 +1211,28 @@
12061211
1207 th:first-child,1212 th:first-child,
1208 td:first-child {1213 td:first-child {
1214 vertical-align: inherit;
1209 width: 5%;1215 width: 5%;
1210 }1216 }
1217
1218 th:nth-of-type(2),
1219 td:nth-of-type(2) {
1220 width: 20em;
1221 }
1222
1223 th:nth-of-type(3),
1224 td:nth-of-type(3) {
1225 width: 12em;
1226 }
1227
1228 th:nth-of-type(4),
1229 td:nth-of-type(4) {
1230 width: 6em;
1231 }
1232
1233 th:nth-of-type(5),
1234 td:nth-of-type(5) {
1235 }
1211 </style>1236 </style>
1212 """), file=header)1237 """), file=header)
1213 if self.project == "kubuntu":1238 if self.project == "kubuntu":
@@ -1326,8 +1351,6 @@
1326 "https://help.ubuntu.com/community/BitTorrent", "BitTorrent")1351 "https://help.ubuntu.com/community/BitTorrent", "BitTorrent")
13271352
1328 for prefix in prefixes:1353 for prefix in prefixes:
1329 print('<div class="row p-divider"><div class="p-card">',
1330 file=header)
1331 for publish_type in all_publish_types:1354 for publish_type in all_publish_types:
1332 if not self.find_images(directory, prefix, publish_type):1355 if not self.find_images(directory, prefix, publish_type):
1333 continue1356 continue
@@ -1365,6 +1388,8 @@
1365 if not paths:1388 if not paths:
1366 continue1389 continue
13671390
1391 print('<div class="row p-divider"><div class="p-card">',
1392 file=header)
1368 cdtypestr = self.cdtypestr(publish_type, image_format)1393 cdtypestr = self.cdtypestr(publish_type, image_format)
1369 print('<div class="col-6 p-divider__block">',1394 print('<div class="col-6 p-divider__block">',
1370 file=header)1395 file=header)
@@ -1376,20 +1401,6 @@
1376 print(tag, file=header)1401 print(tag, file=header)
1377 print(file=header)1402 print(file=header)
13781403
1379 if len(paths) == 1:
1380 print(
1381 "<p>There is one image available:</p>",
1382 file=header)
1383 elif publish_type == "src":
1384 print(
1385 "<p>There are %s images available:</p>" %
1386 self.numbers[len(paths)], file=header)
1387 else:
1388 print(
1389 "<p>There are %s images available, each for a "
1390 "different type of computer:</p>" %
1391 self.numbers[len(paths)], file=header)
1392
1393 print(file=header)1404 print(file=header)
13941405
1395 print('</div>', file=header)1406 print('</div>', file=header)
@@ -1424,7 +1435,6 @@
1424 self.titlecase(cdtypestr), archstr)1435 self.titlecase(cdtypestr), archstr)
1425 archdesc = self.archdesc(arch, publish_type)1436 archdesc = self.archdesc(arch, publish_type)
14261437
1427 print("<div>", file=header)
1428 if os.path.exists(path):1438 if os.path.exists(path):
1429 print(1439 print(
1430 "<a href=\"%s\">%s</a>" %1440 "<a href=\"%s\">%s</a>" %
@@ -1492,9 +1502,9 @@
1492 htaccessimagestr, extstr,1502 htaccessimagestr, extstr,
1493 os.path.basename(extpath)),1503 os.path.basename(extpath)),
1494 file=htaccess)1504 file=htaccess)
1495 print("</div>", file=header)
1496 print('</div>', file=header)1505 print('</div>', file=header)
1497 print('</div></div>', file=header)1506 print('</div></div>', file=header)
1507
1498 published_ec2_path = os.path.join(1508 published_ec2_path = os.path.join(
1499 directory, "published-ec2-%s.txt" % status)1509 directory, "published-ec2-%s.txt" % status)
1500 if os.path.exists(published_ec2_path):1510 if os.path.exists(published_ec2_path):
@@ -1616,9 +1626,11 @@
1616 print(tag, file=header)1626 print(tag, file=header)
1617 print(file=header)1627 print(file=header)
16181628
1629 print("<div class='p-table-wrapper'>", file=header)
1630
1619 print(1631 print(
1620 dedent("""\1632 dedent("""\
1621 </div></div></div>1633 </div></div></div></div>
1622 <footer class="p-footer">1634 <footer class="p-footer">
1623 <div class="row">1635 <div class="row">
1624 <p><small>&copy; 2018 Canonical Ltd. Ubuntu and Canonical1636 <p><small>&copy; 2018 Canonical Ltd. Ubuntu and Canonical

Subscribers

People subscribed via source and target branches