-
-
Notifications
You must be signed in to change notification settings - Fork 315
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
Sortable menus #1758
Sortable menus #1758
Conversation
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.
Very nice! This is a huge improvement with very little code. I have some comments though and unfortunately the node folding is now broken. Mind take another look?
c5d93f4
to
71e949b
Compare
This is the simplest serializer for a node.
This moves an node to a specified parent and a specified position.
This is a nice sorting library that works with trees.
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.
@mamhoff do you mind to make these two little changes?
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.
Thanks. There are some more things to consider, though.
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.
Very nice extraction of the utils. That is very helpful. There are still some issue to tackle. But we are very close. Thanks for all the work 👏
This commit allows sorting of menus. We instantiate for each element in the tree one instance of Sortable.js, but - through the `group` option - allow dragging and dropping between each element. For this to work, all `ul` elements need to be present (otherwise we cannot drop a node below a list that does not have nodes yet). A little tricky was the correct behaviour of the `folded` button: It should only be displayed if there are any pages to fold. This is now accomplished through rendering the `+` and `-` buttons in Javascript, depending on how many elements are in each list after sorting. Additionally: The full node tree is now rendered on page load, and all manipulations manipulate the DOM. There are no nodes being inserted or deleted through sorting or folding.
I had to reimplement jQuery.on for this to work.
We'll need to use this elsewhere.
Now it also returns a bit of JSON about the toggled node.
To make the dragging functionality more obvious.
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.
Thank you very much. This is a great improvement 🍰
- Sortable menus [#1758](#1758) ([mamhoff](https://github.com/mamhoff)) - Programmatic menus [#1756](#1756) ([mamhoff](https://github.com/mamhoff)) - Fix syntax in non-erb Menu templates [#1751]((#1751)) ([Alexander ADAM](https://github.com/alexanderadam)) - Fix `render_menu` for custom controllers [#1746]((#1746)) ([Alexander ADAM](https://github.com/alexanderadam))
What is this pull request for?
Allows sorting of menus :)
Notable changes (remove if none)
Adds a dependency to Sortable.js. I THINK this might actually work for the page tree as well, and the chosen solution is probably way faster than the solution chosen for the page tree. We only update one element, really.
Screenshots
Checklist