statgen / locuszoom

A Javascript/d3 embeddable plugin for interactively visualizing statistical genetic data from customizable sources.
https://statgen.github.io/locuszoom/
MIT License
156 stars 29 forks source link

Fix Interval Tool Tip Behaviors #116

Closed Frencil closed 7 years ago

Frencil commented 7 years ago

The Problem(s)

As @welchr documented in #106 tool tips were not working for interval annotation tracks in merge-tracks mode (Referred to in the solution as Issue A).

In addition, I noticed that while tool tips always worked in split-tracks mode, the bounding box to highlight the track wasn't appearing when a plot was first loaded in split-tracks mode but would appear after merging and re-splitting tracks (Referred to in the solution as Issue B).

This Solution

Edited to reflect current approach

This branch addresses both issues by refactoring the notion of how a data layer may have separate status nodes to class when applying a status (like highlighted or selected) in addition to classing the node most closely bound to the element by ID.

We see this on intervals as well as genes, where what we're rendering for data is not necessarily what will provide a visual cue that a status is applied (as opposed to a simpler scatter plot element which shows status with a CSS class change alone). Both of these data layers have "bounding box" nodes that render behind data and can even have a layout-defineable padding.

To abstract this behavior including showing status with a "shared" node (e.g. highlight the whole track for intervals but only when splitting tracks) this refactor introduces the concept of status nodes as described above and relies on a new method DataLayer.getElementStatusNodeId() to select the proper element to apply the status to. This method is abstracted for all data layers and will just return null, skipping any classing of status nodes that probably don't exist. But genes and intervals override this function:

These changes eliminate two undocumented layout directives: hover_element and group_hover_elements_on_field (the former appeared in genes and intervals and the latter only appeared in intervals). These were "dynamic" layout directives that wouldn't make sense for a user to define up front anyway, so it's arguably better that this can all be done more abstractly now. The main point of group_hover_elements_on_field was actually a way of saying "use the track split field as defined in the layout" but at the abstract data layer level... now the interval layer can express that in terms of the getElementStatusNodeId() override instead.

Frencil commented 7 years ago

@abought see updates and new PR description. I've refactored things to be a bit more abstract as the behavior here was also partially present in the genes layer. This approach, while going beyond the original scope of the bug, provides a more general framework for data layers using a single shared element to show highlight or selection for a group of individual elements - something we may find ourselves needing to do again in the future.

welchr commented 7 years ago

Looks good to me now, thanks for the fix! 👍

abought commented 7 years ago

I've refactored things to be a bit more abstract as the behavior here was also partially present in the genes layer.

That is a remarkably robust answer to the question of whether it's a general mechanism; thank you!

I'll work on reviewing this now. The remaining wishlist item- better documenting layout behaviors- can be handled at any later time, independent of this being merged.

abought commented 7 years ago

Really this section should be expanded to include the basic anatomy of a data layer subclass (including conventions for defining styles in general as well as what style patterns can map to automated behaviors), along with strategies for both developing a new data layer type from scratch as well as expanding an existing one.

I agree about expanding scope- though I'm not sure I'd be comfortable grouping both user and developer facing docs in the same section. Both are extremely valuable, but serve a different audience. (By proportion, most readers of that section will still likely be people who want to know what parameters to use in their layout JSON)

Glad to hear the manhattan layer is progressing! I will be turning some attention to credible sets display (and better color control customization) in the near future; I'll let you know if I make major overlapping changes to docs on my end.