Canan Talayhan <canan.t.talayhan@gmail.com> writes:

> Hi team,
>
> Thanks for your help.
>
> @Chris
> After all the modifications that I've made according to your comments,
> I've created the latest version of my patch.
> Could you please review the patch attached and share your ideas?
>
> Please note that a few parts are left. After your confirmation, I can
> handle it shortly.

Thanks Canan, I've had a quick look:

> +(define page-header "Dumps")
> +
>  (define (view-dumps available-dumps)
>    (layout
> +   #:title
> +   page-header
>     #:body
>     `(,(header)
>       (div
> @@ -31,7 +35,7 @@
>         (@ (class "row"))
>         (div
>          (@ (class "col-sm-12"))
> -        (h1 "Database dumps")))
> +        (h1 ,page-header)))

I'd generally stick with more descriptive titles, "Database dumps" is
far clearer than "Dumps" in this case.

> +  (define page-header "Recent Events")
> +
>    (layout
> +   #:title
> +   page-header
>     #:body
>     `(,(header)
>       (div
> @@ -200,7 +208,7 @@
>          (@ (class "col-sm-12"))
>          (a (@ (href "/jobs"))
>             (h3 "Jobs"))
> -        (h1 "Recent events")))
> +        (h1 ,page-header)))

The capitalisation used here "events" rather than "Events" is
intentional, I wouldn't change it while looking at the titles at least.

>  (define (view-job job-id query-parameters log)
> +  (define page-header "Job")
> +
>    (layout
> +   #:title
> +   (string-append page-header " " job-id)
>     #:body
>     `(,(header)
>       (div
> @@ -339,7 +356,7 @@
>         (@ (class "row"))
>         (div
>          (@ (class "col-sm-12"))
> -        (h1 "Job " ,job-id)))
> +        (h1 ,page-header ," " ,job-id)))

I'd be tempted to combine the Job string and the job-id then use it for
both the #:title and h1 element.

> +   (string-append page-header " " (string-take commit-hash 7) " Channel News Entries")

This title looks good, I'd put the more specific bits nearer the start
though, so I'd put "Channel News Entries" first, then the Revision bit
second. Also, watch out for the line length, I'd indent this so it's not
longer than 80 lines.