Merge into devel : polls-three-o : Code : Launchpad itself

Status:

Superseded

Proposed branch:

lp:~salgado/launchpad/polls-three-o

Merge into:

lp:launchpad

Diff against target:

None lines

To merge this branch:

bzr merge lp:~salgado/launchpad/polls-three-o

Related bugs:

Bug #416460: Convert poll pages to 3.0

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Jeroen T. Vermeulen (community)		2009-08-18	Needs Fixing on 2009-08-19
Review via email: mp+10343@code.launchpad.net

This proposal has been superseded by a proposal from 2009-08-19.

Revision history for this message

Guilherme Salgado (salgado) wrote on 2009-08-18:

#

= Summary =

Convert a few poll pages to 3.0

== Proposed fix ==

Convert the easy edit pages and turn the action menu into a related
pages section at the bottom.

before/after screenshots at
https://devpad.canonical.com/~salgado/poll-shots/

== Pre-implementation notes ==

== Implementation details ==

== Tests ==

./bin/test -vvt stories.team-polls

== Demo and Q/A ==

= 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/registry/browser/configure.zcml
  lib/lp/registry/templates/poll-newoption.pt
  lib/lp/registry/stories/team-polls/edit-options.txt
  lib/lp/registry/templates/team-polls.pt
  lib/lp/registry/templates/polloption-edit.pt
  lib/lp/registry/browser/poll.py
  lib/lp/registry/templates/poll-edit.pt
  lib/lp/registry/stories/team-polls/create-poll-options.txt

Revision history for this message

Jeroen T. Vermeulen (jtv) wrote on 2009-08-19:

#

Hi Salgado,

I stubbed my toe a few times looking at the UI changes, until I realized
that I'm really missing a breadcrumb for the poll. Once you've drilled
down into a poll, everything you do in there refers back to the team.
On the edit-option page I found no reference to the poll whatsoever:
it's like you're editing a direct property of the team itself. Having
the poll shown as a first-class entity would make a big difference, I
think. But that's not for a template conversion to fix.

There were a few small inconsistencies, or at least things that I think
are inconsistent: adding an option gives me "Show options" and "Change
details" as related pages, plus a Save button, but editing one gives me
Save plus the Cancel link. Is there a reason why these are so
different?

Something that may force the issue is that the "Change details" link on
the new-option page is for the poll. Had I not known where this UI
element came from, I might have expected it to apply to the option I'm
adding. So I think it'd be more intuitive to remove these "related
pages" links from the new-option page and add a Cancel link back to the
poll instead.

Similarly, unless Martin has a rule against it, I think the poll edit
form should have a Cancel link. Right now there's no direct way back to
the poll page.

(The same goes for the options overview by the way, which is the only
place where I would really expect to be able to add or edit options. It
looks to me as though the options page wants to be integrated into the
poll page, with an "Add new option" link inline and without any remnants
of this old actions menu anywhere. Maybe that's what you have planned
anyway.)

Another very minor annoyance is that some of the page headings look a
little redundant. Sometimes they include the poll name, sometimes they
don't. Where they do, they duplicate the poll title displayed in small
font right above the heading. But I don't see any way of fixing that
without either removing that small title (which seems to be baked into
the application menu above it) or removing the poll name from the page
title--both of which sound like bad alternatives.

Speaking of headings: your branch sets the label for the poll edit form
to "Edit poll details." What exactly that text should read is an
ongoing discussion (started by Curtis on the mailing list), but more to
the point, I don't see it show up anywhere! If you provide a page_title
in this view it'll get picked up (and the one in pagetitles.py will no
longer be needed), but label doesn't seem to be.

Apart from that, the code as such looks good to me. Nice use of a mixin
for the navigation menu, though in this case I'd leave it out altogether
instead of migrating it to the 3.0 setup.

Jeroen

Hi Salgado,

