From mboxrd@z Thu Jan 1 00:00:00 1970 From: ludo@gnu.org (Ludovic =?utf-8?Q?Court=C3=A8s?=) Subject: Re: Patches: Trivial (add div), Progressive Enhancement, Externalise CSS/JS Date: Mon, 19 Aug 2013 00:43:17 +0200 Message-ID: <87r4dqsjqy.fsf@gnu.org> References: Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:50005) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VBBqi-0002ki-Gf for guix-devel@gnu.org; Sun, 18 Aug 2013 18:53:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VBBqb-0007lS-56 for guix-devel@gnu.org; Sun, 18 Aug 2013 18:53:28 -0400 Received: from hera.aquilenet.fr ([141.255.128.1]:60367) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VBBqa-0007lK-Rx for guix-devel@gnu.org; Sun, 18 Aug 2013 18:53:21 -0400 In-Reply-To: (alex sassmannshausen's message of "Sun, 18 Aug 2013 14:55:28 +0200") List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org To: alex sassmannshausen Cc: guix-devel@gnu.org alex sassmannshausen 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=E2=80=99s 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=E2=80=A6), 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=E2=80=99d 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 > Date: Sun, 18 Aug 2013 13:34:05 +0200 > Subject: [PATCH 1/3] list-packages: Add missing closing after foot= er > include. > > * build-aux/list-packages.scm (list-packages): Add missing closing > after footer include. Applied. > From fcbf81a5164e49a385bd1cd28a67e59d868a1cbf Mon Sep 17 00:00:00 2001 > From: Alex Sassmannshausen > Date: Sun, 18 Aug 2013 13:43:05 +0200 > Subject: [PATCH 2/3] list-packages: Progressive Enhancement approach to J= S. > > * build-aux/list-packages.scm (package->sxml): Remove element and JS > function calls. These are now provided through (show_hide-grouper) belo= w. > 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 thingLi= nk. > prep: new JS function. > bulk_show_hide: new JS function. [...] > + (td (span (strong ,(package-synopsis package))) Shouldn=E2=80=99t we use CSS instead of ? > +(define show_hide-grouper I should have mentioned it: the name of a procedure should describe its result or computation (when it=E2=80=99s a pure function, like =E2=80=98exp= t=E2=80=99), or its effect (like =E2=80=98display=E2=80=99). Otherwise it=E2=80=99s 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 =E2=80=98display=E2=80=99 with =E2=80=98Take=E2=80=99. > From 3c3bfb6dea1b20447ba8bb48ec95a8cc4b556129 Mon Sep 17 00:00:00 2001 > From: Alex Sassmannshausen > 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 element calling= the > external CSS stylesheet. Contains structure for inlining the CSS from t= he > 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 f= ile. > (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 =E2=80=98inline=E2=80=99 parameter, and keep only the second arm= of the =E2=80=98if=E2=80=99 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. > > GNU Guix - GNU Distribution - GNU Project > ") > - (insert-css) > - (insert-js) > + (sxml->xml `(,(insert-css) > + ,(insert-js))) =E2=80=98insert-css=E2=80=99 and =E2=80=98insert-js=E2=80=99 return the uns= pecified 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=E2=80=99.