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 editable tree item bugs #3187

Merged
merged 10 commits into from
Feb 9, 2024

Conversation

apedroferreira
Copy link
Member

@apedroferreira apedroferreira commented Feb 9, 2024

Fix some bugs seen in editable tree items:

  • In pages explorer, since the items were changed to show page display names instead of node names, the renaming feature was confusing and could even cause pages to be deleted unintentionally. Fix: Changed the renaming to update display name instead of node name.
  • In page hierarchy explorer, somehow clicking near the outside of an item triggered a blur event that cleared the editable input. Fix: I removed a state update to the value of this input that didn't seem like it was doing anything.
  • When deleting a page that is not the active page from the explorer, Toolpad changed to an empty app screen.

Before:

Screen.Recording.2024-02-09.at.01.11.57.mov

After:

Screen.Recording.2024-02-09.at.01.09.45.mov

@apedroferreira apedroferreira added the bug 🐛 Something doesn't work label Feb 9, 2024
@apedroferreira apedroferreira self-assigned this Feb 9, 2024
@apedroferreira apedroferreira requested a review from a team February 9, 2024 01:21
@Janpot
Copy link
Member

Janpot commented Feb 9, 2024

Interesting. It feels a bit counterintuitive to me. Like, you have a page "users". And you rename it to "employees". Now you have "pages/users/page.yml" in the file system and "employees" in the editor file tree. Maybe we need to change the editor file tree back to use page name? And then show the display name in a tooltip?

@apedroferreira
Copy link
Member Author

apedroferreira commented Feb 9, 2024

Interesting. It feels a bit counterintuitive to me. :Like, you have a page "users". And you rename it to "employees". Now you have "pages/users/page.yml" in the file system and "employees" in the editor file tree. Maybe we need to change the editor file tree back to use page name? And then show the display name in a tooltip?

Yeah originally when we added display names I thought the pages explorer would stay the same.
Let's change it back to showing the file names then but still show the display names somehow, a tooltip might work!

@Janpot
Copy link
Member

Janpot commented Feb 9, 2024

Yeah originally when we added display names I thought the pages explorer would stay the same.

👍 I changed that, didn't give it enough thought

a tooltip might work

👍 another alternative is pageName (Display Name). But maybe that would create too long strings.

@apedroferreira

This comment was marked as outdated.

@apedroferreira
Copy link
Member Author

Ok, looks like a Tooltip does the job, just not as immediately visible but I think that's fine?

Screen.Recording.2024-02-09.at.16.53.05.mov

@bharatkashyap
Copy link
Member

Ok, looks like a Tooltip does the job, just not as immediately visible but I think that's fine?

Screen.Recording.2024-02-09.at.16.53.05.mov

This feels like the best option to me 👍

@apedroferreira apedroferreira merged commit 1c6a90d into mui:master Feb 9, 2024
11 checks passed
@apedroferreira apedroferreira deleted the fix-editable-tree-item-bugs branch February 9, 2024 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants