From: ludo@gnu.org (Ludovic Courtès)
To: alex sassmannshausen <alex.sassmannshausen@gmail.com>
Cc: guix-devel@gnu.org
Subject: Re: Patches: Trivial (add div), Progressive Enhancement, Externalise CSS/JS
Date: Mon, 19 Aug 2013 00:43:17 +0200 [thread overview]
Message-ID: <87r4dqsjqy.fsf@gnu.org> (raw)
In-Reply-To: <CAKdwf2BhW0pntTN_YO-4kNcWhcXAw120U9JQnt8xDWKtepcUnw@mail.gmail.com> (alex sassmannshausen's message of "Sun, 18 Aug 2013 14:55:28 +0200")
alex sassmannshausen <alex.sassmannshausen@gmail.com> 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 <alex.sassmannshausen@gmail.com>
> 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 <alex.sassmannshausen@gmail.com>
> 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 <alex.sassmannshausen@gmail.com>
> 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’.
next prev parent reply other threads:[~2013-08-18 22:53 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-08-18 12:55 Patches: Trivial (add div), Progressive Enhancement, Externalise CSS/JS alex sassmannshausen
2013-08-18 22:43 ` Ludovic Courtès [this message]
2013-08-19 23:21 ` Alex Sassmannshausen
2013-08-19 23:55 ` Alex Sassmannshausen
2013-08-26 14:16 ` Patches: Progressive Enhancement Alex Sassmannshausen
2013-08-26 14:25 ` Alex Sassmannshausen
2013-08-28 12:34 ` Ludovic Courtès
2013-09-22 14:11 ` Alex Sassmannshausen
2013-09-23 15:54 ` Ludovic Courtès
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://guix.gnu.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87r4dqsjqy.fsf@gnu.org \
--to=ludo@gnu.org \
--cc=alex.sassmannshausen@gmail.com \
--cc=guix-devel@gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/guix.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).