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

Improving the selection of the next active tree after tree deletion #4370

Merged
merged 10 commits into from
Dec 17, 2019

Conversation

MichaelBuessemeyer
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented Dec 10, 2019

This PR improves the selection for the next active tree after deleting a tree.
For further information: Link to Discuss

URL of deployed dev instance (used for testing):

Steps to test:

  • open a skeleton tracing
  • create a few trees (can be empty)
  • After the deletion of a tree in the middle of the tree list, the next active tree should be the tree above the deleted tree.
  • After the deletion of the first tree in the tree list, the next active tree should the first in the list.
  • After the deletion the last tree in the tree list, the next active tree should be the tree last tree in the list.

Issues:


@MichaelBuessemeyer
Copy link
Contributor Author

@daniel-wer Could you please review this PR?

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Nice, worked well in my tests 👍

One thing I noticed is that this method doesn't work if users sorted trees by name (and technically it's not even correct if trees are sorted by timestamp, which is the default, because the code is looking at ids [but id and timestamp order mostly corresponds]). I also see, however, that choosing the correct tree in these circumstances would be harder since the sorting state is most likely component state. Do you have an opinion on this @philippotto?

Co-Authored-By: Daniel <[email protected]>
@philippotto
Copy link
Member

One thing I noticed is that this method doesn't work if users sorted trees by name (and technically it's not even correct if trees are sorted by timestamp, which is the default, because the code is looking at ids [but id and timestamp order mostly corresponds]). I also see, however, that choosing the correct tree in these circumstances would be harder since the sorting state is most likely component state. Do you have an opinion on this @philippotto?

I'd say, taking sorting by name into account would be nice to have, but given that the prior behavior was tolerated for so long, this PR should be a sufficient improvement. Since id and timestamp order corresponds, the major use case should be covered by this.

However, groups are probably disturbing the behavior, too, right? In that case, the global sorting is neither by id nor by name. But as I said: this PR should be an improvement, nevertheless, and we have other issues with higher priority in my opinion :)

@@ -230,6 +230,7 @@ class DatasetTable extends React.PureComponent<Props, State> {
/>
<Column
title="Data Layers"
key="dataLayers"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just noticed that react was complaining because the key prop was missing. I just added it here, as an own PR for this change would be overkill.

@MichaelBuessemeyer
Copy link
Contributor Author

MichaelBuessemeyer commented Dec 17, 2019

@daniel-wer Could you please check my newest changes?

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@MichaelBuessemeyer MichaelBuessemeyer merged commit 2181b11 into master Dec 17, 2019
@philippotto philippotto deleted the better-active-tree-after-tree-deletion branch June 14, 2022 11:36
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.

Activate next tree in list after tree deletion
3 participants