Code review comment for lp:~jcsackett/launchpad/hidden-project-configuration-636000

Revision history for this message
j.c.sackett (jcsackett) wrote :

> The tech-debt bug: you copy a big chunk of code into activate_collapsible_div. We talked about this on IRC and you explained that you'd tried refactoring, but changing the code to work on <div> instead of <fieldset> made it a huge pain. We agreed that you'd file a tech-debt bug and put an XXX in the code.

So, in the clarity of a new day I realized that instead of all the complicated "what sort of node am I looking at" logic I was getting into I could just double check for a span with id=legend; throw in some of the structural changes I made to the HTML and the rest of the javascript nastiness was removed.

So now it's a few line change to the regular activate_collapsible function in lp.app.javascript.lp.js

I think everyone can agree that's a good bit better.

> The question: I don't know if it makes sense to use waitForPageLoad when the page has already loaded. Is there a better way to wait for some Javascript to run? If so, please change this.

Turns out forElement can take classname selectors, not just element types. So that's the right kind of wait and the code has been updated.

« Back to merge proposal