Merge lp:~bac/launchpad/hidden_links into lp:launchpad

[Brad Crittenden]

[r=thumper][ui=sinzui][no-qa] Allow menu links to be enabled but hidden.

= Summary =

We have a need to have a link enabled but hidden which will subsequently
be made visible via JavaScript. If it were not hidden then browsers
which don't support JavaScript would see the link which would be a dead-end.

That said, this branch modifies the Link mechanism to add a 'hidden'
attribute. Links that are hidden but enabled will be rendered with the
class 'invisible-link'.

== Proposed fix ==

As above.

== Pre-implementation notes ==

Talk with Curtis.

== Implementation details ==

As above.

== Tests ==

bin/test -vvt doc/menus.txt

== Demo and Q/A ==

None

= Launchpad lint =

Checking for conflicts and issues in changed files.

I may fix these pre-existing problems but I may not.

Linting changed files:
  lib/canonical/launchpad/webapp/interfaces.py
  lib/canonical/launchpad/webapp/menu.py
  lib/canonical/launchpad/templates/launchpad-inline-link.pt
  lib/canonical/launchpad/doc/menus.txt

./lib/canonical/launchpad/webapp/interfaces.py
     879: redefinition of unused 'StartRequestEvent' from line 871
      42: 'UnexpectedFormData' imported but unused
     279: E301 expected 1 blank line, found 0
     281: E202 whitespace before ')'
     289: E302 expected 2 blank lines, found 1
     309: E202 whitespace before ')'
     314: E301 expected 1 blank line, found 0
     317: E301 expected 1 blank line, found 0
     319: E301 expected 1 blank line, found 0
     321: E301 expected 1 blank line, found 0
     428: E301 expected 1 blank line, found 0
     435: E301 expected 1 blank line, found 0
     443: E301 expected 1 blank line, found 0
     458: E301 expected 1 blank line, found 0
     502: E302 expected 2 blank lines, found 1
     549: E302 expected 2 blank lines, found 1
     611: E202 whitespace before ')'
     644: E202 whitespace before ')'
     756: E301 expected 1 blank line, found 0
     826: E301 expected 1 blank line, found 0
     873: E301 expected 1 blank line, found 0
     879: E301 expected 1 blank line, found 2
     200: Line exceeds 78 characters.
./lib/canonical/launchpad/doc/menus.txt
       1: narrative uses a moin header.
       6: narrative uses a moin header.
      55: narrative uses a moin header.
     102: source exceeds 78 characters.
     164: narrative uses a moin header.
     253: narrative uses a moin header.
     333: source exceeds 78 characters.
     370: narrative uses a moin header.
     429: narrative uses a moin header.
     501: narrative uses a moin header.
     568: narrative uses a moin header.
     576: source exceeds 78 characters.
     688: narrative uses a moin header.
     878: narrative uses a moin header.
     897: narrative uses a moin header.
     909: narrative uses a moin header.

[Tim Penhey (thumper)]

Hi Brad,

Nothing wrong with the concept, but I have a few comments
on the implementation.

Can we not have a single tal expression to render the link?

Move the full class definition into the view object, and should
specify the invisible-link or not. Also avoiding the inline
style of "line-height: 20px". That should be moved to a class
style in our stylesheet for invisible-link.

Also the anchor tag specifying empty href, class and title
attributes had me confused until I looked at the tal:attributes.
I find they detract more than they help. Can you remove them?

review: Needs Fixing

[Brad Crittenden (bac)]

Hi Tim,

I didn't move the class definition into the view originally as I was under the impression the view wanted to remain generic and the customization of the rendering was done via the templates. But I see your point and will do it.

Curtis and I discussed the "line-height" issue and he wanted to have a full investigation of it to see if it is the culprit in some alignment issues. He asked that I not remove it in this pass. Also in the past there has been vigorous discouragement of moving one-off stylings into the CSS.

Finally, I too was confused by the empty anchor tag attributes. I meant to remove them but forgot. Thanks for pointing these out.

[Tim Penhey (thumper)]

Brad, a little aggressive in the removal of code. A link with no link should be a <span>

If you fix this, you're good to go.

review: Approve

[Curtis Hovey (sinzui)]

Hi brad. Sorry about not completing the review. I suck. As I said in the implementation talk, He cannot mess with line-height. It is wrong and needs to be removed. I will remove the source you cargo-culted.

review: Approve (ui)

=== modified file 'lib/canonical/launchpad/browser/launchpad.py'
--- lib/canonical/launchpad/browser/launchpad.py	2011-03-15 09:26:33 +0000
+++ lib/canonical/launchpad/browser/launchpad.py	2011-03-18 19:17:47 +0000
@@ -211,6 +211,33 @@
         else:
             return ''
 
+    @property
+    def css_class(self):
+        """Return the CSS class."""
+        value = ["menu-link-%s" % self.context.name]
+        if not self.context.linked:
+            value.append('nolink')
+        if self.context.icon:
+            value.append(self.sprite_class)
+            value.append(self.context.icon)
+        if self.context.hidden:
+            value.append('invisible-link')
+        return " ".join(value)
+
+    @property
+    def url(self):
+        """Return the url if linked."""
+        if self.context.linked:
+            return self.context.url
+        return ''
+
+    @property
+    def summary(self):
+        """Return the summary if linked."""
+        if self.context.linked:
+            return self.context.summary
+        return ''
+
 
 class Hierarchy(LaunchpadView):
     """The hierarchy part of the location bar on each page."""

=== modified file 'lib/canonical/launchpad/doc/menus.txt'
--- lib/canonical/launchpad/doc/menus.txt	2010-11-07 21:15:07 +0000
+++ lib/canonical/launchpad/doc/menus.txt	2011-03-18 19:17:47 +0000
@@ -192,7 +192,7 @@
     ...         target = '+foo'
     ...         text = 'Foo'
     ...         return Link(target, text)
-    ... 
+    ...
     ...     def bar(self):
     ...         target = '+bar'
     ...         text = 'Bar'
@@ -215,6 +215,7 @@
     --- link foo ---
     enabled: True
     escapedtext: Foo
+    hidden: False
     icon: None
     icon_url: None
     linked: True
@@ -232,6 +233,7 @@
     --- link bar ---
     enabled: True
     escapedtext: Bar
+    hidden: False
     icon: None
     icon_url: None
     linked: True
@@ -357,6 +359,13 @@
     # Clean up our special login.
     >>> login(ANONYMOUS)
 
+A menu item can be marked as hidden even though it is enabled.
+
+    >>> link = Link('z', 'text', 'summary', icon='icon', hidden=True)
+    >>> print ILink(link).render() #doctest: +NORMALIZE_WHITESPACE
+    <a class="menu-link-None sprite icon invisible-link"
+        title="summary">text</a>
+
 
 == How do we tell which link from a facetmenu is the selected one? ==
 
@@ -448,10 +457,10 @@
 adapter.  For this part of the test, we won't worry about that.
 
     >>> class FooApplicationMenu(ApplicationMenu):
-    ... 
+    ...
     ...     links = ['first']
     ...     facet = 'foo'
-    ... 
+    ...
     ...     def first(self):
     ...         target = '+first'
     ...         text = 'First menu'
@@ -473,6 +482,7 @@
     --- link first ---
     enabled: True
     escapedtext: First menu
+    hidden: False
     icon: None
     icon_url: None
     linked: True
@@ -515,9 +525,9 @@
 For this part of the test, we won't worry about that.
 
     >>> class MyContextMenu(ContextMenu):
-    ... 
+    ...
     ...     links = ['first']
-    ... 
+    ...
     ...     def first(self):
     ...         target = '+firstcontext'
     ...         text = 'First context menu item'
@@ -539,6 +549,7 @@
     --- link first ---
     enabled: True
     escapedtext: First context menu item
+    hidden: False
     icon: None
     icon_url: None
     linked: True
@@ -583,9 +594,9 @@
 
     >>> class FacetsForThing(Facets):
     ...     usedfor = IThingHavingFacets
-    ... 
+    ...
     ...     links = ['foo', 'bar', 'baz']
-    ... 
+    ...
     ...     def baz(self):
     ...         target = ''
     ...         text = 'baz'
@@ -694,16 +705,16 @@
     >>> from canonical.launchpad.webapp.vhosts import allvhosts
     >>> class FakeRequest:
     ...     implements(IHTTPApplicationRequest, IBrowserRequest)
-    ... 
+    ...
     ...     interaction = None
-    ... 
+    ...
     ...     def __init__(self, url, query=None, url1=None):
     ...         self.url = url
     ...         self.query = query
     ...         self.url1 = url1  # returned from getURL(1)
     ...         self.method = 'GET'
     ...         self.annotations = {}
-    ... 
+    ...
     ...     def getURL(self, level=0):
     ...         assert 0 <= level <=1, 'level must be 0 or 1'
     ...         if level == 0:
@@ -712,7 +723,7 @@
     ...             assert self.url1 is not None, (
     ...                 'Must set url1 in FakeRequest')
     ...             return self.url1
-    ... 
+    ...
     ...     def getRootURL(self, rootsite):
     ...         if rootsite is not None:
     ...             return allvhosts.configs[rootsite].rooturl
@@ -722,14 +733,14 @@
     ...     def getApplicationURL(self):
     ...         # Just the http://place:port part, so stop at the 3rd slash.
     ...         return '/'.join(self.url.split('/', 3)[:3])
-    ... 
+    ...
     ...     def get(self, key, default=None):
     ...         assert key == 'QUERY_STRING', 'we handle only QUERY_STRING'
     ...         if self.query is None:
     ...             return default
     ...         else:
     ...             return self.query
-    ... 
+    ...
     ...     def setPrincipal(self, principal):
     ...         self.principal = principal
     >>> request = FakeRequest('http://launchpad.dev/sesamestreet/+bar')
@@ -906,7 +917,7 @@
     >>> from canonical.launchpad.webapp import enabled_with_permission
 
     >>> class SomeMenu(ContextMenu):
-    ... 
+    ...
     ...     @enabled_with_permission('launchpad.Admin')
     ...     def foo(self):
     ...         return Link('+admin', 'Admin the foo')
@@ -930,4 +941,3 @@
     >>> foolink = somemenu.foo()
     >>> foolink.enabled
     True
-

=== modified file 'lib/canonical/launchpad/templates/launchpad-inline-link.pt'
--- lib/canonical/launchpad/templates/launchpad-inline-link.pt	2010-08-02 16:36:09 +0000
+++ lib/canonical/launchpad/templates/launchpad-inline-link.pt	2011-03-18 19:17:47 +0000
@@ -1,40 +1,24 @@
 <tal:link-enabled
   xmlns:tal="http://xml.zope.org/namespaces/tal"
   condition="context/enabled">
+
   <tal:link-linked
-    condition="context/linked"><a
-      href="" class="" title=""
-      tal:condition="context/icon"
-      tal:attributes="
-        class string:menu-link-${context/name} ${view/sprite_class} ${context/icon};
-        href context/url;
-        title context/summary;
-        "
-      tal:content="structure context/escapedtext"
-    /><a
-      tal:condition="not: context/icon"
-      style="line-height: 20px;"
-      href="" class="" title=""
-      tal:attributes="
-        class string:menu-link-${context/name};
-        href context/url;
-        title context/summary;
-        "
-      tal:content="structure context/escapedtext"
-    /></tal:link-linked>
+     condition="context/linked">
+     <a tal:attributes="
+         class view/css_class;
+         href view/url;
+         title view/summary;
+         "
+         tal:content="structure context/escapedtext"
+     />
+  </tal:link-linked>
+
   <tal:link-not-linked
-    condition="not: context/linked"><span
-      tal:condition="context/icon"
-      tal:attributes="
-        class string:menu-link-${context/name} nolink
-              ${view/sprite_class} ${context/icon};
-        "
-      tal:content="structure context/escapedtext"
-    >link text</span><span
-      tal:condition="not: context/icon"
-      tal:attributes="
-        class string:menu-link-${context/name} nolink
-        "
-      tal:content="structure context/escapedtext"
-    >link text</span></tal:link-not-linked>
+      condition="not: context/linked">
+      <span
+          tal:attributes="class view/css_class"
+          tal:content="structure context/escapedtext"
+      >link text</span>
+  </tal:link-not-linked>
+
 </tal:link-enabled>

=== modified file 'lib/canonical/launchpad/webapp/interfaces.py'
--- lib/canonical/launchpad/webapp/interfaces.py	2011-01-20 23:55:37 +0000
+++ lib/canonical/launchpad/webapp/interfaces.py	2011-03-18 19:17:47 +0000
@@ -187,6 +187,12 @@
     sort_key = Attribute(
         "The sort key to use when rendering it with a group of links.")
 
+    hidden = Attribute(
+        "Boolean to say whether this link is hidden.  This is separate from "
+        "being enabled and is used to support links which need to be be "
+        "enabled but not viewable in the rendered HTML.  The link may be "
+        "changed to visible by JavaScript or some other means.")
+
 
 class ILink(ILinkData):
     """An object that represents a link in a menu.

=== modified file 'lib/canonical/launchpad/webapp/menu.py'
--- lib/canonical/launchpad/webapp/menu.py	2011-03-01 05:00:01 +0000
+++ lib/canonical/launchpad/webapp/menu.py	2011-03-18 19:17:47 +0000
@@ -128,7 +128,7 @@
     implements(ILinkData)
 
     def __init__(self, target, text, summary=None, icon=None, enabled=True,
-                 site=None, menu=None):
+                 site=None, menu=None, hidden=False):
         """Create a new link to 'target' with 'text' as the link text.
 
         'target' is a relative path, an absolute path, or an absolute url.
@@ -158,6 +158,7 @@
         self.enabled = enabled
         self.site = site
         self.menu = menu
+        self.hidden = hidden
 
 Link = LinkData
 

=== modified file 'lib/lp/app/browser/tests/menu.txt'
--- lib/lp/app/browser/tests/menu.txt	2009-10-30 17:54:58 +0000
+++ lib/lp/app/browser/tests/menu.txt	2011-03-18 19:17:47 +0000
@@ -87,8 +87,7 @@
     <BLANKLINE>
     <BLANKLINE>
         <li>
-            <a style="..."
-               href="http://launchpad.dev/~beaker/+edit-people"
+            <a href="http://launchpad.dev/~beaker/+edit-people"
                class="menu-link-edit_people">Edit people related to thing</a>
         </li>
         ...
@@ -145,11 +144,10 @@
       <ul>
         <li>
           <span class="menu-link-edit_thing nolink
-                      sprite modify edit">Edit thing</span>
+              sprite modify edit">Edit thing</span>
         </li>
         <li>
-          <a style="..."
-             href="http://launchpad.dev/~beaker/+edit-people"
+          <a href="http://launchpad.dev/~beaker/+edit-people"
              class="menu-link-edit_people">Edit people related to thing</a>
         </li>
       </ul>