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.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
