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: Sun, 18 Apr 2021 18:53:23 +0100 [thread overview]
Message-ID: <87wnsziing.fsf@cbaines.net> (raw)
In-Reply-To: <CAAosC5+KJDc4KEoVHLSUsVpmk=20nbsg0av=chC5m2mEhJ=yOA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 9439 bytes --]
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.
@@ -419,14 +424,18 @@
'(span (@ (class "text-success glyphicon glyphicon-plus pull-left")
(style "font-size: 1.5em; padding-right: 0.4em;"))))
+ (define page-header "Comparing derivations")
+
(layout
+ #:title
+ page-header
#:body
`(,(header)
(div
(@ (class "container"))
(div
(@ (class "row"))
- (h1 ,@(let ((base-commit (assq-ref query-parameters 'base_commit))
+ (h1 ,(let ((base-commit (assq-ref query-parameters 'base_commit))
Why's the @ being removed here?
@@ -435,7 +444,7 @@
" and "
(a (@ (href ,(string-append "/revision/" target-commit)))
(samp ,(string-take target-commit 8) "…")))
- '("Comparing derivations")))))
+ `,page-header))))
The quote then immediate unquote here isn't necessary, also, I think
this should stick to being a list containing a string, as the other part
of the if returns a list.
diff --git a/guix-data-service/web/dumps/html.scm b/guix-data-service/web/dumps/html.scm
index 71e69c8..9645f7c 100644
--- a/guix-data-service/web/dumps/html.scm
+++ b/guix-data-service/web/dumps/html.scm
@@ -21,8 +21,13 @@
#:use-module (guix-data-service web view html)
#:export (view-dumps))
+(define page-header "Database dumps")
+
(define (view-dumps available-dumps)
+
(layout
+ #:title
+ page-header
#:body
`(,(header)
(div
@@ -31,7 +36,7 @@
(@ (class "row"))
(div
(@ (class "col-sm-12"))
- (h1 "Database dumps")))
+ (h1 ,page-header)))
Like the others, I'd probably put page-header inside the view-dumps
procedure. Same goes for other places where it's outside.
@@ -267,7 +279,7 @@
(@ (class "col-sm-12"))
(a (@ (href "/jobs"))
(h3 "Jobs"))
- (h1 "Queued jobs ("
+ (h1 ,page-header"("
,(length jobs-and-events)
")")))
(div
I'd suspect the title here would be "Queued jobs(", I'd also put a space
between ,page-header the bit after it in the code.
@@ -329,8 +341,13 @@
'())))))
jobs-and-events)))))))))
+
(define (view-job job-id query-parameters log)
+ (define page-header (string-append "Job " job-id))
+
(layout
+ #:title
+ page-header
#:body
`(,(header)
(div
Most of the procedures are separated by one line, and I wouldn't change
that here.
@@ -24,7 +24,11 @@
#:export (view-package))
(define* (view-package name package-version-with-branches)
+ (define page-header "Package")
+
(layout
+ #:title
+ (string-append page-header " " name)
#:body
`(,(header)
(div
@@ -33,7 +37,7 @@
(@ (class "row"))
(div
(@ (class "col-md-12"))
- (h1 "Package: " ,name)))
+ (h1 ,page-header ," " ,name)))
,@(map
(match-lambda
((('version . version)
I'm not that fussed about the colon, but I'd probably keep it.
I'd try to keep the page-header variable meaningful if you're going to
use it though. "Package" is named as the page-header, but it's not what
the title is, or the h1 element. They're both better as they include the
package name, I'd probably just make the page-header the actual string
used in both places.
@@ -65,7 +69,11 @@
(define* (view-git-repository git-repository-id
label url cgit-url-base
branches-with-most-recent-commits)
+ (define page-header (string-append "Repository " (string-drop url 8)))
+
(layout
+ #:title
+ page-header
#:body
`(,(header)
(div
This is really nice, it's good that the pages for different repositories
won't have the same title.
@@ -197,7 +209,11 @@
branch-name
package-name
versions-by-revision-range)
+ (define page-header (string-append branch-name " " package-name))
+
(layout
+ #:title
+ page-header
#:body
`(,(header)
(div
I think you might need some more words for this title to make sense
though, even just having package-name " on " branch-name would probably
help people work out what's going on.
@@ -386,6 +402,8 @@
(map first derivations-by-revision-range))))
(layout
+ #:title
+ (string-append package-name " Package derivations")
#:body
`(,(header)
(div
@@ -636,6 +654,8 @@
(map first outputs-by-revision-range))))
(layout
+ #:title
+ (string-append package-name " Package outputs")
#:body
`(,(header)
(div
@@ -849,6 +869,8 @@
valid-systems
system-test-history)
(layout
+ #:title
+ (string-append system-test-name " History")
#:body
`(,(header)
(div
Given there's something coming first in these titles, I wouldn't
capitalise the H in History or P in Package, since it's not at the start
of the title.
@@ -48,7 +48,12 @@
(define* (view-revision-news commit-hash
query-parameters
news-entries)
+ (define page-header "Revision")
+
(layout
+ #:title
+ (string-append "Channel News Entries " page-header " "
+ (string-take commit-hash 7))
#:body
`(,(header)
(div
@@ -48,7 +48,12 @@
(define* (view-revision-news commit-hash
query-parameters
news-entries)
+ (define page-header "Revision")
+
(layout
+ #:title
+ (string-append "Channel News Entries " page-header " "
+ (string-take commit-hash 7))
#:body
`(,(header)
(div
Same thing here regarding defining a page-header which isn't actually
used as the header.
I do think it's useful to include both the Channel News Entries bit and
the revision bit in the title, but I'd probably put a separator (like a
-) in between them. Same goes for the other changes in this file.
@@ -2314,7 +2386,11 @@ figure {
(define (unknown-revision commit-hash job git-repositories-and-branches
jobs-and-events)
+ (define page-header "Uknown revision")
+
(layout
+ #:title
+ page-header
#:body
`(,(header)
(div
A letter was lost here.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 987 bytes --]
next prev parent reply other threads:[~2021-04-18 17:53 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 [this message]
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
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=87wnsziing.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.