monarch-initiative / phenogrid

The phenogrid widget
13 stars 14 forks source link

Genotype expansion for release #225

Closed yuanzhou closed 8 years ago

yuanzhou commented 8 years ago

@harryhoch in this branch, several things have been tested again and fixed accordingly:

when you have a chance, please review and help me test. Thanks!

harryhoch commented 8 years ago

@yuanzhou, This looks pretty good, but I think I've found reproducible bug. Try the following

  1. Start the genotype expansion for some gene A
  2. Mouse over a different gene B.

This sequence seems to reliably throw an error. Please talk to me in person if you can't reproduce.

yuanzhou commented 8 years ago

@harryhoch I think it's not an issue of async ajax as I talked to you earlier. Because once I clicked to expand one gene, without clicking to expand another gene, as long as I move my mouse randomly, I could get reproduce that error, and at that time there's only one ajax call. Weird enough. I suspect it's related to DOM or D3. And I think the previous annoying bug was similar to this one, but I didn't discover the cause. I'll dig into this.

harryhoch commented 8 years ago

@yuanzhou , ok, thanks. consider a flag to disable events while waiting for data return...

yuanzhou commented 8 years ago

@harryhoch finally resolved this annoying bug. The cause was easy to fix but hard to trace down. I was using the species name grabbed from the tooltip attribute with jQuery when we expand a gene ID, so I can pass this species name to the dataLoader. And this data-species attribute was added when we create the tooltip box each time. So right after we click the expansion link, an ajax call fires off to get the genotype data, but once we move the mouse onto any other gene labels or data cells/phenotype labels, the tooltip box gets to re-rendered immediately. As a result, the species name became "undefined" by the time we need it in the ajax callback to transform the raw simsearch data into the format that fits Phenogrid's needs. During this debugging process, I also noticed some bugs of Google chrome's debugger, which also confused me for several times.

My fix was to just use the species name from this.state.selectedCompareTargetGroup array since it's being updated before we load any additional data and doesn't require any DOM updates to get this data.

Please review and test, if everything looks good, then I think we can merge this PR. Thanks @harryhoch for noticing this bug.