search

monarch-initiative / phenogrid

The phenogrid widget
13 stars 14 forks source link

Tooltiprender refactor #213

Closed yuanzhou closed 8 years ago

yuanzhou commented 8 years ago

@harryhoch as you've suggested to refactor the tooltiprender. During the refactoring this morning, I realized that we could totally shrink the tooltiprender code into one simple method and place it inside phenogrid.js. There's no need to keep a separate tooltiprender class with many internal methods in a separate file. By making this change, we've also eliminated many "passing variables by references". This also led me to cleanup the HTML tags used to render the tooltip as well as the appearance of "phenotype/disease/gene" in the method names.

I feel it's similar to what I realized that what we could do with stickeytooltip, basically one or two methods can achieve their purposes. Less code, more straightforward.

I've tested this as well as the genotype expansion (enabled for testing then disabled again), please review and merge.

harryhoch commented 8 years ago

@yuanzhou, I might want a detailed review, but we can do that later. looks good for now.

yuanzhou commented 8 years ago

Thanks @harryhoch. I liked the idea of separating the tooltiprender component from the main phenogrid originally. After refactoring the stickytooltip and this tooltiprender, I realized that the tooltip actions and its content are just tightly coupled with other phenogrid methods/variables. It makes more sense to move the functionalities back to phenogrid, especially after they were minimized to only a couple of straightforward methods.