auspice icon indicating copy to clipboard operation
auspice copied to clipboard

Deselecting a single strain doesn't remove the info panel overlay

Open trvrb opened this issue 3 years ago • 2 comments

Current Behavior

With new filters behavior (implemented in #1200) clicking on a single tip in the tree adds this single tip as a "filter" and covers tree with gray info panel overlay, like so:

tip-selected

This behavior is as intended. Once in this state you can click outside the overlay panel and both the overlay panel and the strain filter will be dismissed. This is also as intended.

However, if you click on the trash can icon to remove the filter you're left with the full tree displayed but the overlay panel remains. It takes a second click outside the panel to dismiss it.

dismissed

Expected behavior

Instead of this behavior, clicking the trash can icon should remove both strain filter and the overlay panel.

trvrb avatar Dec 10 '20 21:12 trvrb

@jameshadfield I started looking into this. I'm a bit confused about the different trees.

This is what I've gathered:

  1. Clicking outside the overlay panel restores the view by removing the modal (L149) and inactivating the filter (L152): https://github.com/nextstrain/auspice/blob/d8b3d2ae700978a8ef6ebb817b52e994936b1a09/src/components/tree/reactD3Interface/callbacks.js#L149-L153
  2. Clicking the trash can icon only removes the filter using the same applyFilter function. It doesn't reset selectedNode which is the reason for this bug. https://github.com/nextstrain/auspice/blob/d8b3d2ae700978a8ef6ebb817b52e994936b1a09/src/components/info/filtersSummary.js#L54

And respectively,

  1. The external click, through a callback binding, removes the modal via selectedNode on this Tree class: https://github.com/nextstrain/auspice/blob/d8b3d2ae700978a8ef6ebb817b52e994936b1a09/src/components/tree/tree.js#L23
  2. The icon click is connected to this Tree reducer: https://github.com/nextstrain/auspice/blob/d8b3d2ae700978a8ef6ebb817b52e994936b1a09/src/reducers/tree.js#L32

I can't figure out how to get to the Tree class from the Tree reducer. Are they even related?

victorlin avatar Nov 20 '21 06:11 victorlin

Notes after a call between Jover / Victor / Jennifer earlier this week

  1. The piece of redux state tree.selectedStrain used to encode the selected strain (!) and we had code within the Tree component to watch for changes here and update the modal appropriately. Despite references to this redux state remaining within middleware, components and reducers, this is no longer set by any action (i.e. it is always undefined).

  2. If one filters to a strain via the sidebar filtering UI, then the modal shouldn't pop up (no change from current behavior). I think it makes sense in the current design to only show the modal when clicking on the tree tip. Actions which should remove the modal: clicking outside it, clicking inactivate / remove on the filter badge. (Other ideas welcomed here.)

  3. Point 2 implies that we wish to store a piece of redux state representing "strain modal showing" or similar and that we can't use filter state as it currently stands to determine whether a modal should be shown. (One could introduce a more complicated structure into the strain filter state, but I think the concepts are separate enough to warrant different pieces of state.) We could reuse reduxState.tree.selectedStrain if we want, but we don't have to.

  4. Currently all information about which parts of the tree are hovered / clicked, and therefore if a modal is showing, is represented by the Tree component's state.selectedNode. This is an object with three properties: node {obj}, type {str}, event {str}. To elaborate, the <NodeClickedPanel> is always part of the render cycle of <Tree>, but in most cases simply returns null (and therefore no DOM elements are drawn) if state.selectedNode.event!=="click".

  5. We could replace the tip-click events currently encoded by (4) with actions to change redux state (3), with the redux state being the one source of truth for which strain is showing in a modal. Alternatively we could use a redux boolean indicating whether to show a tip modal or not, but this raises issues of keeping redux state and tree component state in sync.

  6. As a bonus, the deselecting of a tip modal by clicking outside the tree would ideally not affect the filter state. This should be easy no matter which decision (5) we choose.

  7. One day we may choose a different UI to a modal (separate to this issue). Having the selected modal strain in redux will help with this work.

jameshadfield avatar Nov 23 '21 21:11 jameshadfield