[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Outreachy] - Guix Data Service - Set a more informative page title
From: |
Canan Talayhan |
Subject: |
Re: [Outreachy] - Guix Data Service - Set a more informative page title |
Date: |
Fri, 23 Apr 2021 11:34:10 +0300 |
It seems after testing lots of pages this one escaped me since I only
tested the working case.
Please find the quick fix in the link below.
https://pastebin.ubuntu.com/p/s7tWyPHZ8F/
I'm looking forward to making another contribution. Could you please
review it as soon as possible?
Thanks,
Canan Talayhan
On Thu, Apr 22, 2021 at 10:47 PM Christopher Baines <mail@cbaines.net> wrote:
>
>
> 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?
0001-Set-a-more-informative-page-titles.patch
Description: Text Data
- Re: [Outreachy] - Guix Data Service - Set a more informative page title, (continued)
- Re: [Outreachy] - Guix Data Service - Set a more informative page title, Canan Talayhan, 2021/04/15
- Re: [Outreachy] - Guix Data Service - Set a more informative page title, Christopher Baines, 2021/04/15
- Re: [Outreachy] - Guix Data Service - Set a more informative page title, Canan Talayhan, 2021/04/16
- Re: [Outreachy] - Guix Data Service - Set a more informative page title, Christopher Baines, 2021/04/16
- Re: [Outreachy] - Guix Data Service - Set a more informative page title, Canan Talayhan, 2021/04/18
- Re: [Outreachy] - Guix Data Service - Set a more informative page title, Christopher Baines, 2021/04/18
- Re: [Outreachy] - Guix Data Service - Set a more informative page title, Canan Talayhan, 2021/04/18
- Re: [Outreachy] - Guix Data Service - Set a more informative page title, Christopher Baines, 2021/04/19
- Re: [Outreachy] - Guix Data Service - Set a more informative page title, Canan Talayhan, 2021/04/21
- Re: [Outreachy] - Guix Data Service - Set a more informative page title, Christopher Baines, 2021/04/22
- Re: [Outreachy] - Guix Data Service - Set a more informative page title,
Canan Talayhan <=
- Re: [Outreachy] - Guix Data Service - Set a more informative page title, Christopher Baines, 2021/04/23
- Re: [Outreachy] - Guix Data Service - Set a more informative page title, Christopher Baines, 2021/04/24
- Re: [Outreachy] - Guix Data Service - Set a more informative page title, Canan Talayhan, 2021/04/24
- Re: [Outreachy] - Guix Data Service - Set a more informative page title, Christopher Baines, 2021/04/24