The Javascript methods used to manage adding a comment on the expandable row of the diff pages should be refactored into a proper YUI widget.
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
Launchpad itself |
Fix Released
|
Low
|
Raphaël Badin |
Bug Description
This will allow testing for these functions and may as well be a good occasion to refactor that code a bit:
"""
827 + var slide = function(direction) {
828 + // Slide out if direction is true, slide in if direction
829 + // is false, otherwise do the opposite of what's being
830 + // animated right now.
831 + if (slide_anim === null) {
Why the "do the opposite" option? Shouldn't the caller know what the desired end state is? In my experience, toggle functionality in code is almost always a bad idea. It comes with a potential for inconsistent states. I find it's usually better for the calling code to figure out what it wants, and then tell other code unequivocally to go do it. What you want is a policy decision, and how you get it is an implementation decision. Moving the policy decision into the implementation is asking for trouble; allowing the policy decision to be either in the caller or the callee adds an extra layer of confusion.
"""
Plus, of course, in this case:
838 + else {
839 + if (Y.Lang.
840 + slide_anim.
841 + }
842 + else {
843 + slide_anim.
844 + }
This opens the door for pseudo-boolean values (e.g. because a function that answers a boolean question about the DOM may actually return a Node for "true" or null for "false") to trigger a toggle rather than a specific direction. In that situation, this code effectively flips a coin to pick an animation direction. As per Sod's Law, it will do the right thing in testing but the wrong thing in production. This may not be the right branch to ditch the toggle functionality, but I'd at least put up large red flags around it and think about how to kill it.
Related branches
- Gavin Panella (community): Approve
-
Diff: 921 lines (+550/-217)3 files modifiedlib/lp/registry/javascript/distroseriesdifferences_details.js (+199/-181)
lib/lp/registry/javascript/tests/test_distroseriesdifferences_details.html (+1/-0)
lib/lp/registry/javascript/tests/test_distroseriesdifferences_details.js (+350/-36)
tags: | added: javascript ui |
Changed in launchpad: | |
importance: | Undecided → Low |
tags: |
added: qa-untestable removed: qa-needstesting |
Changed in launchpad: | |
status: | Fix Committed → Fix Released |
Fixed in stable r13680 <http:// bazaar. launchpad. net/~launchpad- pqm/launchpad/ stable/ revision/ 13680>.