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

[Canan Talayhan]

Hi Maxime,

Is this ‘introductory task’ publicly available?  If so, could you post a link?

I'm not up-to-date with Outreachy.

It's not available under the Official Outreachy page if you're not logged in.

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>).

It's a good catch : ) done.

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 ...) ...))
    ...)

After sending the patch I've turned the patch like below.

      (title ,(if title
            `,(string-append title " - Guix Data Service")
            '("Guix Data Service")))

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

We're generating titles using the available h1 value to prevent duplication.

Adding space between the id and job could be more readable, I agree. It's done.

Thanks for the improvements,

Canan Talayhan

MSc. in Computer Engineering
Guix IRC: canant

On Tue, Apr 13, 2021 at 2:58 PM Maxime Devos <maximedevos@telenet.be> wrote:

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