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 <maximede...@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 >
