launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Review: Needs Fixing code and ui
Hi Brad.

This change looks great. I see I see extra white space between the download
portlet and announcements.

I think we need a model change to support this :( I an configure Answers to
not be official and my application may not be translatable. So the progress
bar will never be complete. Consider Launchpad. It does not have a po file,
so it cannot be translated, but the UI requires me to set translations to be
official to make the bar be complete. I think we need to None, True, or False
to make this work. I am seeing this same issue my the feature I am working
on. I propose we switch from bool to a vocab or UNKNOWN, LAUNCHPAD, EXTERNAL.
We may only need this some of the apps.

I have a lot of concern about the implementation. I do not think ILink, its
model, and template needed to change. I certainly do not think we should
be making invalid HTML on all Launchpad pages for a single portlet that
appears on one page. I think the fix is easy, but I have not spent a lot
of time looking at the issue.

> === modified file 'lib/canonical/launchpad/icing/style-3-0.css.in'
> --- lib/canonical/launchpad/icing/style-3-0.css.in	2010-07-26 14:48:43 +0000
> +++ lib/canonical/launchpad/icing/style-3-0.css.in	2010-07-26 22:01:07 +0000
> @@ -686,6 +686,13 @@
>      margin: 0;
>      padding: 0;
>      }
> +div.centered {
> +    text-align: center;
> +    }
> +div.centered table {
> +    margin: 0 auto;
> +    text-align: left;
> +    }

We used to have rules like this for the 1.0 layouts. I guess It was naive
of me to remove them.

> === modified file 'lib/canonical/launchpad/templates/launchpad-inline-link.pt'
> --- lib/canonical/launchpad/templates/launchpad-inline-link.pt	2009-08-27 02:22:07 +0000
> +++ lib/canonical/launchpad/templates/launchpad-inline-link.pt	2010-07-26 22:01:07 +0000
> @@ -2,8 +2,10 @@
>    xmlns:tal="http://xml.zope.org/namespaces/tal";
>    condition="context/enabled">
>    <tal:link-linked
> -    condition="context/linked"><a
> -      href="" class="" title=""
> +    condition="context/linked">
> +    <table width="100%"><tr>
> +        <td width="90%">
> +    <a href="" class="" title=""

This looks scary. I think it is wrong. This file is used to adapt every link,
and most links are inline text.

/me runs lp

This cannot land. It is create tables in paragraphs and inline markup that
cannot contain tables.

Since we are designing a presentation that will appear on exactly one page
and we have not discussed reusing it. I expected to see either a subclass
or some form of delegates so that the portlet's needs did not step on any
other part of launchpad. The Involvement portlet is already a bundle of
exceptions for menus and does its own layout. I think the view and existing
template could be doing all the work.

Maybe we should have looked closer at why the Involvement used CSS to place
a right aligned background image.

...

> === modified file 'lib/canonical/launchpad/webapp/interfaces.py'
> --- lib/canonical/launchpad/webapp/interfaces.py	2010-07-21 08:15:57 +0000
> +++ lib/canonical/launchpad/webapp/interfaces.py	2010-07-26 22:01:07 +0000
> @@ -238,6 +238,10 @@
>      icon_url = Attribute(
>          "The full URL for this link's associated icon, if it has one.")
>  
> +    configured = Attribute(
> +        "Whether the item has been configured yet.  If None, no indicator is"
> +        "used.  Otherwise it is a boolean.")
> +
>      def render():
>          """Return a HTML representation of the link."""

We have one use case for this. 
/me thinks the 'menu' arg is not used in Lp anymore.


> === modified file 'lib/canonical/launchpad/webapp/menu.py'
> --- lib/canonical/launchpad/webapp/menu.py	2010-06-15 19:35:41 +0000
> +++ lib/canonical/launchpad/webapp/menu.py	2010-07-26 22:01:07 +0000

...

> @@ -121,6 +121,9 @@
>          'blueprint' for a specific site.
>  
>          :param menu: The sub menu used by the page that the link represents.
> +
> +        :parem configured: Whether the item has been configured.  If not None,
> +        then show a boolean indicator for configured or not configured.

grammar: s/parem/param/

...

> @@ -171,6 +175,13 @@
>          else:
>              return cgi.escape(text)
>  
> +    def set_configured(self, value):
> +        self._linkdata.configured = value
> +    def get_configured(self):
> +        return self._linkdata.configured
> +
> +    configured = property(get_configured, set_configured)

