Re: Patches: Trivial (add div), Progressive Enhancement, Externalise CSS/JS

[Ludovic Courtès]

alex sassmannshausen <address@hidden> skribis:

> The second patch re-introduces changes to the SXML and JavaScript to
> fulfill the criteria of Progressive Enhancement:
> - All content is shown when JavaScript is not enabled on GUI browsers.
> In addition the patch implements JS that carries out the changes to the
> HTML document successively rather than all at the end, which, with the
> current size of the page, would cause a 'flicker'.

Nice.  (What’s Progressive Enhancement?)

> I have improved the changelog, as well as, I believe, made the functions
> that these changes require, clearer.
> Ludo, I appreciate set! being evil (maybe not the true extent yet
> though…), but I believe it to be necessary on this occasion:

More on that below.

> Currently show_hide-grouper inserts roughly 31 JavaScript calls to
> bulk_show hide (1 every 15 package descriptions + 1 at the end to
> collect the remaining package descriptions) in the final HTML
> document. Hope this makes sense; if you can think of a better way of
> doing it please let me know.

I trust you.  :-)

> The third patch finally moves the CSS and JS into separate files, as
> suggested by Ludo. I have implemented this so that by default, the page
> simply links to those external files (this allows browsers to cache the
> JS and CSS independently from the HTML). This does mean that those 2
> additional files (package-list.js and package-list.css) need to be
> pushed to the Guix site whenever changes are carried out to the CSS/JS.

I’d prefer inlining, to simplify web site maintenance.

> The alternative, inlining the CSS and JS in the resulting HTML, which is
> I think what was proposed, has been implemented as suggested, but I'm
> running into an error

Can you send the backtrace that you get, and/or the error message?

> From 01d1183d310a404500ecfda2e3bee4c996d580f8 Mon Sep 17 00:00:00 2001
> From: Alex Sassmannshausen <address@hidden>
> Date: Sun, 18 Aug 2013 13:34:05 +0200
> Subject: [PATCH 1/3] list-packages: Add missing closing </div> after footer
>  include.
>
> * build-aux/list-packages.scm (list-packages): Add missing closing </div>
>   after footer include.

Applied.

> From fcbf81a5164e49a385bd1cd28a67e59d868a1cbf Mon Sep 17 00:00:00 2001
> From: Alex Sassmannshausen <address@hidden>
> Date: Sun, 18 Aug 2013 13:43:05 +0200
> Subject: [PATCH 2/3] list-packages: Progressive Enhancement approach to JS.
>
> * build-aux/list-packages.scm (package->sxml): Remove <a> element and JS
>   function calls. These are now provided through (show_hide-grouper) below.
>   Add call to (show_hide-grouper) for every package added to the table.
>   (show_hide-grouper): New function.
>   (packages->sxml): Add final call to (show_hide-grouper).
>   (insert-js): show_hide: add compatibility check, introduce, use thingLink.
>                prep: new JS function.
>                bulk_show_hide: new JS function.

[...]

