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

[Canan Talayhan <canan.t.talayhan@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3857 bytes --]

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
>

[-- Attachment #2: Type: text/html, Size: 6847 bytes --]