Merge lp:~danilo/launchpad/drop-collapsibles into lp:launchpad

Proposed by Данило Шеган
Status: Merged
Approved by: Данило Шеган
Approved revision: no longer in the source branch.
Merged at revision: 13422
Proposed branch: lp:~danilo/launchpad/drop-collapsibles
Merge into: lp:launchpad
Prerequisite: lp:~danilo/launchpad/replace-expanders-1
Diff against target: 0 lines
To merge this branch: bzr merge lp:~danilo/launchpad/drop-collapsibles
Reviewer Review Type Date Requested Status
j.c.sackett (community) Approve
Review via email: mp+67224@code.launchpad.net

Commit message

[r=jcsackett][bug=807089] Replace global collapsibles implementation with the one based on two lines of new Expander widget code.

Description of the change

= Bug 807089: replace collapsibles implementation =

Among many different collapsible/expandable/foldable animations, "activate_collapsibles" sticks through. It is called from base-layout-macros (so, for each and every page), and ideally wouldn't be (iow, we'd only set collapsibles up when really appropriate).

However, this branch only changes the implementation in lib/lp/app/javascript/lp.js to use the new generic Expander widget which animates much more nicely, thus removing all of toggle_collapsible() and most of activate_collapsibles() code, replacing it with two lines of code. Test files are another kills.

In all the places where collapsibles is used, the API is only slightly changed: 'collapsed' CSS class is not used anymore to make it appear collapsed initially (because that's the default). To not trigger the hiding from JS, no matter how fast, we instead introduce 'unseen' flag (as used by Expander itself) on appropriate content nodes.

In a few places, we also wrap <table> inside a <div> so it would animate nicely (<table> itself is not a simple display:block element, so 'height' setting doesn't work at all times). In those cases, I usually haven't indented the inner table because that would increase the diff size and I am already close to the limit. I'd be happy to do that after review, though (but please remind me :).

The most complex change is in the pillar-involvement-portlet.pt: we move the initial state from containing node to the content node, as expected by the Expander widget.

I haven't fixed the lp.js lint issues and I'd rather not (it's part of LPClient implementation, and fixes there should be a separate branch).

== Tests ==

Whatever is already there, mostly just QA that it all works. Tested in Epiphany (pure WebKit-based), Chromium and Firefox 5.

== Demo and Q/A ==

1. https://bugs.launchpad.dev/redfish/+filebug
Search for 'a' then play with "Extra options" expander: note how it works much better then the matching bugs expanders when clicked quickly in succession.

2. https://code.launchpad.dev/~name12/landscape/feature-x/+register-merge ("Extra options")

3. https://launchpad.dev/firefox
   "Configuration options" in a portlet on the right-hand side.

   https://launchpad.dev/evolution
   Configure all and reload to make sure it starts hidden because everything is set-up.

4. https://launchpad.dev/firefox/+download ("Release information")

5. https://launchpad.dev/firefox/trunk/0.9.2 (Edit change log, then come back to this page)

6. I need help to construct sample data for sourcepackagerecipe-related-branches.pt change.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/app/javascript/expander.js
  lib/lp/app/javascript/lp.js
  lib/lp/bugs/templates/bugtarget-macros-filebug.pt
  lib/lp/code/templates/branch-register-merge.pt
  lib/lp/code/templates/sourcepackagerecipe-related-branches.pt
  lib/lp/registry/templates/pillar-involvement-portlet.pt
  lib/lp/registry/templates/product-files.pt
  lib/lp/registry/templates/productrelease-portlet-data.pt

./lib/lp/app/javascript/lp.js
      52: Expected '!==' and instead saw '!='.
      91: Expected '===' and instead saw '=='.
     106: Expected '===' and instead saw '=='.
     107: Expected '===' and instead saw '=='.
     139: Move 'var' declarations to the top of the function.
     139: Stopping. (57% scanned).
      -1: JSLINT had a fatal error.

To post a comment you must log in.
Revision history for this message
j.c.sackett (jcsackett) wrote :

Danilo--

This looks good. I'm impressed with how much cleaner this implementation is.

review: Approve

Preview Diff

Empty