Launchpad itself

Code review comment for lp:~sinzui/launchpad/milestone-design-oops

milestone-design-oops
Merge into devel

Revision history for this message

Curtis Hovey (sinzui) wrote on 2009-09-17:

On Thu, 2009-09-17 at 08:30 +0000, Michael Nelson wrote:
> Review: Approve code ui*
> > This is my branch to fix timeout issues and IProject issues with the
> > milestone page and improve the layout.
> >
>
> Hi Curtis, thanks for all the details here - and for finding a solution
> to a common problem!
>
> r=me (code/ui*)
>
> I've got some questions below, but I'll leave it up to you whether they
> are worth considering.
>
> > lp:~sinzui/launchpad/milestone-design-oops
> > Diff size: 155
> > Launchpad bug: https://bugs.launchpad.net/bugs/430726
> > Test command: ./bin/test -vv -t "milestone-views|object-milestones"
> > Pre-implementation: wgrant
> > Target release: 3.1.0
>
> Did I miss 3.0?

3.0 is mis named. The series is 3.1.

> >
> >
> > = Fix timeout issues and IProject issues with the milestone page =
> >
> > There is a content and a design problem with the milestone page.
> >
> > 1. There are many more timeouts because of the cost of showing the bug tags.
> > Remove them, they do not work well are they are in the design.
> >
> > 2. For most users, the side bar has one link, and if the context is an
> > IProject, there is no side bar. The content does not fill the page well.
> > Remove the side bar from the IProject case, consider moving the three
> > action links inline or float a fake side bar so that the content can fill
> > the page.
> >
> >
> > == Rules ==
> >
> > 1. Removed the tags from the page.
>
> Great.
>
> >
> > 2. This is hard to do. I had several failures before settling on the CSS
> > approach.
> > a) I could not conditional set the page macro
>
> I haven't need to do this but, if it's helpful, I found an example at:
>
> http://docs.zope.org/zope2/zope2book/source/ZPT.html#macro-details
>
> I assume this should work the same for us?

I am very familiar with metal and tal. The problem is the we are *way*
to clever for our own good. There is some crazy abstraction going on
when we call macro: that ensures we call the whole macro...getting part
of the object is a no-no. I thought I could create two <html> nodes for
each condition:

But once we call the first one, the state is set.

The object state cannot be monkeyed with because we are talking to a
cached proxy that guards access to the real object. So I cannot get the
the object in python and pass it as a variable.

I think we could dismantle the insanity when we tear out the 1.0 and 2.0
support from the macro.

> > b) I could not reliably make a mock side bar in a main_only template --
> > If someone changed the yui-* definition in the CSS, the template would
> > be out of sync.
>
> Yep, good catch.
>
> > c) I tried using CSS to collapse the yui-* widths to reclaim the space,
> > but Firefox ignored them
> > d) I decided to give up a just add a conditional CSS block that hides the
> > empty background of the side bar.
>
> This seems to work well.
>
> >
> > 3. I then saw that the Project milestone table is still very compressed
> > because it has a "Product" column. Well that is not right. It should
> > read "Project"
> >
> > 4. The I experimented with a new class that was on a different z-index and
> > very wide. The bug and spec work items filled the space very well.
>
> Great.
>
> >
> > 5. I decided to a .wide to the release files portlets so that its description
> > was not compressed.
>
> Yes, I agree it looks better.
>
> >
> >
> > == QA ==
> >
> > The side bar and wide tables show look like these images.
> > http://people.canonical.com/~curtis/milestones/
> >
> >
> > == Lint ==
> >
> > Linting changed files:
> > lib/canonical/launchpad/icing/style-3-0.css
> > lib/lp/registry/browser/tests/milestone-views.txt
> > lib/lp/registry/stories/milestone/object-milestones.txt
> > lib/lp/registry/templates/milestone-index.pt
> >
> >
> > == Test ==
> >
> > * lib/lp/registry/browser/tests/milestone-views.txt
> > * Added a test to verify that CSS is added to hide the project group
> > milestone side bar.
> > * lib/lp/registry/stories/milestone/object-milestones.txt
> > * Updated the test to verify that the UI uses "Project", not "Product".
> >
> >
> > == Implementation ==
> >
> > * lib/canonical/launchpad/icing/style-3-0.css
> > * Added a wide rule for content to reclaim the space lost to the
> > side bar.
> > * lib/lp/registry/templates/milestone-index.pt
> > * Added CSS for the is_project_milestone condition
> > * Added .wide to the product release files portlet and the bug/blueprint
> > work items portlet
> > * Removed the bug tags that were cause a zillion queries.
>
> Heh.
>
> > === modified file 'lib/canonical/launchpad/icing/style-3-0.css'
> > --- lib/canonical/launchpad/icing/style-3-0.css 2009-09-14 10:13:55 +0000
> > +++ lib/canonical/launchpad/icing/style-3-0.css 2009-09-17 07:26:10 +0000
> > @@ -237,7 +237,10 @@
> > padding: 0 0 0.5em 0;
> > margin: 0 0 1em;
> > }
> > -
> > +.wide {
> > + z-index: 10;
> > + width: 131%;
> > + }
>
> I wonder if we should give this a more meaningful name, perhaps something
> like 'full-width-main-side'? Maybe that' too verbose. See what you think.