> +        (td (span (strong ,(package-synopsis package)))

Shouldn’t we use CSS instead of <strong>?

> +(define show_hide-grouper

I should have mentioned it: the name of a procedure should describe its
result or computation (when it’s a pure function, like ‘expt’), or its
effect (like ‘display’).  Otherwise it’s harder to tell what it does etc.

> +  (let ((lid '())
> +        (group-counter 15))
> +    (lambda (id)
> +      "Collect GROUP-COUNTER element IDs, then apply them to
> +bulk_show_hide. If ID is #f, force the application of collected IDs to
> +bulk_show_hide even when the number of IDs is smaller than GROUP-COUNTER."
> +      (define (insert-js-call)
> +        "Return an sxml call to bulk_show_hide."
> +        (define (lid->js-argl)
> +          "Parse a Scheme list into a JavaScript style arguments list."
> +          (define (helper l)
> +            (if (null? l)
> +                (begin
> +                  (set! lid '()) ; lid, now processed, safe to reset.
> +                  "")
> +                (string-append ", '" (car l) "'"      ; further args.
> +                               (helper (cdr l)))))
> +          (string-append "'" (car lid) "'"             ; initial arg.
> +                         (helper (cdr lid))))
> +        `(script (@ (type "text/javascript"))
> +                 ,(format #f "bulk_show_hide(~a)"
> +                          (lid->js-argl))))
> +      (if id
> +          (begin
> +            ;; If ID is not false, then we add ID to LID.

But this function still has a single call with #f as its argument.  Can
you remove the parameter and the dead code?  Or am I missing something?

[...]

> +/* Take n element IDs, prepare them for javascript enhanced
> +display and hide the IDs by default. */
> +function bulk_show_hide()

Align ‘display’ with ‘Take’.

> From 3c3bfb6dea1b20447ba8bb48ec95a8cc4b556129 Mon Sep 17 00:00:00 2001
> From: Alex Sassmannshausen <address@hidden>
> Date: Sun, 18 Aug 2013 15:23:03 +0200
> Subject: [PATCH 3/3] list-packages: Externalise CSS and JavaScript.
>
> * build-aux/list-packages.scm (%load-directory): New variable.
>   (%css-file): New variable.
>   (%js-file): New variable.
>   (insert-css): Rewrite to, by default, generate a <link> element calling the
>   external CSS stylesheet. Contains structure for inlining the CSS from the
>   external stylesheet.
>   (insert-js): Rewrite to, by default, call external JS file, rather than
>   inlining it. Contains structure for inlining the JS from the external file.
>   (list-packages): generate sxml from insert-css and insert-js.
> * package-list.css: New CSS file, with minor tweaks.
> * package-list.js: New JS file, identical to last commit.

Looks good!  A couple of details:

> +(define %load-directory
> +  (string-append (dirname (current-filename)) "/"))

No need for the trailing slash.

> +(define %css-file
> +  "package-list.css")
> +(define %js-file
> +  "package-list.js")

Should be (string-append %load-directory "/package-list.css"), to make
sure it works regardless of the current working directory.

> +(define (insert-css . inline)
> +  "Return an sxml link to the CSS file for the list-packages page.  If
> +the optional inline argument is present, inline the CSS instead.
> +Inlining currently fail with wrong-num-args error."
> +  (if (null? inline)
> +      `(link (@ (type "text/css")
> +             (rel "stylesheet")
> +             (href ,%css-file)))
> +      `(style (@ (type "text/css"))
> +      ,(with-input-from-file (string-append %load-directory %css-file)
> +         (cute dump-port <> (current-output-port))))))

Remove the ‘inline’ parameter, and keep only the second arm of the ‘if’
for now.

> +(define (insert-js . inline)
> +  "Return an sxml link to the CSS file for the list-packages page.  If
> +the optional inline argument is present, inline the CSS instead.
> +Inlining currently fail with wrong-num-args error."
> +  (if (null? inline)
> +      `(script (@ (type "text/javascript")
> +                  (src ,%js-file))
> +               " ")
> +      `(style (@ (type "text/javascript"))
> +      ,(with-input-from-file (string-append %load-directory %js-file)
> +         (cute dump-port <> (current-output-port))))))

Ditto.

>  <!-- Parent-Version: 1.70 $ -->
>  <title>GNU Guix - GNU Distribution - GNU Project</title>
>  ")
> -   (insert-css)
> -   (insert-js)
> +   (sxml->xml `(,(insert-css)
> +                ,(insert-js)))

‘insert-css’ and ‘insert-js’ return the unspecified value; instead, they
write to the current output port.

So I guess you rather want to keep the two calls as before, no?

> diff --git a/build-aux/package-list.css b/build-aux/package-list.css
> new file mode 100644
> index 0000000..67d423a
> --- /dev/null
> +++ b/build-aux/package-list.css

Can you indent this file in C-mode in Emacs or something similar?

Thanks for following up!

Ludo’.