PEP 8 will remind you to have a blank line between methods.

I do not think these changes are needed.

...

> === modified file 'lib/lp/registry/browser/product.py'
> --- lib/lp/registry/browser/product.py	2010-07-09 10:22:32 +0000
> +++ lib/lp/registry/browser/product.py	2010-07-26 22:01:07 +0000
> @@ -349,7 +349,33 @@
>      """Encourage configuration of involvement links for projects."""
>  
>      has_involvement = True
> -    visible_disabled_link_names = ['submit_code']

Why was this removed? Does this relate to the issue that we were not
seeing a strong indication that no one every configured translations, etc...?

> +    show_progress = True
> +
> +    @property
> +    def visible_disabled_link_names(self):
> +        """Show all disabled links...except blueprints"""
> +        involved_menu = MenuAPI(self).navigation
> +        all_links = involved_menu.keys()
> +        # The register blueprints link should not be shown since its use is
> +        # not encouraged.
> +        all_links.remove('register_blueprint')
> +        return all_links
> +
> +    @cachedproperty
> +    def configuration_states(self):
> +        """Create a dictionary indicating the configuration statuses.
> +
> +        Each app area will be represented in the return dictionary, except
> +        blueprints which we are not currently promoting.
> +        """
> +        states = {}
> +        states['configure_bugtracker'] = (
> +            self.official_malone or self.context.bugtracker is not None)
> +        states['configure_answers'] = self.official_answers
> +        states['configure_translations'] = self.official_rosetta
> +        states['configure_codehosting'] = (
> +            self.context.development_focus.branch is not None)
> +        return states

I think this work shows that the view is really doing the work and since
it has a special template for rendering itself, ILink and models and templates
do not need to change.

...

> === modified file 'lib/lp/registry/stories/product/xx-product-files.txt'
> --- lib/lp/registry/stories/product/xx-product-files.txt	2010-01-15 21:44:23 +0000
> +++ lib/lp/registry/stories/product/xx-product-files.txt	2010-07-26 22:01:07 +0000

Revert this change

> === modified file 'lib/lp/registry/templates/pillar-involvement-portlet.pt'
> --- lib/lp/registry/templates/pillar-involvement-portlet.pt	2010-06-15 19:37:37 +0000
> +++ lib/lp/registry/templates/pillar-involvement-portlet.pt	2010-07-26 22:01:07 +0000
> @@ -31,6 +31,29 @@
>      </tal:disabled>
>    </ul>
>  
> +  <tal:editor tal:condition="context/required:launchpad.Edit">
> +    <tal:show_configuration
> +       tal:condition="registration"
> +       tal:define="registration view/registration_completeness">
> +      <h2>Configuration Progress</h2>
> +      <div class="centered">
> +          <table width="80%" border="1">

Width and border attributes are deprecated. Use style.

I think this is the one template that needs to be making these special links:
I see:

  <ul tal:condition="view/configuration_links" style="padding-top: 1em">
    <li tal:repeat="link view/configuration_links">
      <a tal:replace="structure link/fmt:link" />
    </li>
  </ul>

If view/configuration_links returned a dict of link and state then:

  <ul tal:condition="view/configuration_links" style="padding-top: 1em">
    <li tal:repeat="configured_link view/configuration_links">
      <table>
        <tr>
          <td>
            <a tal:replace="structure configured_link/link/fmt:link" />
          </td>
          <td>
            <img src="/@@/${configured_link/configured}" />
          </td>
    </li>
  </ul>

Or using css:

  <ul tal:condition="view/configuration_links" style="padding-top: 1em">
    <li tal:repeat="configured_link view/configuration_links"
      attributes="class configured_link/css_state">
      <a tal:replace="structure configured_link/link/fmt:link" />
    </li>
  </ul>

...

> === modified file 'lib/lp/registry/tests/test_product.py'
> --- lib/lp/registry/tests/test_product.py	2010-04-21 16:15:36 +0000
> +++ lib/lp/registry/tests/test_product.py	2010-07-26 22:01:07 +0000

Revert this.
-- 
https://code.launchpad.net/~bac/launchpad/lep-projconfig/+merge/30999
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~bac/launchpad/lep-projconfig into lp:launchpad/devel.