I stubbed my toe a few times looking at the UI changes, until I realized
that I'm really missing a breadcrumb for the poll.  Once you've drilled
down into a poll, everything you do in there refers back to the team.
On the edit-option page I found no reference to the poll whatsoever:
it's like you're editing a direct property of the team itself.  Having
the poll shown as a first-class entity would make a big difference, I
think.  But that's not for a template conversion to fix.

There were a few small inconsistencies, or at least things that I think
are inconsistent: adding an option gives me "Show options" and "Change
details" as related pages, plus a Save button, but editing one gives me
Save plus the Cancel link.  Is there a reason why these are so
different?

Something that may force the issue is that the "Change details" link on
the new-option page is for the poll.  Had I not known where this UI
element came from, I might have expected it to apply to the option I'm
adding.  So I think it'd be more intuitive to remove these "related
pages" links from the new-option page and add a Cancel link back to the
poll instead.

Similarly, unless Martin has a rule against it, I think the poll edit
form should have a Cancel link.  Right now there's no direct way back to
the poll page.

(The same goes for the options overview by the way, which is the only
place where I would really expect to be able to add or edit options.  It
looks to me as though the options page wants to be integrated into the
poll page, with an "Add new option" link inline and without any remnants
of this old actions menu anywhere.  Maybe that's what you have planned
anyway.)

Another very minor annoyance is that some of the page headings look a
little redundant.  Sometimes they include the poll name, sometimes they
don't.  Where they do, they duplicate the poll title displayed in small
font right above the heading.  But I don't see any way of fixing that
without either removing that small title (which seems to be baked into
the application menu above it) or removing the poll name from the page
title--both of which sound like bad alternatives.

Speaking of headings: your branch sets the label for the poll edit form
to "Edit poll details."  What exactly that text should read is an
ongoing discussion (started by Curtis on the mailing list), but more to
the point, I don't see it show up anywhere!  If you provide a page_title
in this view it'll get picked up (and the one in pagetitles.py will no
longer be needed), but label doesn't seem to be.

Apart from that, the code as such looks good to me.  Nice use of a mixin
for the navigation menu, though in this case I'd leave it out altogether
instead of migrating it to the 3.0 setup.

Jeroen

review: Needs Fixing

Revision history for this message

Guilherme Salgado (salgado) wrote on 2009-08-19:

#

Download full text (4.2 KiB)

On Wed, 2009-08-19 at 08:13 +0000, Jeroen T. Vermeulen wrote:
> Review: Needs Fixing
> Hi Salgado,
>
> I stubbed my toe a few times looking at the UI changes, until I realized
> that I'm really missing a breadcrumb for the poll. Once you've drilled
> down into a poll, everything you do in there refers back to the team.
> On the edit-option page I found no reference to the poll whatsoever:
> it's like you're editing a direct property of the team itself. Having
> the poll shown as a first-class entity would make a big difference, I
> think. But that's not for a template conversion to fix.

I think everybody who's used polls had a similar experience to yours.
They were written a long time ago and were left mostly untouched since
then, even when we redesigned navigation/UI for 1.0 and 2.0

>
> There were a few small inconsistencies, or at least things that I think
> are inconsistent: adding an option gives me "Show options" and "Change
> details" as related pages, plus a Save button, but editing one gives me
> Save plus the Cancel link. Is there a reason why these are so
> different?

That's because URL of these related pages are relative, so they work on
IPoll pages (like +newoption) but not on IPollOption ones (like
+option/id). The Cancel link was added there only to make it possible
to return to the +options page, but all forms should have a Cancel link.

>
> Something that may force the issue is that the "Change details" link on
> the new-option page is for the poll. Had I not known where this UI
> element came from, I might have expected it to apply to the option I'm
> adding. So I think it'd be more intuitive to remove these "related
> pages" links from the new-option page and add a Cancel link back to the
> poll instead.

I think that's a good thing to do.

>
> Similarly, unless Martin has a rule against it, I think the poll edit
> form should have a Cancel link. Right now there's no direct way back to
> the poll page.

They all should have a Cancel button.

