FinalsClub / karmaworld

KarmaNotes.org v3.0
GNU Affero General Public License v3.0
7 stars 6 forks source link

wysihtml init.js called on load unnecessarily #410

Closed btbonval closed 9 years ago

btbonval commented 9 years ago

init.js is called on page load, generating a new editor.

init.js is called twice in a row when clicking on Edit this Note (every time the user clicks on it).

That means before the GUI ever renders the first time, init.js has created 3 wysihtml5.Editors.

I know @yourcelf said it needed to be reinitialized on each click of Edit this Note (although I have forgotten why), but I feel like this number of extraneous calls is not intended.

Additionally, inside init.js is an empty onLoad and an onChange which outputs debug information; perhaps I haven't seen the right notes but the debug output is always EDITOR: undefined and ELEMENT: undefined. I'm not sure this onChange is needed to set undefined?

yourcelf commented 9 years ago

Sure; the first run on load is not needed.

The only hard requirement is: it needs to run once after the modal has opened. I found it simpler to "run on open, tear down on close" then to try to implement something stateful, but if something stateful seems better that's fine.

btbonval commented 9 years ago

Thanks for the reminder, @yourcelf .

I put a breakpoint on line 2 of init.js. https://github.com/FinalsClub/karmaworld/blob/note-editing/karmaworld/apps/wysihtml5/static/wysihtml5/init.js#L2

It fires on page load, so we should remove that.

However, it fires twice before the modal loads, then the modal is visible and it doesn't fire again until I've closed the modal and click Edit this Note again.

btbonval commented 9 years ago

I do see a for loop which calls initWysihtml5, but this is probably the single page load call: https://github.com/FinalsClub/karmaworld/blob/note-editing/karmaworld/apps/wysihtml5/static/wysihtml5/init.js#L19-L22

Not sure why 2 calls are being made on click.

yourcelf commented 9 years ago

The on('load', ...) should be removed. The on('change', ...) may not be needed -- I think I was debugging something where the value of the editor didn't seem to get persisted back to the underlying textarea; it may be effectively no-op.

If it is, we don't need wysihtml's init.js at all in notes, because we're doing our own initing in response to the foundation modal.

I'd check to see whether foundation's "open" event is getting fired multiple times. Check https://github.com/FinalsClub/karmaworld/blob/note-editing/karmaworld/templates/notes/note_base.html#L83

btbonval commented 9 years ago

That line does not trigger on load (not that we expected it to).

It only seems to trigger once after clicking Edit this Note, and when I click continue, the modal pops up.

Oh you know what I think the problem is, when I stepped through the code, I noticed it went back to line 2 after evaluating the parameters it needed to pass in. It's probably not being called twice, but rather triggering that particular line twice as an artifact of processing parameters.

So we're probably safe to remove the on-load call to close out this ticket.