From: Christopher Baines <mail@cbaines.net>
To: Canan Talayhan <canan.t.talayhan@gmail.com>
Cc: guix-devel@gnu.org
Subject: Re: [Outreachy] - Guix Data Service - Set a more informative page title
Date: Thu, 22 Apr 2021 20:46:57 +0100 [thread overview]
Message-ID: <878s5ahzke.fsf@cbaines.net> (raw)
In-Reply-To: <CAAosC5JphwX2AFXmcYxD0R6ysFm_8dOhVBB6yhibvT3dCWEneA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 6037 bytes --]
Canan Talayhan <canan.t.talayhan@gmail.com> writes:
> I've missed it unintentionally. I've not touched the "@" sign this time. :)
>
>>> + (define page-header "Comparing")
> >> +
> >> (layout
> >> + #:title
> >> + (string-append page-header " " (string-take base-commit 8) " and "
> >> + (string-take target-commit 8))
> >> #:body
> >> `(,(header)
> >> (div
> >> @@ -107,7 +112,7 @@
> >> (@ (class "col-sm-7"))
> >> ,@(if invalid-query?
> >> `((h1 "Compare"))
> >> - `((h1 "Comparing "
> >> + `((h1 ,page-header ," "
> >> (a (@ (href ,(string-append "/revision/" base-commit)))
> >> (samp ,(string-take base-commit 8) "…"))
> >> " and "
>
> I think I misunderstood that comment.
>>> There's a couple of things here. I'd be tempted not to use a variable
>>> for "Comparing", it's not really the page header, as that's more
>>> complicated, so I think I'd just use the string in both places.
>
> Now, I've fixed it this way. I hope this version is good.
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
> *(layout #:title (string-append "Comparing " (string-take base-commit
> 8) " and " (string-take target-commit 8)) #:body `(,(header)
> (div (@ (class "container")) (div (@ (class "row"))
> (div (@ (class "col-sm-7")) ,@(if invalid-query?
> `((h1 "Compare")) `((h1 "Comparing " (a
> (@ (href ,(string-append "/revision/" base-commit)))
> (samp ,(string-take base-commit 8) "…")) " and "*
>
I think your email came through a bit garbled, anyway, I think there's
still an issue with the title here.
> On Mon, Apr 19, 2021 at 10:16 PM Christopher Baines <mail@cbaines.net>
> wrote:
>
>>
>> Canan Talayhan <canan.t.talayhan@gmail.com> writes:
>>
>> > Thanks for your quick response.
>> >
>> >>Why's the @ being removed here?
>> > It interprets like an HTML code when I use the page-header like
>> > `,page-header, so I removed it. According to your comment, I reverted
>> > to the original version.
>> >
>> > " 'GET repository..." which includes package/package-name in the URL
>> > has not the best titles since I couldn't test them because of the
>> > error that I've mentioned.
>> > I'm open to suggestions.
>> >
>> > Could you please re-review the patch that contains all the
>> > modifications you've mentioned in the previous message?
>>
>> I've had another look, see my comments below.
>>
>> > On Sun, Apr 18, 2021 at 8:53 PM Christopher Baines <mail@cbaines.net>
>> wrote:
>> >>
>> >>
>> >> Canan Talayhan <canan.t.talayhan@gmail.com> writes:
>> >>
>> >> > I've updated the patch that contains all the suggestions. I think the
>> patch
>> >> > is ready to merge.
>> >> >
>> >> > One thing that I would like to ask you about the package and
>> package-name
>> >> > in web/repository/controller.scm.
>> >> >
>> >> > When I test the URL below I'm getting this error. (
>> >> > https://pastebin.ubuntu.com/p/HdKShmKqH7/)
>> >> >
>> >> > - ('GET "repository" repository-id "branch" branch-name "package"
>> >> > package-name) ->
>> >> > http://localhost:8765/repository/1/branch/master/package/emacs
>> >> >
>> >> > What do you think? BTW it's accessible on the official server.
>> >> >
>> >> > -
>> https://data.guix.gnu.org/repository/1/branch/master/package/emacs/
>> >>
>> >> Hmm, this could possibly be due to an issue with the small dump of the
>> >> database.
>> >>
>> >> > Could you please review the patch attached?
>> >> > I'm very excited to make my first FOSS contribution. :)
>> >>
>> >> I've had a look though, and I have some more comments:
>> >>
>> >> diff --git a/guix-data-service/web/compare/html.scm
>> b/guix-data-service/web/compare/html.scm
>> >> index 5b5fe0a..170fb12 100644
>> >> --- a/guix-data-service/web/compare/html.scm
>> >> +++ b/guix-data-service/web/compare/html.scm
>> >> @@ -96,7 +96,12 @@
>> >> (unless invalid-query?
>> >> (query-parameters->string query-parameters)))
>> >>
>> >> + (define page-header "Comparing")
>> >> +
>> >> (layout
>> >> + #:title
>> >> + (string-append page-header " " (string-take base-commit 8) " and "
>> >> + (string-take target-commit 8))
>> >> #:body
>> >> `(,(header)
>> >> (div
>> >> @@ -107,7 +112,7 @@
>> >> (@ (class "col-sm-7"))
>> >> ,@(if invalid-query?
>> >> `((h1 "Compare"))
>> >> - `((h1 "Comparing "
>> >> + `((h1 ,page-header ," "
>> >> (a (@ (href ,(string-append "/revision/"
>> base-commit)))
>> >> (samp ,(string-take base-commit 8) "…"))
>> >> " and "
>> >>
>> >> There's a couple of things here. I'd be tempted not to use a variable
>> >> for "Comparing", it's not really the page header, as that's more
>> >> complicated, so I think I'd just use the string in both places.
>> >>
>> >> Second thing, the (if invalid-query? bit when constructing the h1
>> >> element is important. The query parameters being invalid could mean
>> >> anything from the form just hasn't been filled in, to the value isn't
>> >> actually a commit hash, but something else, maybe some HTML/JavaScript
>> >> that is malicious and shouldn't be included in the page. A similar
>> >> approach probably needs taking for the title.
>>
>> This stuff above still looks the same to me, although part of my
>> analysis was wrong, as the type of an invalid parameter is a record, so
>> the page just breaks if the parameters are invalid (which I guses is
>> better than what I was describing).
>>
>> Anyway, I think this still needs fixing.
These are my relevant comments from before, you should be able to see
the error yourself if you go to /compare locally, does this page work
for you?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 987 bytes --]
next prev parent reply other threads:[~2021-04-22 20:09 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-13 9:01 [Outreachy] - Guix Data Service - Set a more informative page title Canan Talayhan
2021-04-13 11:57 ` Maxime Devos
2021-04-13 15:56 ` Canan Talayhan
2021-04-13 17:51 ` Maxime Devos
2021-04-15 12:08 ` Canan Talayhan
2021-04-15 21:52 ` Christopher Baines
2021-04-16 9:58 ` Canan Talayhan
2021-04-16 11:11 ` Christopher Baines
2021-04-18 13:42 ` Canan Talayhan
2021-04-18 17:53 ` Christopher Baines
2021-04-18 20:37 ` Canan Talayhan
2021-04-19 19:16 ` Christopher Baines
2021-04-21 15:43 ` Canan Talayhan
2021-04-22 19:46 ` Christopher Baines [this message]
2021-04-23 8:34 ` Canan Talayhan
2021-04-23 12:10 ` Christopher Baines
2021-04-24 11:39 ` Christopher Baines
2021-04-24 15:30 ` Canan Talayhan
2021-04-24 20:21 ` Christopher Baines
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=878s5ahzke.fsf@cbaines.net \
--to=mail@cbaines.net \
--cc=canan.t.talayhan@gmail.com \
--cc=guix-devel@gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/guix.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.