>
> (The same goes for the options overview by the way, which is the only
> place where I would really expect to be able to add or edit options. It
> looks to me as though the options page wants to be integrated into the
> poll page, with an "Add new option" link inline and without any remnants
> of this old actions menu anywhere. Maybe that's what you have planned
> anyway.)

In fact, my only plan is to migrate these pages without making things
any worse than they already are and spend the least possible amount of
time doing so.

But I really liked your suggestion, so I filed
https://bugs.edge.launchpad.net/bugs/415896

>
> Another very minor annoyance is that some of the page headings look a
> little redundant. Sometimes they include the poll name, sometimes they
> don't. Where they do, they duplicate the poll title displayed in small
> font right above the heading. But I don't see any way of fixing that
> without either removing that small title (which seems to be baked into
> the application menu above it) or removing the poll name from the page
> title--both of which sound like bad alternatives.
>
> Speaking of headings: your branch sets the label for the pol...

On Wed, 2009-08-19 at 08:13 +0000, Jeroen T. Vermeulen wrote:
> Review: Needs Fixing
> Hi Salgado,
> 
> I stubbed my toe a few times looking at the UI changes, until I realized
> that I'm really missing a breadcrumb for the poll.  Once you've drilled
> down into a poll, everything you do in there refers back to the team.
> On the edit-option page I found no reference to the poll whatsoever:
> it's like you're editing a direct property of the team itself.  Having
> the poll shown as a first-class entity would make a big difference, I
> think.  But that's not for a template conversion to fix.

I think everybody who's used polls had a similar experience to yours.
They were written a long time ago and were left mostly untouched since
then, even when we redesigned navigation/UI for 1.0 and 2.0

> 
> There were a few small inconsistencies, or at least things that I think
> are inconsistent: adding an option gives me "Show options" and "Change
> details" as related pages, plus a Save button, but editing one gives me
> Save plus the Cancel link.  Is there a reason why these are so
> different?

That's because URL of these related pages are relative, so they work on
IPoll pages (like +newoption) but not on IPollOption ones (like
+option/id).  The Cancel link was added there only to make it possible
to return to the +options page, but all forms should have a Cancel link.

> 
> Something that may force the issue is that the "Change details" link on
> the new-option page is for the poll.  Had I not known where this UI
> element came from, I might have expected it to apply to the option I'm
> adding.  So I think it'd be more intuitive to remove these "related
> pages" links from the new-option page and add a Cancel link back to the
> poll instead.

I think that's a good thing to do.

> 
> Similarly, unless Martin has a rule against it, I think the poll edit
> form should have a Cancel link.  Right now there's no direct way back to
> the poll page.

They all should have a Cancel button.

> 
> (The same goes for the options overview by the way, which is the only
> place where I would really expect to be able to add or edit options.  It
> looks to me as though the options page wants to be integrated into the
> poll page, with an "Add new option" link inline and without any remnants
> of this old actions menu anywhere.  Maybe that's what you have planned
> anyway.)

In fact, my only plan is to migrate these pages without making things
any worse than they already are and spend the least possible amount of
time doing so.

But I really liked your suggestion, so I filed
https://bugs.edge.launchpad.net/bugs/415896

> 
> Another very minor annoyance is that some of the page headings look a
> little redundant.  Sometimes they include the poll name, sometimes they
> don't.  Where they do, they duplicate the poll title displayed in small
> font right above the heading.  But I don't see any way of fixing that
> without either removing that small title (which seems to be baked into
> the application menu above it) or removing the poll name from the page
> title--both of which sound like bad alternatives.
> 
> Speaking of headings: your branch sets the label for the poll edit form
> to "Edit poll details."  What exactly that text should read is an
> ongoing discussion (started by Curtis on the mailing list), but more to
> the point, I don't see it show up anywhere!  If you provide a page_title
> in this view it'll get picked up (and the one in pagetitles.py will no
> longer be needed), but label doesn't seem to be.

The .label should be picked up by launchpad-form.pt, which is used in
these pages.  I can't seem to find what template picks up the page title
and includes it as an <h1>, though.  This must be something specific to
the locationless macro as I don't see this behaviour on other
LaunchpadFormViews.