Maybe. I do not link wide, but I also want to avoid a meaningless name
in 1 year when we change the design again. I'll use full-page-width.

...

> > === modified file 'lib/lp/registry/browser/tests/milestone-views.txt'
> > --- lib/lp/registry/browser/tests/milestone-views.txt 2009-09-11 20:38:21 +0000
> > +++ lib/lp/registry/browser/tests/milestone-views.txt 2009-09-17 07:26:10 +0000
> > @@ -259,6 +259,29 @@
> >
> > >>> login_person(person)
> >
> > +
> > +Project milestones
> > +------------------
> > +
> > +The projectgroup milestones are virtual and cannot be modified. The template
> > +generates CSS that hides the space occupied by the side portlets.
> > +
> > + >>> projectgroup = factory.makeProject(name='flock')
> > + >>> product.project = projectgroup
> > + >>> project_milestone = projectgroup.getMilestone('kakapo')
> > + >>> view = create_initialized_view(
> > + ... project_milestone, '+index', principal=person)
> > + >>> print find_tag_by_id(view.render(), 'hide-side-portlets')['type']
> > + text/css
> > +
> > +A normal milestone does not have the CSS rule.
> > +
> > + >>> view = create_initialized_view(
> > + ... milestone, '+index', principal=person)
> > + >>> print find_tag_by_id(content, 'hide-side-portlets')
> > + None
>
> The test is fine - I've got a more general question/discussion point
> that I'd be keen to chat with you about some other time:
>
> I remember you mentioning this way of testing the template as part of
> the view test - and I can understand that it makes the stories much easier
> to read by removing the unnecessary detail, but I'm not convinced that
> the view test is the place for it. I've always assumed the dependencies
> are:
>
> model -> view -> template
>
> That is, if I update a model, all tests could fail. If I update a view,
> the model test cannot fail, but the view and template tests might. If I
> update a template, the model and view tests should not fail, but the
> template tests might.
>
> I think what I'm trying to get at is, perhaps we should have some separate
> template tests that do exactly what you've done here (or alternatively as
> a unittest)? That way the stories stay clean, but the view tests would
> remain focused on view logic?

I understand you point. The problem is that stories are not the place
for testing conditional logic. They only verify a user can perform a
basic action moving from one page to the next. The user does not care
how we make the side bar appear or disappear.

We could have a separate file to test that the template responds the of
the view and context state. There are some in fact because the test of
the view was getting large.

__C U R T I S C. H O V E Y_______
Guilty of stealing everything I am.

On Thu, 2009-09-17 at 08:30 +0000, Michael Nelson wrote:
> Review: Approve code ui*
> > This is my branch to fix timeout issues and IProject issues with the
> > milestone page and improve the layout.
> > 
> 
> Hi Curtis, thanks for all the details here - and for finding a solution
> to a common problem!
> 
> r=me (code/ui*)
> 
> I've got some questions below, but I'll leave it up to you whether they
> are worth considering.
> 
> >     lp:~sinzui/launchpad/milestone-design-oops
> >     Diff size: 155
> >     Launchpad bug: https://bugs.launchpad.net/bugs/430726
> >     Test command: ./bin/test -vv -t "milestone-views|object-milestones"
> >     Pre-implementation: wgrant
> >     Target release: 3.1.0
> 
> Did I miss 3.0?

3.0 is mis named. The series is 3.1.

> > 
> > 
> > = Fix timeout issues and IProject issues with the milestone page =
> > 
> > There is a content and a design problem with the milestone page.
> > 
> > 1. There are many more timeouts because of the cost of showing the bug tags.
> >     Remove them, they do not work well are they are in the design.
> > 
> > 2. For most users, the side bar has one link, and if the context is an
> >    IProject, there is no side bar. The content does not fill the page well.
> >    Remove the side bar from the IProject case, consider moving the three
> >    action links inline or float a fake side bar so that the content can fill
> >    the page.
> > 
> > 
> > == Rules ==
> > 
> > 1. Removed the tags from the page.
> 
> Great.
> 
> > 
> > 2. This is hard to do. I had several failures before settling on the CSS
> >    approach.
> >    a) I could not conditional set the page macro
> 
> I haven't need to do this but, if it's helpful, I found an example at:
> 
> http://docs.zope.org/zope2/zope2book/source/ZPT.html#macro-details
> 
> I assume this should work the same for us?

But once we call the first one, the state is set.

The object state cannot be monkeyed with because we are talking to a
cached proxy that guards access to the real object. So I cannot get the
the object in python and pass it as a variable.

I think we could dismantle the insanity when we tear out the 1.0 and 2.0
support from the macro.

