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

FEATURE: batch operations on tree nodes #2568

Merged
merged 25 commits into from
Dec 2, 2019
Merged

FEATURE: batch operations on tree nodes #2568

merged 25 commits into from
Dec 2, 2019

Conversation

dimaip
Copy link
Contributor

@dimaip dimaip commented Oct 17, 2019

Resolves: #2599

Support multiple-selection on trees via these keyboard shortcuts:

  • hold CMD/CTRL and click: select multiple nodes
  • hold SHIFT and click: select a range of nodes (only on one level though)

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.

multi-demo

TODO:

  • hide
  • delete
  • copy
  • move
  • dnd
  • inspector summary
  • single move of unselected nodes
  • don't focus inline on batch content focus clicks
  • refactor selectors
  • tooltip to make the feature discoverable
  • i18n
  • support Shift key for selecting ranges
  • improve DND visualization
  • adjust the tests
  • write e2e tests

In the future:

  • batch editing in inspector

@dimaip dimaip changed the title WIP FEATURE: batch operations on tree nodes FEATURE: batch operations on tree nodes Oct 18, 2019
@bwaidelich
Copy link
Member

I added an issue for this in order to simplify release management and to collect higher-level details for the release notes: #2599

@davidspiola
Copy link
Contributor

davidspiola commented Dec 1, 2019

@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

@markusguenther
Copy link
Member

Right now we have advent celebration with kids and grandma but can check the notebook later :)

Copy link
Member

@markusguenther markusguenther left a 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'
Copy link
Member

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?

Copy link
Contributor Author

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.

packages/neos-ui/src/Containers/Modals/DeleteNode/index.js Outdated Show resolved Hide resolved
@dimaip
Copy link
Contributor Author

dimaip commented Dec 2, 2019

Hey @markusguenther, thanks a lot for the thorough review!
I've fixed I18n stuff.

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 :)

@markusguenther
Copy link
Member

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.

Copy link
Member

@markusguenther markusguenther left a 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 👏🏻

@dimaip
Copy link
Contributor Author

dimaip commented Dec 2, 2019

Yup, deleting nodes with parents doesn't make much sense and could lead to errors.
Theoretically we could automatically filter out all the children and permit delete in such situations, I don't know how much UX value would actually come from this...

@dimaip
Copy link
Contributor Author

dimaip commented Dec 2, 2019

Would be great to get more reviews on this, it's a huge feature after all!
I'd wait with merging till the end of the day, if no reviews come in I'd merge.

@dimaip
Copy link
Contributor Author

dimaip commented Dec 2, 2019

And yes, if somebody would be merging it without me this PR should definitely be squashed.

@kdambekalns
Copy link
Member

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…"

@dimaip
Copy link
Contributor Author

dimaip commented Dec 2, 2019

@kdambekalns you can still see the original commits in the PR history, e.g. https://github.com/neos/neos-ui/pull/2589/commits
Having dozens of commits per each change is really annoying e.g. when doing git-bisect and in general when navigating history.
I'm fine with PRs to be merged if they have nice edited history, but in this particular case with all those "fix linter" commits, I would like them to end up in the codebase. Neither do I care about them enough to actually do git rebase -i...
But that's just my take on it, I'm open for doing things differently.

@kdambekalns
Copy link
Member

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… :)

you can still see the original commits in the PR history

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. ;)

@dimaip dimaip merged commit 48233fe into master Dec 2, 2019
@dimaip dimaip deleted the multi branch December 2, 2019 09:18
@dimaip
Copy link
Contributor Author

dimaip commented Dec 2, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Batch operations on tree nodes
5 participants