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

Fix pin bugs #1349

Merged
merged 4 commits into from
Jan 14, 2021
Merged

Fix pin bugs #1349

merged 4 commits into from
Jan 14, 2021

Conversation

mason-fish
Copy link
Contributor

fixes #1166

There were a few inconsistencies in the tree view, and this PR aims to fix them with these changes:

  • When clicking on any part of the investigation tree, the value you click on will go to the search bar and any of it's parental lineage will be be used as pins. Previously the underlying query was being used, even if you had only intended to use a fragment of that original query.
  • When deleting an entry from the tree, we use the underlying values as well, but now we inform the user about that behavior.
  • Replaces our own Tree class with the tree-model package we are using in the query lib reducer.

Mason Fish added 3 commits January 11, 2021 12:55
Signed-off-by: Mason Fish <[email protected]>
Signed-off-by: Mason Fish <[email protected]>
@mason-fish mason-fish requested a review from a team January 11, 2021 23:08
@jameskerr
Copy link
Member

This behavior is much more strait forward now. Thanks for swapping in the tree-model too.

I noticed that we provide an "are you sure" popup now in the tree view, but we don't in the linear view. Was that your intent? I think it might be cumbersome to click through that each time I wanted to delete something from the tree. What do you think about changing the label from "Delete" to "Delete underlying entry"? That might provide enough of a warning to the user without making them click through a popup. It would also match the linear view. What do you think?

return res
},
[]
)
Copy link
Member

@jameskerr jameskerr Jan 13, 2021

Choose a reason for hiding this comment

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

I think might be simpler as a map function: node.getPath().map(n => n.model.filter)

.then(({response}) => {
if (response === 0) {
const multiTs = node
.all(() => true)
Copy link
Member

Choose a reason for hiding this comment

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

Is this the same as saying node.children?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned offline: no, all() is similar to a descendants() or function which recursively returns all children, grand-children, etc. It just takes a predicate function which I'm not really utilizing any sort of filter here, hence () => true (return all nodes)

Signed-off-by: Mason Fish <[email protected]>
@mason-fish
Copy link
Contributor Author

@jameskerr I agree the dialog was a bit too much, here is the new behavior:
image

Copy link
Member

@jameskerr jameskerr left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good

@mason-fish mason-fish merged commit 0d54efe into master Jan 14, 2021
@mason-fish mason-fish deleted the 1166-pins_bug branch January 14, 2021 01:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent pins in tree view
2 participants