> >    b) I could not reliably make a mock side bar in a main_only template --
> >       If someone changed the yui-* definition in the CSS, the template would
> >       be out of sync.
> 
> Yep, good catch.
> 
> >    c) I tried using CSS to collapse the yui-* widths to reclaim the space,
> >       but Firefox ignored them
> >    d) I decided to give up a just add a conditional CSS block that hides the
> >       empty background of the side bar.
> 
> This seems to work well.
> 
> > 
> > 3. I then saw that the Project milestone table is still very compressed
> >    because it has a "Product" column. Well that is not right. It should
> >    read "Project"
> > 
> > 4. The I experimented with a new class that was on a different z-index and
> >    very wide. The bug and spec work items filled the space very well.
> 
> Great.
> 
> > 
> > 5. I decided to a .wide to the release files portlets so that its description
> >    was not compressed.
> 
> Yes, I agree it looks better.
> 
> > 
> > 
> > == QA ==
> > 
> > The side bar and wide tables show look like these images.
> >     http://people.canonical.com/~curtis/milestones/
> > 
> > 
> > == Lint ==
> > 
> > Linting changed files:
> >   lib/canonical/launchpad/icing/style-3-0.css
> >   lib/lp/registry/browser/tests/milestone-views.txt
> >   lib/lp/registry/stories/milestone/object-milestones.txt
> >   lib/lp/registry/templates/milestone-index.pt
> > 
> > 
> > == Test ==
> > 
> >     * lib/lp/registry/browser/tests/milestone-views.txt
> >       * Added a test to verify that CSS is added to hide the project group
> >         milestone side bar.
> >     * lib/lp/registry/stories/milestone/object-milestones.txt
> >       * Updated the test to verify that the UI uses "Project", not "Product".
> > 
> > 
> > == Implementation ==
> > 
> >     * lib/canonical/launchpad/icing/style-3-0.css
> >       * Added a wide rule for content to reclaim the space lost to the
> >         side bar.
> >     * lib/lp/registry/templates/milestone-index.pt
> >       * Added CSS for the is_project_milestone condition
> >       * Added .wide to the product release files portlet and the bug/blueprint
> >         work items portlet
> >       * Removed the bug tags that were cause a zillion queries.
> 
> Heh.
> 
> > === modified file 'lib/canonical/launchpad/icing/style-3-0.css'
> > --- lib/canonical/launchpad/icing/style-3-0.css	2009-09-14 10:13:55 +0000
> > +++ lib/canonical/launchpad/icing/style-3-0.css	2009-09-17 07:26:10 +0000
> > @@ -237,7 +237,10 @@
> >      padding: 0 0 0.5em 0;
> >      margin: 0 0 1em;
> >      }
> > -
> > +.wide {
> > +    z-index: 10;
> > +    width: 131%;
> > +    }
> 
> I wonder if we should give this a more meaningful name, perhaps something
> like 'full-width-main-side'? Maybe that' too verbose. See what you think.

Maybe. I do not link wide, but I also want to avoid a meaningless name
in 1 year when we change the design again. I'll use full-page-width.

...

> > === modified file 'lib/lp/registry/browser/tests/milestone-views.txt'
> > --- lib/lp/registry/browser/tests/milestone-views.txt	2009-09-11 20:38:21 +0000
> > +++ lib/lp/registry/browser/tests/milestone-views.txt	2009-09-17 07:26:10 +0000
> > @@ -259,6 +259,29 @@
> >  
> >      >>> login_person(person)
> >  
> > +
> > +Project milestones
> > +------------------
> > +
> > +The projectgroup milestones are virtual and cannot be modified. The template
> > +generates CSS that hides the space occupied by the side portlets.
> > +
> > +    >>> projectgroup = factory.makeProject(name='flock')
> > +    >>> product.project = projectgroup
> > +    >>> project_milestone = projectgroup.getMilestone('kakapo')
> > +    >>> view = create_initialized_view(
> > +    ...     project_milestone, '+index', principal=person)
> > +    >>> print find_tag_by_id(view.render(), 'hide-side-portlets')['type']
> > +    text/css
> > +
> > +A normal milestone does not have the CSS rule.
> > +
> > +    >>> view = create_initialized_view(
> > +    ...     milestone, '+index', principal=person)
> > +    >>> print find_tag_by_id(content, 'hide-side-portlets')
> > +    None
> 
> The test is fine - I've got a more general question/discussion point
> that I'd be keen to chat with you about some other time:
> 
> I remember you mentioning this way of testing the template as part of
> the view test - and I can understand that it makes the stories much easier
> to read by removing the unnecessary detail, but I'm not convinced that
> the view test is the place for it. I've always assumed the dependencies
> are:
> 
> model -> view -> template
> 
> That is, if I update a model, all tests could fail. If I update a view,
> the model test cannot fail, but the view and template tests might. If I
> update a template, the model and view tests should not fail, but the
> template tests might.
> 
> I think what I'm trying to get at is, perhaps we should have some separate
> template tests that do exactly what you've done here (or alternatively as
> a unittest)? That way the stories stay clean, but the view tests would
> remain focused on view logic?

We could have a separate file to test that the template responds the of
the view and context state. There are some in fact because the test of
the view was getting large.

__C U R T I S  C.  H O V E Y_______
Guilty of stealing everything I am.

« Back to merge proposal