Code review comment for lp:~sinzui/launchpad/full-page-width

Revision history for this message
Curtis Hovey (sinzui) wrote :

This is my branch to fix the milestone page width problem.

    lp:~sinzui/launchpad/full-page-width
    Diff size: 36
    Launchpad bug: https://bugs.launchpad.net/bugs/473738
    Test command: ./bin/test -vvt "stories/(milestone|productrelease)"
    Pre-implementation: No one
    Target release: 3.1.10

= Fix the milestone page width problem =

bug 473738 [milestone content exceeds the browser viewport]
    There seems to be an odd resizing bug on the +milestone page.
    The bugs and blueprints tables scroll to the right, even after
    the browser is maximised.

== Rules ==

This problem is only visible when there is a release. The release data
portlet is creating bad markup.

The <div style="clear:both" /> are well formed but invalid HTML. FF saw two
open divs, so it rendered the side portlets below the main content. This then
exposed a flaw in nested .full-page-width that caused the content to exceed
the browser viewport. Fixing the markup is all that is required, but added an
additional rule for nested .full-page-width is the right thing to do since the
content should never exceed the view port.

== QA ==

Before: http://people.canonical.com/~curtis/broken-page-width.png
    The side porlet is below the main content; the bug is wider than the page.

After: http://people.canonical.com/~curtis/fixed-page-width.png
    The side portlet is to the side. the bug fits in the page width.

== Lint ==

Linting changed files:
  lib/canonical/launchpad/icing/style-3-0.css
  lib/lp/registry/templates/productrelease-portlet-data.pt

== Test ==

No tests changed. The changes are CSS and layout markup

== Implementation ==

    * lib/canonical/launchpad/icing/style-3-0.css
      * Added a rule to ensure nested .full-page-width classes do not exceed
        the browser view port.
    * lib/lp/registry/templates/productrelease-portlet-data.pt
      * Replace the invalid markup with valid HTML markup.

« Back to merge proposal