Skip to content

Add ability to rename nodes #2749

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

Merged
merged 11 commits into from
Apr 7, 2024
Merged

Add ability to rename nodes #2749

merged 11 commits into from
Apr 7, 2024

Conversation

joeyballentine
Copy link
Member

@joeyballentine joeyballentine commented Apr 5, 2024

image

image

I'm not super thrilled with the way i ended up doing this (in regard to the text input being in the menu) but it was the easiest way.

@RunDevelopment
Copy link
Member

I don't think the UX with renaming like that is too bad. There are just a few things that need a bit more polish:

  1. Clicking on Rename, then closing the context menu (e.g. by clicking elsewhere), and then opening the context menu again will still show the text box for the name. Closing the context menu should switch back the button IMO.
  2. Focus and default text. I think then clicking on Rename, the text box should be (1) filled with the current node name (nickname or schema name), (2) focused (so the user can start typing), and (3) have all text selected (so the user can overwrite the current name without having to select all text manually). Basically, I want to behave like renaming a variable in VSCode.
  3. The text box isn't the same height as the Rename button, which awkwardly moves the below elements down 2px. Please make them the same height.

Copy link
Member

@RunDevelopment RunDevelopment left a comment

Choose a reason for hiding this comment

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

Sorry, I didn't mean to approve.

@joeyballentine
Copy link
Member Author

  1. I'm not really sure how to accomplish that. The way that the menu hook is set up makes it really difficult to hook into any part of the life cycle of the menu from the content
  2. I tried this, but it doesn't seem possible. Something about chakras inputs makes .focus()ing on a ref set to the input not work.

@RunDevelopment
Copy link
Member

Okay, I can make the text input be focused and selected, but it's still not good enough. Chakra reaaally messes with focus in ways that I don't understand. Even if the text box is focused, simply hovering over another menu item causes it to lose focus.

I honestly think we need a different input method. I can think of 2 alternatives:

  1. The WYSIWYG approach: double-click on the name of a node in the node header, and it will transform into a text box where you can edit the name.
  2. Same as VSCode: show a little text box near the name of the node where the user can change the name.

Approach 1 is intuitive, but it (1) changes the meaning of double-clicking a node (not a problem IMO) and (2) has the accessibility problem that the size of the text in the text box depends on the zoom level.

Approach 2 has none of these issues, but might be harder to implement, I'm not sure.

What do you think?

@joeyballentine
Copy link
Member Author

Approach 1 is what I initially tried but I had some trouble getting it to work, but I could try again I suppose

@joeyballentine
Copy link
Member Author

image

@RunDevelopment
Copy link
Member

Instead of adding review comments, I made you a PR with what I would have changed. Please test it to see whether you like the behavior. #2752

@joeyballentine joeyballentine merged commit fe722fa into main Apr 7, 2024
@joeyballentine joeyballentine deleted the node-renaming branch April 7, 2024 17:23
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.

2 participants