Maybe Curtis can enlighten us.

> 
> Apart from that, the code as such looks good to me.  Nice use of a mixin
> for the navigation menu, though in this case I'd leave it out altogether
> instead of migrating it to the 3.0 setup.
>

Agreed, but I don't really have the time to combine the pages and inline
the links in them, so the nav menus will have to do for now. :/

Thanks a lot for the review and the suggestions.

-- 
Guilherme Salgado <salgado@canonical.com>

Launchpad itself

Merge lp:~salgado/launchpad/polls-three-o into lp:launchpad

Commit message

Description of the change

Preview Diff

Subscribers

1	=== modified file 'lib/lp/registry/browser/configure.zcml'
2	--- lib/lp/registry/browser/configure.zcml 2009-08-18 04:57:21 +0000
3	+++ lib/lp/registry/browser/configure.zcml 2009-08-18 20:24:56 +0000
4	@@ -600,7 +600,8 @@
5	<browser:menus
6	module="lp.registry.browser.poll"
7	classes="
8	- PollContextMenu"/>
9	+ PollContextMenu
10	+ PollEditNavigationMenu"/>
11	<browser:defaultView
12	for="lp.registry.interfaces.poll.IPoll"
13	name="+index"/>
14
15	=== modified file 'lib/lp/registry/browser/poll.py'
16	--- lib/lp/registry/browser/poll.py 2009-06-25 04:06:00 +0000
17	+++ lib/lp/registry/browser/poll.py 2009-08-18 20:24:56 +0000
18	@@ -7,6 +7,7 @@
19	'BasePollView',
20	'PollAddView',
21	'PollContextMenu',
22	+ 'PollEditNavigationMenu',
23	'PollEditView',
24	'PollNavigation',
25	'PollOptionAddView',
26	@@ -17,13 +18,14 @@
27
28	from zope.event import notify
29	from zope.component import getUtility
30	+from zope.interface import implements, Interface
31	from zope.lifecycleevent import ObjectCreatedEvent
32	from zope.app.form.browser import TextWidget
33
34	from canonical.launchpad.webapp import (
35	action, canonical_url, ContextMenu, custom_widget,
36	enabled_with_permission, LaunchpadEditFormView, LaunchpadFormView, Link,
37	- Navigation, stepthrough)
38	+ Navigation, NavigationMenu, stepthrough)
39	from canonical.launchpad.webapp.interfaces import ILaunchBag
40	from lp.registry.interfaces.poll import (
41	IPoll, IPollOption, IPollOptionSet, IPollSubset, IVoteSet, PollAlgorithm,
42	@@ -31,13 +33,10 @@
43	from canonical.launchpad.helpers import shortlist
44
45
46	-class PollContextMenu(ContextMenu):
47	-
48	- usedfor = IPoll
49	- links = ['showall', 'addnew', 'edit']
50	+class PollEditLinksMixin:
51
52	def showall(self):
53	- text = 'Show option details'
54	+ text = 'Show options'
55	return Link('+options', text, icon='info')
56
57	@enabled_with_permission('launchpad.Edit')
58	@@ -51,6 +50,21 @@
59	return Link('+edit', text, icon='edit')
60
61
62	+class PollContextMenu(ContextMenu, PollEditLinksMixin):
63	+ usedfor = IPoll
64	+ links = ['showall', 'addnew', 'edit']
65	+
66	+
67	+class IPollEditMenu(Interface):
68	+ """A marker interface for the 'Change details' navigation menu."""
69	+
70	+
71	+class PollEditNavigationMenu(NavigationMenu, PollEditLinksMixin):
72	+ usedfor = IPollEditMenu
73	+ facet = 'overview'
74	+ links = ['showall', 'addnew', 'edit']
75	+
76	+
77	class PollNavigation(Navigation):
78
79	usedfor = IPoll
80	@@ -353,6 +367,7 @@
81
82	class PollEditView(LaunchpadEditFormView):
83
84	+ implements(IPollEditMenu)
85	schema = IPoll
86	label = "Edit poll details"
87	field_names = ["name", "title", "proposition", "allowspoilt", "dateopens",
88	@@ -372,6 +387,11 @@
89	field_names = ["name", "title"]
90	custom_widget("title", TextWidget, width=30)
91
92	+ @property
93	+ def cancel_url(self):
94	+ """See `LaunchpadFormView`."""
95	+ return canonical_url(self.context.poll, view_name='+options')
96	+
97	@action("Save", name="save")
98	def save_action(self, action, data):
99	self.updateContextFromData(data)
100	@@ -381,6 +401,7 @@
101	class PollOptionAddView(LaunchpadFormView):
102	"""Create a new option in a given poll."""
103
104	+ implements(IPollEditMenu)
105	schema = IPollOption
106	label = "Create new poll option"
107	field_names = ["name", "title"]
108
109	=== modified file 'lib/lp/registry/stories/team-polls/create-poll-options.txt'
110	--- lib/lp/registry/stories/team-polls/create-poll-options.txt 2009-04-17 10:32:16 +0000
111	+++ lib/lp/registry/stories/team-polls/create-poll-options.txt 2009-08-18 20:24:56 +0000
112	@@ -37,7 +37,7 @@
113	And here we see the option listed as one of the active options for this
114	poll.
115
116	- >>> team_admin_browser.getLink('Show option details').click()
117	+ >>> team_admin_browser.getLink('Show options').click()
118	>>> team_admin_browser.url
119	'http://launchpad.dev/~ubuntu-team/+poll/dpl-2080/+options'
120
121
122	=== modified file 'lib/lp/registry/stories/team-polls/edit-options.txt'
123	--- lib/lp/registry/stories/team-polls/edit-options.txt 2008-10-15 17:56:31 +0000
124	+++ lib/lp/registry/stories/team-polls/edit-options.txt 2009-08-18 20:24:56 +0000
125	@@ -7,7 +7,7 @@
126	>>> user_browser.getLink('A public poll that never closes').click()
127	>>> user_browser.url
128	'http://launchpad.dev/~ubuntu-team/+poll/never-closes4'
129	- >>> user_browser.getLink('Show option details').click()
130	+ >>> user_browser.getLink('Show options').click()
131	>>> user_browser.url
132	'http://launchpad.dev/~ubuntu-team/+poll/never-closes4/+options'
133	>>> user_browser.getLink('Edit')
134	@@ -21,7 +21,7 @@
135	>>> browser = setupBrowser(
136	... auth='Basic jeff.waugh@ubuntulinux.com:jdub')
137	>>> browser.open('http://launchpad.dev/~ubuntu-team/+poll/never-closes4')
138	- >>> browser.getLink('Show option details').click()
139	+ >>> browser.getLink('Show options').click()
140	>>> browser.url
141	'http://launchpad.dev/~ubuntu-team/+poll/never-closes4/+options'
142	>>> browser.getLink('[Edit]').click()
143	@@ -43,7 +43,7 @@
144	>>> browser.url
145	'http://launchpad.dev/~ubuntu-team/+poll/not-yet-opened'
146
147	- >>> browser.getLink('Show option details').click()
148	+ >>> browser.getLink('Show options').click()
149	>>> browser.url
150	'http://launchpad.dev/~ubuntu-team/+poll/not-yet-opened/+options'
151
152
153	=== modified file 'lib/lp/registry/templates/poll-edit.pt'
154	--- lib/lp/registry/templates/poll-edit.pt 2009-07-17 17:59:07 +0000
155	+++ lib/lp/registry/templates/poll-edit.pt 2009-08-18 20:24:56 +0000
156	@@ -3,19 +3,12 @@
157	xmlns:tal="http://xml.zope.org/namespaces/tal"
158	xmlns:metal="http://xml.zope.org/namespaces/metal"
159	xmlns:i18n="http://xml.zope.org/namespaces/i18n"
160	- xml:lang="en"
161	- lang="en"
162	- dir="ltr"
163	- metal:use-macro="context/@@main_template/master"
164	+ metal:use-macro="view/macro:page/main_only"
165	i18n:domain="launchpad"
166	>
167
168	<body>
169
170	- <metal:portlets fill-slot="portlets">
171	- <div tal:replace="structure context/@@+portlet-details" />
172	- </metal:portlets>
173	-
174	<div metal:fill-slot="main">
175
176	<div tal:condition="context/isNotYetOpened">
177	@@ -33,6 +26,8 @@
178	opens it can't be edited anymore.</p>
179	</div>
180
181	+ <tal:menu replace="structure view/@@+related-pages" />
182	+
183	</div>
184
185	</body>
186
187	=== modified file 'lib/lp/registry/templates/poll-newoption.pt'
188	--- lib/lp/registry/templates/poll-newoption.pt 2009-07-17 17:59:07 +0000
189	+++ lib/lp/registry/templates/poll-newoption.pt 2009-08-18 20:24:56 +0000
190	@@ -3,19 +3,12 @@
191	xmlns:tal="http://xml.zope.org/namespaces/tal"
192	xmlns:metal="http://xml.zope.org/namespaces/metal"
193	xmlns:i18n="http://xml.zope.org/namespaces/i18n"
194	- xml:lang="en"
195	- lang="en"
196	- dir="ltr"
197	- metal:use-macro="context/@@main_template/master"
198	+ metal:use-macro="view/macro:page/main_only"
199	i18n:domain="launchpad"
200	>
201
202	<body>
203
204	- <metal:portlets fill-slot="portlets">
205	- <div tal:replace="structure context/@@+portlet-details" />
206	- </metal:portlets>
207	-
208	<div metal:fill-slot="main">
209
210	<tal:block condition="context/isNotYetOpened">
211	@@ -37,6 +30,8 @@
212	</p>
213	</tal:block>
214
215	+ <tal:menu replace="structure view/@@+related-pages" />
216	+
217	</div>
218
219	</body>
220
221	=== modified file 'lib/lp/registry/templates/polloption-edit.pt'
222	--- lib/lp/registry/templates/polloption-edit.pt 2009-07-17 17:59:07 +0000
223	+++ lib/lp/registry/templates/polloption-edit.pt 2009-08-18 20:24:56 +0000
224	@@ -3,19 +3,12 @@
225	xmlns:tal="http://xml.zope.org/namespaces/tal"
226	xmlns:metal="http://xml.zope.org/namespaces/metal"
227	xmlns:i18n="http://xml.zope.org/namespaces/i18n"
228	- xml:lang="en"
229	- lang="en"
230	- dir="ltr"
231	- metal:use-macro="context/@@main_template/master"
232	+ metal:use-macro="view/macro:page/main_only"
233	i18n:domain="launchpad"
234	>
235
236	<body>
237
238	- <metal:portlets fill-slot="portlets">
239	- <div tal:replace="structure context/poll/@@+portlet-details" />
240	- </metal:portlets>
241	-
242	<div metal:fill-slot="main">
243
244	<tal:block condition="context/poll/isNotYetOpened">
245
246	=== modified file 'lib/lp/registry/templates/team-polls.pt'
247	--- lib/lp/registry/templates/team-polls.pt 2009-07-17 17:59:07 +0000
248	+++ lib/lp/registry/templates/team-polls.pt 2009-08-18 20:24:56 +0000
249	@@ -78,8 +78,8 @@
250
251	<br />
252	<tal:block tal:condition="request/lp:person">
253	- <ul class="add" tal:condition="context/required:launchpad.Edit">
254	- <li><a href="+newpoll">Set up a new poll</a></li>
255	+ <ul tal:condition="context/required:launchpad.Edit">
256	+ <li><a class="sprite add" href="+newpoll">Set up a new poll</a></li>
257	</ul>
258	</tal:block>
259
260