Launchpad itself

Merge lp:~deryck/launchpad/comment-add-ui-updates-413611 into lp:launchpad

  1. comment-add-ui-updates-413611
  2. Merge into devel
Proposed by Deryck Hodge
Status: Merged
Merged at revision: not available
Proposed branch: lp:~deryck/launchpad/comment-add-ui-updates-413611
Merge into: lp:launchpad
Diff against target: None lines
To merge this branch: bzr merge lp:~deryck/launchpad/comment-add-ui-updates-413611
Reviewer Review Type Date Requested Status
Eleanor Berger (community) ui Approve
Gavin Panella (community) Approve
Review via email: mp+10383@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Deryck Hodge (deryck) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

These are mechanical changes to get the add a comment or attachment page
updated to the 3.0 UI. See bug 413611 for lots of discussion as I
learned my way around what look these changes should ultimately produce.

There are several tests which can verify these changes are clean in
lib/lp/bugs/stories/bugattachments.

./bin/test -vv -t stories/bugattachments

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/bugs/browser/bugmessage.py
  lib/canonical/launchpad/pagetitles.py
  lib/lp/bugs/templates/bug-comment-add.pt
  lib/lp/bugs/browser/configure.zcml

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkqL+zwACgkQ4glRK0DaE8gQ0QCePOaclGw9wBWuEW2bZVdU17FP
jFEAoN04Kn+0/JlZ3BVoX7fr9fYmOwoH
=4N/z
-----END PGP SIGNATURE-----

Revision history for this message
Gavin Panella (allenap) wrote :
Download full text (3.8 KiB)

Hi Deryck,

Looks good. The layout of the page:

  <h2>Bug #11 in Jokosher: "Make Jokosher use autoaudiosink"</h2>
  <h1>Add a comment or attachment to Bug #11</h1>

is better than, IMO, the layout I did for the bug activity log:

  <h1>Activity log for bug #11: Make Jokosher use autoaudiosink</h1>

That would have been better as:

  <h2>Bug #11 in Jokosher: "Make Jokosher use autoaudiosink"</h2>
  <h1>Activity log for bug #11</h1>

So that there's a consistent look, could you could patch the bug
activity stuff to look the same as here and land it in this branch?
There's a patch at <http://pastebin.ubuntu.com/255760/>.

Other than that, only one tiny comment :)

Gavin.

> === modified file 'lib/canonical/launchpad/pagetitles.py'
> --- lib/canonical/launchpad/pagetitles.py 2009-08-19 02:58:06 +0000
> +++ lib/canonical/launchpad/pagetitles.py 2009-08-19 13:49:59 +0000
> @@ -230,8 +230,6 @@
>
> bug_branch_add = LaunchbagBugID('Bug #%d - Add branch')
>
> -bug_comment_add = LaunchbagBugID('Bug #%d - Add a comment or attachment')
> -
> bug_cve = LaunchbagBugID("Bug #%d - Add CVE reference")
>
> bug_edit = ContextBugId('Bug #%d - Edit')
>
> === modified file 'lib/lp/bugs/browser/bugmessage.py'
> --- lib/lp/bugs/browser/bugmessage.py 2009-08-05 15:05:52 +0000
> +++ lib/lp/bugs/browser/bugmessage.py 2009-08-19 13:49:59 +0000
> @@ -25,6 +25,14 @@
> initial_focus_widget = None
>
> @property
> + def label(self):
> + return 'Add a comment or attachment to Bug #%d' % self.context.bug.id

I think: s/Bug/bug/

> +
> + @property
> + def page_title(self):
> + return self.label
> +
> + @property
> def initial_values(self):
> return dict(subject=self.context.bug.followup_subject())
>
>
> === modified file 'lib/lp/bugs/browser/configure.zcml'
> --- lib/lp/bugs/browser/configure.zcml 2009-07-27 12:39:34 +0000
> +++ lib/lp/bugs/browser/configure.zcml 2009-08-19 13:49:59 +0000
> @@ -524,7 +524,7 @@
> <browser:page
> name="+addcomment"
> for="lp.bugs.interfaces.bugtask.IBugTask"
> - class="lp.bugs.browser.bug.BugView"
> + class="lp.bugs.browser.bugmessage.BugMessageAddFormView"
> permission="launchpad.Edit"
> template="../templates/bug-comment-add.pt"/>
> <browser:page
>
> === modified file 'lib/lp/bugs/templates/bug-comment-add.pt'
> --- lib/lp/bugs/templates/bug-comment-add.pt 2009-07-17 17:59:07 +0000
> +++ lib/lp/bugs/templates/bug-comment-add.pt 2009-08-19 13:49:59 +0000
> @@ -3,33 +3,12 @@
> xmlns:tal="http://xml.zope.org/namespaces/tal"
> xmlns:metal="http://xml.zope.org/namespaces/metal"
> xmlns:i18n="http://xml.zope.org/namespaces/i18n"
> - xml:lang="en"
> - lang="en"
> - dir="ltr"
> - metal:use-macro="context/@@main_template/master"
> - i18n:domain="malone"
> ->
> -
> -<metal:rightportlets fill-slot="portlets_two">
> - <div tal:replace="structure context/bug/@@+portlet-attachments" />
> - <div tal:replace="structure context/@@+portlet-search" />
> -</metal:rightportlets>
> -
> -<body>
> - <metal:heading fill-slot="pageheading"
> - tal:define="bugtask context">
> - <div>
> - ...

