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

Lack of control over delete leads to some weird scenarios #232

Open
kid-icarus opened this issue Mar 5, 2024 · 2 comments
Open

Lack of control over delete leads to some weird scenarios #232

kid-icarus opened this issue Mar 5, 2024 · 2 comments

Comments

@kid-icarus
Copy link

Consider the following scenario:

I have some entities in a store, and deleting those entities from the tree should also delete them from the store. However, I might need to validate that they can be deleted, and if not, display some error notification stating such. If the node can't be deleted, I'd like to not focus on the next sibling.

Screen.Recording.2024-03-05.at.11.03.09.AM.mov

Next, lets say I hit delete on the last node. It wraps around to the root node, that has no siblings, but only children. If I were to hit delete again, all selection is lost, and the focus remains on the root node because the logic cant find the next node to focus on

Screen.Recording.2024-03-05.at.11.11.52.AM.mov

Lastly, it's not immediately obvious to me how I can manage focused state in this scenario: I attempt to delete a "forbidden" node, display a notification, and when the notification disappears a rerender happens. When this happens, the focus state resets to next sibling of the node that I was attempting to delete.

I have a couple thoughts here, wanted to see what if this makes sense to you.

  1. I could create my own tree container, and pass it to the renderContainer. However, the internal components used by the default container aren't exported. This means I'd have to recreate those.
  2. Perhaps we could add a prop, something to the effect of onDeleteOverride, whereby the full behavior of delete could be controlled. We could check for this prop in the default container, and if present, do not run the default backspace event handler. Rather, we'd just invoke whatever was passed in the prop.

Curious if you have any thoughts on a these suggestions or other directions this could go :)

@jameskerr
Copy link
Member

Thanks for taking the time to makes these videos and explain what problems you're facing. This makes sense to me. In the next version I plan to allow custom "commands" to be registered on the tree. It could be used like this... (psudo-code). Then along with shortcuts, it could work something like this. What do you think?

const commands = {
  "deleteIfAllowed": (tree) => {
     const node = tree.focused()
     // if it cannot be deleted do nothing with the focus. 
  }
}

const shortcuts = {
  "Backspace":  "deleteIfAllowed"
}

<TreeView 
  commands={commands}
  shortcuts={shortcuts}
/>

@jameskerr
Copy link
Member

All those event handlers in "defaultContainer" are going to be turned into commands with names that can be overridden. Much like your suggestion number 2.

I'm working on this today actually.

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

No branches or pull requests

2 participants