Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
trvrb opened this issue Dec 10, 2020 · 2 comments · Fixed by #1749
Closed

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

trvrb opened this issue Dec 10, 2020 · 2 comments · Fixed by #1749
Assignees
Labels
bug Something isn't working

Comments

@trvrb
Copy link
Member

trvrb commented Dec 10, 2020

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 trvrb added the bug Something isn't working label Dec 10, 2020
@victorlin victorlin self-assigned this Nov 20, 2021
@victorlin
Copy link
Member

@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):
    this.setState({selectedNode: {}});
    if (selectedNode.type==="tip") {
    /* restore the tip visibility! */
    this.props.dispatch(applyFilter("inactivate", strainSymbol, [selectedNode.node.n.name]));
    }
  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.
    remove={() => {this.props.dispatch(applyFilter("remove", filterName, [item.value]));}}

And respectively,

  1. The external click, through a callback binding, removes the modal via selectedNode on this Tree class:
    class Tree extends React.Component {
  2. The icon click is connected to this Tree reducer:
    const Tree = (state = getDefaultTreeState(), action) => {

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

@jameshadfield
Copy link
Member

jameshadfield commented Nov 23, 2021

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.

@victorlin victorlin moved this from Committed to New in Nextstrain planning (archived) Dec 22, 2021
@victorlin victorlin moved this from Prioritized to Backlog in Nextstrain planning (archived) Feb 16, 2022
jameshadfield added a commit that referenced this issue Mar 16, 2022
This implements what we think is an intuitive UI, and also helps with
the bug described in #1243 where removing/disabling a strain-filter
results in the tree visualisation falling out-of-sync with the modal.

See #1479 for context.

Closes #1479
jameshadfield added a commit that referenced this issue Mar 17, 2022
This implements what we think is an intuitive UI, and also helps with
the bug described in #1243 where removing/disabling a strain-filter
results in the tree visualisation falling out-of-sync with the modal.

Note that `event.key` may not be implemented in all browsers, however
this functionality is a nicety rather than a must-have, so this is
tolerable. Thanks to @joverlee521 for suggesting to use this instead of
`event.keyCode` during PR review.

See #1479 for context.

Closes #1479
jameshadfield added a commit that referenced this issue Feb 5, 2024
Clicking a tip brings up a modal and activates the corresponding strain
filter. Inactivating or removing this filter now also clears the modal.

Closes #1243
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Status: Backlog
Development

Successfully merging a pull request may close this issue.

3 participants