-
-
Notifications
You must be signed in to change notification settings - Fork 135
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
FEATURE: batch operations on tree nodes #2568
Conversation
I added an issue for this in order to simplify release management and to collect higher-level details for the release notes: #2599 |
@markusguenther and @jonnitto do you guys have time for a review? @dimaip said it's ready for merging. https://neos-project.slack.com/archives/C12EZDXA8/p1575210010002400 |
Right now we have advent celebration with kids and grandma but can check the notebook later :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @dimaip
It is such an awesome improvement. Just had little comments.
I love it ❤️
name: 'Copy node to clipboard' | ||
uriPattern: 'copy-node' | ||
name: 'Copy nodes to clipboard' | ||
uriPattern: 'copy-nodes' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn`t that kind of breaking when we change the end-point URL, even it is makes sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those endpoints are not part of our public API, it's merely an implementation detail to be able to keep the copied nodes between different sites/domains.
Hey @markusguenther, thanks a lot for the thorough review! I think I saw your remark about not being able to delete nodes or something, but I can no longer find that comment... And deleting works just fine for me :) |
Yes in the first testing round I tried to move nodes and then delete them. But after the move the delete button was deactivated. So i thought it is a bug but maybe just the parent was also selected then. Because could not reproduce it anymore. Except you select nodes with their parent node. But guess that it is intended. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again thank you for that huge improvement 👏🏻
Yup, deleting nodes with parents doesn't make much sense and could lead to errors. |
Would be great to get more reviews on this, it's a huge feature after all! |
And yes, if somebody would be merging it without me this PR should definitely be squashed. |
Why? I firmly believe there is a lot of value in being able to see the development of a feature… And it doesn't add confusion, since you can always just look at the merge to see "the full change at once…" |
@kdambekalns you can still see the original commits in the PR history, e.g. https://github.com/neos/neos-ui/pull/2589/commits |
I was rather just interested in the reasoning… for me in this case the ration of useful/useless ("real change" vs "stupid fix") is quite good… :)
But that assumes GH is used, I more often work with the history locally, using my favourite Git client. And there squash commits don't really work well, especially since the original branch shows up as unmerged (really confusing IMHO), and disappears when deleted. Anyway, no blocker for anything, as long as I get my "proper merge" for my stuff, at least. ;) |
I've merged it as is with just one review since I intend to keep on improving this feature (dnd + e2e), and I would be able to fix it future bugfix releases if something wrong is discovered. |
Resolves: #2599
Support multiple-selection on trees via these keyboard shortcuts:
When multiple nodes are selected it's possible to do operations on all of them at once: move them (via dragging or via cut/paste buttons), copy, hide, delete.
TODO:
In the future: