Merge lp:~sinzui/launchpad/launchpad-header-0 into lp:launchpad
| Status: | Merged |
|---|---|
| Approved by: | Paul Hummer on 2010-07-24 |
| Approved revision: | no longer in the source branch. |
| Merged at revision: | 11229 |
| Proposed branch: | lp:~sinzui/launchpad/launchpad-header-0 |
| Merge into: | lp:launchpad |
| Diff against target: |
1273 lines (+397/-542) 13 files modified
lib/canonical/launchpad/doc/hierarchical-menu.txt (+20/-14) lib/canonical/launchpad/icing/style-3-0.css.in (+28/-13) lib/lp/app/browser/tests/base-layout.txt (+1/-280) lib/lp/app/browser/tests/test_base_layout.py (+163/-0) lib/lp/app/templates/base-layout-macros.pt (+1/-1) lib/lp/app/templates/base-layout.pt (+18/-13) lib/lp/app/templates/launchpad-hierarchy.pt (+4/-4) lib/lp/blueprints/stories/sprints/05-sprint-creation.txt (+99/-117) lib/lp/bugs/stories/bugs/xx-bug-heat-on-bug-page.txt (+4/-4) lib/lp/bugs/stories/bugs/xx-malone-homepage.txt (+10/-18) lib/lp/bugs/stories/bugtracker/bugtrackers-index.txt (+4/-11) lib/lp/bugs/stories/cve/cve-pages.txt (+40/-65) lib/lp/bugs/templates/bugtask-index.pt (+5/-2) |
| To merge this branch: | bzr merge lp:~sinzui/launchpad/launchpad-header-0 |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Paul Hummer (community) | ui | 2010-07-23 | Approve on 2010-07-24 |
| Brad Crittenden (community) | code | Approve on 2010-07-23 | |
| Robert Collins (community) | 2010-07-21 | Abstain on 2010-07-22 | |
|
Review via email:
|
|||
Description of the Change
This is my branch to fix the launchpad header so for the title edit widget.
This branch fixes a number of visual issues in the header, but in doing so,
it call attention to the registered slot. Michael and I decided that the
registration slot is out of scope for this branch and the probable solution
is to move it, which requires a reexamination of all its uses.
lp:~sinzui/launchpad/owner-driver-supervisor
Diff size: 643
Launchpad bug: https:/
Test command: ./bin/test -vv -t test_base_layout -t base-layout
Pre-
Target release: 10.08
Fix the launchpad header so for the title edit widget
-------
The floated left and right instructions in the launchpad header break
the layout of the title edit widget; The widget disassembles itself trying
to avoid the floated content.
Rules
-----
* Consider using inline-block CSS and relative positioning to fix the
issue.
Michael suggested:
* ADDENDUM: Fix the leading white space before the watermark.
* ADDENDUM: Align the apps with the bottom of the icon so that there is
no extra white space between the context, apps, heading and bead crumbs.
Curtis:
* Michael's suggestions are very good. They make other problems in the
header more obvious. The registration slot is taking advantage of the
unwanted white space. It will be obvious that it was an after thought
when it creates an extra line. If it is clear that the apps are the
second part of the header, it may be clear to some users they appear
in the wrong place in the bread crumbs.
* ADDENDUM: Move the check for bread crumbs to the list element so that
no part of the block is rendered when there is no bread crumbs.
QA
--
* Visit https:/
* Choose the edit the title
* Verify the widget stays together and remains adjacent to the logo.
* Visit https:/
* Verify the context, apps, title, and bread crumbs are together...
the reported by line does not interrupt.
* Visit https:/
* Verify the context, apps, title, and bread crumbs are together
Lint
----
Linting changed files:
lib/canonical
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
Test
----
* lib/lp/
* Removed big chunks of a very brittle test that broke from the minor
markup changes. The diff shows the tests for common parts were
different, but they where/should be the same. I decided to move
the testing to a unittest. Note that the diff /before/ my decision to
to replace the tests was long and hard to read.
* lib/lp/
* Moved the tests for the parts that base-layout provides to unittests.
I should now be obvious when parts each layout provides.
Implementation
--------------
* lib/canonical/
* Added .flowed-block which allows us to layout horizontal chunks
of block content like we image it in our heads.
* Replaced #locationbar with.login-logout which accidentally solved
Michaels concern about the leading space :)
* Updated watermark-
content so that the title edit widget does not need to avoid other
parts of the header.
* Removed the excess whitespace from .registering and #logincontrol
* lib/lp/
* Added an id to make the content easy to test.
* lib/lp/
* Wrapped the problem code in flowed-block divs.
* Moved the registration slot below the logo so that there was no
floated content near the heading that the title edit widget wraps.
* Added ids to make content testable.
* lib/lp/
* Moved the bread crumb test condition to the block element...the page
was always rendering space for the bread crumbs.
| Curtis Hovey (sinzui) wrote : | # |
These are the three headers Michael and I looked at. They represent the best and worst of how the header works. In all cases the space before the logo and the space without bread crumbs is fixed \o/. The title edit widget hold together since it does not need to dodge the left and right content. By aligning the primary context title and apps with the bottom of the logo, view.png shows what we wanted users to see. But the two context images have a registration line that inserts a blank line. Registration always did this, it was not as obvious.
http://
http://
http://
http://
I ponder moving the registation before or after the bread crumbs. We want the registration to be adjacent to the title of the context. This was/is not possible for primary contexts. We rely on the sparceness of the header to imply that the context is the only thing the registration text can pertain to. Placing the registration before the breadcrumbs, a, returns to the orginal design for 3.0, and b, puts the information closer to the primary context logo, and the secondary context title when it exists.
/me hacks to make screen caps.
| Curtis Hovey (sinzui) wrote : | # |
I moved the registration slot into context-location area, it it between the context title and bread crumbs. I made the text the same size as the bread crumbs.
http://
http://
http://
http://
These changes did not inflate the diff. I updated a test to verify the content moved.
The bug page did look bad, and that is because it violates the registration slot rules...the
data is immutable; it is not a status. I moved the heat inline and updated the one affected
test. Another was robust. I removed the redundant bug number too (no tests broke).
./bin/test -vvc -t bug-heat-view -t xx-bug-
| Brad Crittenden (bac) wrote : | # |
Robert, if you claim a review from the team and then abstain the review disappears from the view of other reviewers. To remedy, select 'Request another review' and assign it to the Launchpad Reviewers team. (Sorry for cluttering the MP with these instructions but it makes sense since it provides context.)
| Brad Crittenden (bac) wrote : | # |
Curtis I think these changes really clean up the top of the page. The code looks good. Another round of UI review will be beneficial.
This docstring needs a final period:
"""Test for the ILaunchpadRoot permission"""
| Curtis Hovey (sinzui) wrote : | # |
Hi Paul.
In summary. The start of the review says I did not want to fix the registration slot issue, and you can see pictures of how fixing the white space made the problem very obvious. The more recent comments with the better-* set of images shows I decided to move the registration slot anyway. I think this is an improvement, though I would not ever claim it is perfect.
| Paul Hummer (rockstar) wrote : | # |
This looks REALLY good. Thanks for dealing with it. The header can sometimes be an eyesore, so anything we can do to make it better is something I'm onboard with.

This sounds nice, but I'm not comfortable enough with this part of the system yet to be your reviewer - sorry!