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

Improve group expansion handling in the tree view #4897

Merged
merged 8 commits into from
Oct 27, 2020

Conversation

daniel-wer
Copy link
Member

@daniel-wer daniel-wer commented Oct 26, 2020

  • This is a proposal to somewhat improve the group expansion handling in the tree view. In my opinion the current state which auto-expands all groups when loading the tracing is not ideal. When working with larger tracings, the immediate first step after opening the tracing is always to collapse all groups to be able to get an overview of what is contained in that tracing. This "annoyance" has become even more prominent with the recent change that doesn't render the tree tab when it's not visible, because when switching to another tab and back to the tree tab all groups are expanded again. Therefore, I switched the default to be that all groups but the root group are collapsed when opening the tracing.
    All parent groups of the active tree or group are expanded, nevertheless, which doesn't change with this PR.
  • I've also fixed a bug where the auto-expansion of the parent groups of the active tree/group was not properly persisted in the state. This bug led to very annoying errors where a group was suddenly collapsed when a single tree was made visible/invisible (see steps to test).
  • Also add an indeterminate state to groups to indicate if some but not all trees/groups inside of it are visible.
  • Do not show the visibility checkbox for groups that don't contain a tree

This PR doesn't persist the expansion state across reloads - this is another issue.

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • Test that the group expansion state of auto-expanded groups is persisted correctly:
    • Open a tracing which has multiple trees inside a group.
    • Collapse all groups but the root group.
    • Activate another tree by shift-clicking it in the tracing view (the parent groups of that tree should be expanded).
    • Change the visibility of another tree within the same group.
      -> The group expansion state should not change (on master the group was collapsed now).
  • When opening a tracing, only the parent groups of the active tree/group should be expanded.
  • Check that groups where not all but some children are visible show up as indeterminate.

Issues:


…roup expansion state, where groups were missing which led to strange behavior
@daniel-wer daniel-wer self-assigned this Oct 26, 2020
Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Works very well for me 👍 If it's easy, it would be great if this PR could also take care of marking empty groups as indeterminate :)

This "annoyance" has become even more prominent with the recent change that doesn't render the tree tab when it's not visible, because when switching to another tab and back to the tree tab all groups are expanded again.

Oh, I wasn't aware of this downside. I would imagine that this can be very annoying in the long run. I see multiple options for this:

  • only avoid rendering of the tree tab if there are lots of trees (since the impact won't be large otherwise)
  • remember the collapse/expanded information somewhere else (e.g., in the store)
  • keep the tree hierarchy view alive (i.e., move the dom visibility observer deeper down the rendering tree) while still avoiding doing react's reconcilation & co

@philippotto
Copy link
Member

remember the collapse/expanded information somewhere else (e.g., in the store)

might also be necessary, if we want to persist that information across reloads

@bulldozer-boy bulldozer-boy bot merged commit f6408df into master Oct 27, 2020
@bulldozer-boy bulldozer-boy bot deleted the improve-tree-view-expansion-handling branch October 27, 2020 16:26
@daniel-wer daniel-wer mentioned this pull request Oct 27, 2020
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow to toggle empty tree groups Half-selected group shows as empty
2 participants