Merge lp://staging/~gary/launchpad/bug750561 into lp://staging/launchpad

Proposed by Gary Poster
Status: Merged
Approved by: Gary Poster
Approved revision: no longer in the source branch.
Merged at revision: 12800
Proposed branch: lp://staging/~gary/launchpad/bug750561
Merge into: lp://staging/launchpad
Diff against target: 300 lines (+102/-20)
4 files modified
lib/canonical/launchpad/icing/style.css (+11/-0)
lib/lp/registry/javascript/structural-subscription.js (+22/-5)
lib/lp/registry/javascript/tests/test_structural_subscription.html (+2/-5)
lib/lp/registry/javascript/tests/test_structural_subscription.js (+67/-10)
To merge this branch: bzr merge lp://staging/~gary/launchpad/bug750561
Reviewer Review Type Date Requested Status
Henning Eggers (community) Approve
Review via email: mp+57195@code.staging.launchpad.net

Commit message

[r=henninge][bug=750561] add a spinner for when you delete, edit, or add a structural subscription.

Description of the change

This branch adds a spinner when you delete, edit, or add a structural subscription.

It does this with some simple CSS changes. What I've done seems like a pretty good pattern to follow for how to do this, because it seems very simple: just change classes around to spinner while you are waiting for the work to be done.

I added a couple of JS tests for this. They should be relatively straightforward to read. I added the ability of our LPClient stub to halt execution of the callback so you can examine the intermediate state. In order for the test to pass I had to add a dependency for lp.app.errors to work, and then I had to add some code to clean up the overlay that lp.app.errors adds.

While working on the tests, which involved a fair amount of debugging, I became annoyed and distracted by five errors I was seeing in the Chrome error log (but not in the test results). I traced the problem down to the fact that we were reusing the link for every test, which caused the errors as unpleasant side effects. Therefore, I made our setup code set up the link as well. This seemed to work nicely.

I also needed to simulate a click, and I noticed that there were two approaches to simulating a click in our tests: "Y.Event.simulate(Y.Node.getDOMNode(select_none), 'click');" and "select_none.simulate('click');". The second looked wildly nicer and less error-prone (like forgetting to get the DOM node) so I converted all of the code to use it. FF4 newest, Chrome newest and Safari newest all passed with this clean-up.

This is an incremental branch. In order to completely fix this bug, I will need to also add a spinner for the "muting" code in db-devel. I will do that next.

For QA, you should see the "remove" icon turn into a spinner when you click "Unsubscribe" on [structural subscription target]/+subscriptions (when you are in the proper feature-flagged group); and you should see the "submit" button turn into a spinner when you edit or add a subscription on that page or on the structural subscription target overview or bug pages.

Thank you

Gary

To post a comment you must log in.
Revision history for this message
Henning Eggers (henninge) wrote :

Thank you very much. I found this very interesting to read. ;-)

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.