search

SpiderStrategies / kalpa-tree

A tree implementation backed by D3
http://spiderstrategies.github.io/kalpa-tree/
ISC License
4 stars 2 forks source link

Adds ability to deselect whatever node is selected #454

Closed nathanbowser closed 4 years ago

nathanbowser commented 4 years ago

Simply call tree.select() and it will deselect the current node.

Fixes #453 For https://github.com/SpiderStrategies/Scoreboard/issues/26936

mattsgarlata commented 4 years ago

I'm concerned with multiple calls to select toggling whether a node is selected or not. I am 99% sure there are instances in Scoreboard where we call select multiple times just to ensure a node is selected even though it may have already been selected some other way (e.g. by setting an initialSelection). Would it be possible to have a totally separate call in the API to deselect and have the select functionality remain the same?

nathanbowser commented 4 years ago

@mattsgarlata It won't toggle. What makes you think it will toggle if you call it multiple times?

mattsgarlata commented 4 years ago

Oh, I think we're using the word "toggle" to mean different things. I was thinking about toggle as selecting or deselecting, but I think you use the word toggle to mean expanding or collapsing a node.

Anyway, you wrote

Simply call tree.select() and it will deselect the current node.

I'm saying I think the behavior of tree.select() shouldn't change otherwise I think it will be a breaking change for Scoreboard 3.

nathanbowser commented 4 years ago

I totally get not wanting this to be a breaking change for scoreboard and I agree. But I also don't want to bloat an API that's going to live a few more years (hopefully). Other tree calls clear something when arguments aren't passed. e.g. tree.search(null).

But the good news is I don't believe this is a breaking change. Jerry and I both thought an empty select call cleared the selected node before I created the issue. I searched scoreboard and didn't see a single place where we do a tree.select() without passing a node/id.

In your original writeup when you mentioned the word "toggle". I thought you were thinking it would toggle the select property. e.g.

tree.select(1) // It's selected
tree.select(1) // It's not selected
tree.select(1) // It's selected

That would be a breaking change but it's not doing that, that's why I questioned your original writeup.

mattsgarlata commented 4 years ago

OK cool. Sounds like this will be fine then

nathanbowser commented 4 years ago

Cool, if it breaks something I'll happily revisit.