[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: patch for list-packages
|
From: |
Ludovic Courtès |
|
Subject: |
Re: patch for list-packages |
|
Date: |
Thu, 15 Aug 2013 16:54:47 +0200 |
|
User-agent: |
Gnus/5.130007 (Ma Gnus v0.7) Emacs/24.3 (gnu/linux) |
Hi Alex,
Alex Sassmannshausen <address@hidden> skribis:
> Yes, sorry — still getting used to gnus as my mail client.
FWIW, in Gnus I make inlined text/x-patch attachments when posting
patches. Yours in this message were marked as text/x-diff, and not
inlined, so I typed ‘t text/x-patch RET’ on each of them, and that’s
fine. :-)
Regarding your work, I really like
<http://www.atheia.org/guix/output-final.html>!
> From c3a9ba4635e0af47423391b9777ec64f872bd2ab Mon Sep 17 00:00:00 2001
> From: Alex Sassmannshausen <address@hidden>
> Date: Sun, 4 Aug 2013 21:46:26 +0200
> Subject: [PATCH 1/3] list-packages: Centralise CSS styling in <head>.
>
> * build-aux/list-packages.scm (package-logo): Assign class of
> 'package-description' to package synopsis div; 'package-logo'. Move inline
> CSS where possible.
> (packages->sxml): Assign id of 'intro' to intro div, 'packages' to the
> table. Move inline CSS.
> (list-packages): Create new <style> section, containing all inline CSS.
> Move JavaScript <script> section to above banner include to place it in
> <head>.
Applied.
> From 1bc1c348e8a792d3ac7b22bbb82ea9285d03f1ea Mon Sep 17 00:00:00 2001
> From: Alex Sassmannshausen <address@hidden>
> Date: Sun, 11 Aug 2013 19:53:15 +0200
> Subject: [PATCH 2/3] list-packages tidying: tidying and refactoring in
> preparation for substantive changes.
>
> * build-aux/list-packages.scm (license package): add title for <a> element.
> (status package): add title for <a> element.
> (package->sxml package): add alt and title for gnu-logo <img> element.
> (package->sxml package): add title to package website <a> element.
> (packages->sxml packages): wrap <div id="intro"> intro paragraph in <p>
> element.
> (packages->sxml packages): add table header row to <table id="packages">
> (packages->sxml packages): add <a> back to top of the page beneath table.
> (insert-css): create new function returning page's CSS; apply whole load of
> new CSS.
> (insert-js): create new function returning page's JavaScript.
> (list-packages . args): move JavaScript to (insert-js).
> (list-packages . args): move CSS to (insert-css).
Applied, with an additional copyright line for you, and with commit log
slightly modified to match GNU ChangeLog conventions (for instance, only
the procedure name is shown, not its formal parameters, and it doesn’t
need to be repeated; new procedures/vars do not need to be documented in
the change log, just mentioned.)
One comment:
> +(define (insert-css)
> + "Return the CSS for the list-packages page."
> + (format #t
> +"<style>
> +a {transition: all 0.3s}
> +div#intro {margin-bottom: 5em}
> +div#intro div, div#intro p {padding:0.5em}
> +div#intro div {float:left}
> +table#packages, table#packages tr, table#packages tbody, table#packages td,
> +table#packages th {border: 0px solid black}
> +div.package-description {position: relative}
> +table#packages tr:nth-child(even) {background-color: #FFF}
> +table#packages tr:nth-child(odd) {background-color: #EEE}
> +table#packages tr:hover, table#packages tr:focus, table#packages tr:active
> {background-color: #DDD}
> +table#packages tr:first-child, table#packages tr:first-child:hover,
> table#packages tr:first-child:focus, table#packages tr:first-child:active {
> +background-color: #333;
> +color: #fff;
> +}
I would rather indent it in C style, for improved readability.
More importantly, I think it’s worth moving to a separate file now,
like:
...
#:use-module (guix build utils)
...
(define %load-directory
(dirname (current-filename)))
(define %css-file
(string-append %load-directory "/package-list.css"))
(define (insert-css)
(with-input-from-file %css-file
(cute dump-port <> (current-output-port))))
WDYT? Could you do this? I think the CSS itself can be put under CC0,
so the file would need the appropriate header.
> From 2b9542cfcc180a6b5fc3b0ed28217eb0896c0d2b Mon Sep 17 00:00:00 2001
> From: Alex Sassmannshausen <address@hidden>
> Date: Sun, 11 Aug 2013 21:40:29 +0200
> Subject: [PATCH 3/3] list-packages.scm: cause description to be shown, and
> 'void' links to not be shown when JS is disabled.
>
> * build-aux/list-packages.scm (package->sxml package): no longer use
> package-synopsis as link as we cannot assume the JavaScript in the link will
> work (no JS safety net); instead use a span.
> (package->sxml package): remove default invisibility of package
> descriptions.
> (package->sxml package): call (show_hide-grouper description-id) after each
> package is processed to collect the package description-ids to be made
> invisible.
> (show_hide-grouper id-or-force): new function; collect n package IDs, and to
> insert a JS call to bulk_show_hide with the package IDs to be processed into
> the xml.
> (packages->sxml packages): call (show_hide-grouper #f) after (map
> package->sxml packages) returns to force the remaining package IDs to be
> processed, even if n have not been reached yet.
> (insert-js): (show_hide(idThing)): add general clause to check whether
> working, DOM conforming JS exists; introduce thingLink variable to allow its
> text to be modified depending on the show/hide package description state of
> a given package.
> (insert-js): (prep(idThing)): new JS function that generates a link in the
> span containing package-synopsis. New link provides show/hide
> package-description functionality.
> (insert-js): (bulk_show_hide()): new JS function that is able to process
> (first call prep, then show_hide on a package-ID) any number of package-IDs
> sequentially (for now defaults to 15, as provided by (show_hide-grouper
> id-or-force)).
Overall OK, modulo two things:
1. Could you adjust the commit log as mentioned above?
2. See below:
> + (define show_hide-grouper
> + (let ((lid '())
> + (group-counter 15))
> + (lambda (id-or-force)
> + "Collect GROUP-COUNTER element IDs, then apply them to
> +bulk_show_hide. Pass ID-OR-FORCE #f to apply collected IDs to bulk_show_hide
> +even when GROUP-COUNTER IDs have not been collected."
Apparently this procedure is only called with #f. Can you remove the
‘id-or-force’ parameter and the ‘else’ case of the ‘cond’ then?
Seems like this would allow you to get rid of ‘lid’ and the ‘set!’
statements, right? (‘set!’ is evil, you know. ;-))
> + (letrec
> + ((lid->js-argl
> + (lambda (l separator)
> + "Parse a list into JavaScript style arguments."
> + (if (null? l)
> + ""
> + (string-append separator "'" (car l) "'"
> + (lid->js-argl (cdr l) ", "))))))
For readability, better use ‘define’ here (internal defines are
equivalent to ‘letrec’.)
Thank you for taking care of this!
Ludo’.