Read more...

review: Approve
Revision history for this message
Deryck Hodge (deryck) wrote :

Hi, Gavin.

Thanks for the review!

On Wed, Aug 19, 2009 at 9:17 AM, Gavin
Panella<email address hidden> wrote:
> Looks good. The layout of the page:
>
>  <h2>Bug #11 in Jokosher: "Make Jokosher use autoaudiosink"</h2>
>  <h1>Add a comment or attachment to Bug #11</h1>
>
> is better than, IMO, the layout I did for the bug activity log:

I've updated my branch with your patch to match. Glad you mentioned
that. As I understand it from Curtis, this is the form we should use
for most all the pages, only overriding the heading slot for an
object's index page.

> Other than that, only one tiny comment :)

The "Bug #XXX" capitalization has been updated as well. :)

Cheers,
deryck

Revision history for this message
Eleanor Berger (intellectronica) wrote :

The result doesn't look particularly beautiful. The title repeats the same information as the line above it, and the attachment box is much wider than the comment box. Still, since this is just the fallback option and very few users will actually see it, let's go with that.

review: Approve (ui)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/pagetitles.py'
2--- lib/canonical/launchpad/pagetitles.py 2009-08-19 02:58:06 +0000
3+++ lib/canonical/launchpad/pagetitles.py 2009-08-19 12:24:35 +0000
4@@ -230,8 +230,6 @@
5
6 bug_branch_add = LaunchbagBugID('Bug #%d - Add branch')
7
8-bug_comment_add = LaunchbagBugID('Bug #%d - Add a comment or attachment')
9-
10 bug_cve = LaunchbagBugID("Bug #%d - Add CVE reference")
11
12 bug_edit = ContextBugId('Bug #%d - Edit')
13
14=== modified file 'lib/lp/bugs/browser/bugmessage.py'
15--- lib/lp/bugs/browser/bugmessage.py 2009-08-05 15:05:52 +0000
16+++ lib/lp/bugs/browser/bugmessage.py 2009-08-19 12:23:51 +0000
17@@ -25,6 +25,14 @@
18 initial_focus_widget = None
19
20 @property
21+ def label(self):
22+ return 'Add a comment or attachment to Bug #%d' % self.context.bug.id
23+
24+ @property
25+ def page_title(self):
26+ return self.label
27+
28+ @property
29 def initial_values(self):
30 return dict(subject=self.context.bug.followup_subject())
31
32
33=== modified file 'lib/lp/bugs/browser/configure.zcml'
34--- lib/lp/bugs/browser/configure.zcml 2009-07-27 12:39:34 +0000
35+++ lib/lp/bugs/browser/configure.zcml 2009-08-19 12:23:51 +0000
36@@ -524,7 +524,7 @@
37 <browser:page
38 name="+addcomment"
39 for="lp.bugs.interfaces.bugtask.IBugTask"
40- class="lp.bugs.browser.bug.BugView"
41+ class="lp.bugs.browser.bugmessage.BugMessageAddFormView"
42 permission="launchpad.Edit"
43 template="../templates/bug-comment-add.pt"/>
44 <browser:page
45
46=== modified file 'lib/lp/bugs/templates/bug-comment-add.pt'
47--- lib/lp/bugs/templates/bug-comment-add.pt 2009-07-17 17:59:07 +0000
48+++ lib/lp/bugs/templates/bug-comment-add.pt 2009-08-14 16:35:46 +0000
49@@ -3,33 +3,12 @@
50 xmlns:tal="http://xml.zope.org/namespaces/tal"
51 xmlns:metal="http://xml.zope.org/namespaces/metal"
52 xmlns:i18n="http://xml.zope.org/namespaces/i18n"
53- xml:lang="en"
54- lang="en"
55- dir="ltr"
56- metal:use-macro="context/@@main_template/master"
57- i18n:domain="malone"
58->
59-
60-<metal:rightportlets fill-slot="portlets_two">
61- <div tal:replace="structure context/bug/@@+portlet-attachments" />
62- <div tal:replace="structure context/@@+portlet-search" />
63-</metal:rightportlets>
64-
65-<body>
66- <metal:heading fill-slot="pageheading"
67- tal:define="bugtask context">
68- <div>
69- <img alt="" src="/@@/bug" />
70- <a tal:attributes="href bugtask/fmt:url"
71- >Bug #<tal:bug replace="context/bug/id">3252543</tal:bug></a>:
72- <tal:title replace="context/bug/title">Foo doesn't work</tal:title>
73+ metal:use-macro="view/macro:page/main_only"
74+ i18n:domain="malone">
75+ <body>
76+ <div metal:fill-slot="main">
77+ <div tal:content="structure context/@@+addcomment-form" />
78 </div>
79- <h1>Add a comment or attachment</h1>
80- </metal:heading>
81- <div metal:fill-slot="main">
82- <div tal:content="structure context/@@+addcomment-form" />
83- </div>
84-</body>
85-
86+ </body>
87 </html>
88