monarch-initiative / phenogrid

The phenogrid widget
13 stars 14 forks source link

Tooltip refactor #201

Closed yuanzhou closed 8 years ago

yuanzhou commented 8 years ago
  1. Deleted the stickeytooltip.js and added necessary functions in phenogrid.js. Right now we only need to use use _showTooltip() and _hideTooltip() to turn on/off the tooltip.
  2. Also simplified the creation of tooltip by using #pg_tooltip, got rid of the outer div. This also simplified the CSS styles.
  3. Appended pg_tooltip to pg_container instead of the body element, so far the tooltip, unmatched, and control options are only HTML sections in addition to the SVG.
  4. Added _relinkTooltip(), it's only needed for rebinding the tooltip to SVG lables and grid cells when the SVG elements are created or when they are updated. This hides the tooltip when we mouseout the SVG labels and grid cells. Otherwise the tooltip will stay there if we don't move over it.

@harryhoch I'm very happy with this tooltip cleanup, please merge this PR if everything looks good to you. Basically the only effective way to move mouse over to tooltip before it's gone is to create overlap of tooltip div and label/cell. And we have to be ensure the cursor is still on the label/cell on the way to that tooltip.

harryhoch commented 8 years ago

will review tonight.

On 5 Oct 2015, at 4:43 PM, yuanzhou notifications@github.com wrote:

• Deleted the stickeytooltip.js and added nucessary functions in phenogrid.js. Right now we use use _showTooltip() and _hideTooltip() to control the tooltip.

• Also simplified the creation of tooltip by using #pg_tooltip, got rid of the outer div. This also simplified the CSS styles.

• Appended pg_tooltip to pg_container instead of the body element, so far the tooltip, unmatched, and control options are only HTML sections in addition to the SVG.

• Confirmed the use of _relinkTooltip(), it's only needed for rebinding the tooltip to SVG lables and grid cells when the SVG elements are created or when they are updated. This hides the tooltip when we mouseout the SVG labels and grid cells.

@harryhoch I'm very happy with this tooltip cleanup, please merge this PR if everything looks good to you. Basically the only effective way to move mouse over to tooltip before it's gone is to create overlap of tooltip div and label/cell.

You can view, comment on, or merge this pull request online at:

https://github.com/monarch-initiative/phenogrid/pull/201

Commit Summary

• Refining tooltip • Refactored tooltip File Changes

• M css/phenogrid.css (14) • M dist/phenogrid-bundle.css (4) • M dist/phenogrid-bundle.js (34) • M js/phenogrid.js (153) • D js/stickytooltip.js (85) Patch Links:

https://github.com/monarch-initiative/phenogrid/pull/201.patchhttps://github.com/monarch-initiative/phenogrid/pull/201.diff — Reply to this email directly or view it on GitHub.


Harry Hochheiser University of Pittsburgh Department of Biomedical Informatics harryh@pitt.edu 412 648 9300

yuanzhou commented 8 years ago

Glad the Travis CI check found the behave test failed due to the removal of mystickytooltip. Has fixed. Before this PR, the mystickytooltip div was appended to the end of the body tag, so when we see this div id, all the content should have been loaded. I've removed this check since I've moved the pg_tooltip div to inside of pg_container, that behave test no longer applies.

yuanzhou commented 8 years ago

Things to do after PR is merged:

  1. I'll go to master branch update the npm package version to 1.1.2 using npm version 1.1.2
  2. Push to npmjs.org
  3. Update the new version number in monarch-app
harryhoch commented 8 years ago

is this ready to be merged?

yuanzhou commented 8 years ago

Yes, it's ready. Thanks!

harryhoch commented 8 years ago

ok. will merge tonight On Oct 5, 2015, at 9:33 PM, yuanzhou notifications@github.com<mailto:notifications@github.com> wrote:

Yes, it's ready. Thanks!

— Reply to this email directly or view it on GitHubhttps://github.com/monarch-initiative/phenogrid/pull/201#issuecomment-145718794.


Harry Hochheiser University of Pittsburgh Department of Biomedical Informatics harryh@pitt.edumailto:harryh@pitt.edu 412 648 9300

harryhoch commented 8 years ago

@yuanzhou, please delete the branch when convenient.

also, a request - can you look into expanding the region around the tooltip so that we must be more than x pixels out before it deactivates? would that work?

-harry

yuanzhou commented 8 years ago

@harryhoch I don't think I fully understand your request. In the attached screenshot, if I move my mouse along the phenotype label, there should be no problem to move to the tooltip, but if at any point I move cursor out the phenotype label, the tooltip will disappear. Did you mean to create some white-space gap between the tooltip and the label/cell so they don't overlap visually, but still be able to move mouse from label/cell to tooltip? More precisely, if the mouse is over that gap, the tooltip should still stay there? I think an invisible container will do the trick.

capture