Re: [Outreachy] - Guix Data Service - Set a more informative page title

[Maxime Devos]

On Tue, 2021-04-13 at 12:01 +0300, Canan Talayhan wrote:
> Hi everyone,
Welcome!

> My name is Canan. I'm an Outreachy applicant. I'm working on the introductory 
> task for
> Guix Data Service.

Is this ‘introductory task’ publicly available?  If so, could you post a link?
I'm not up-to-date with Outreachy.

> Introductory task:
> Set a more informative page title for any page where the title is "Guix Data 
> Service"
> 
> I've created a patch for the "Jobs" page. If it looks good for everyone then 
> I can proceed with
>  other applicable pages.
> 
> Now, I'm working on the title part of the code snippet below to make it more 
> elegant.
> 
> ```scm
> (define* (layout #:key
>                  (head '())
>                  (body '())
>                  title
>                  description)
>   `((doctype "html")
>     (html
>      (@ (lang "en"))
>      (head
>       ,@(if title
>             `((title ,(string-append title " - Guix Data Service")))

Typographical nitpick: I would use a proper dash here (the figure dash ‒ or the 
—
em dash, I always forgot which is correct), instead of the hyphen-minus -.
(Resource: <https://en.wikipedia.org/wiki/Dash#Figure_dash>).

>             `((title "Guix Data Service")))
> ```

You could bring the 'if' inside:
  `((title ,(if title
                (string-append title " ‒ Guix Data Service")
                "Guix Data Service")))


and a simplification is possible, as adding a title is unconditional:
  (head
    (title ,(if title (string-append ...) ...))
    ...)

> Could you please review and share your comments? I'll be appreciated.
> 
> Attached file: jobs-title-and-view.diff

+  (define page-header"Jobs")

Nitpick: I would put a space before "Jobs"?  Also, what's the point of
defining this variable, if it is constant?

+  (define page-header "Job")
+
   (layout
+   #:title
+   (string-append page-header job-id)

IIRC, job ids are integers, so this would result in titles
like "Job1234"?  Titles like "Job 1234" would be better.

I haven't tested the patch, but better page titles are good, thanks!

Maxime.
Sometimes reviewing patches, but without actually testing them.
Guix IRC: mdevos

signature.asc
Description: This is a digitally signed message part