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

Sortable menus #1758

Merged
merged 14 commits into from
Mar 25, 2020
Merged

Sortable menus #1758

merged 14 commits into from
Mar 25, 2020

Conversation

mamhoff
Copy link
Contributor

@mamhoff mamhoff commented Mar 22, 2020

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

Peek 2020-03-22 19-52

Checklist

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have added tests to cover this change

Copy link
Member

@tvdeyen tvdeyen left a 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?

app/assets/javascripts/alchemy/alchemy.node_sorter.js Outdated Show resolved Hide resolved
app/assets/javascripts/alchemy/alchemy.node_sorter.js Outdated Show resolved Hide resolved
@mamhoff mamhoff force-pushed the sortable-menus branch 2 times, most recently from c5d93f4 to 71e949b Compare March 22, 2020 21:30
@tvdeyen tvdeyen mentioned this pull request Mar 22, 2020
@tvdeyen
Copy link
Member

tvdeyen commented Mar 22, 2020

@mamhoff Now that #1756 has been merged, this needs a rebase

mamhoff added 3 commits March 23, 2020 10:54
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.
Copy link
Member

@tvdeyen tvdeyen left a 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?

Copy link
Member

@tvdeyen tvdeyen left a 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.

app/assets/javascripts/alchemy/alchemy.node_tree.js Outdated Show resolved Hide resolved
app/assets/javascripts/alchemy/alchemy.node_tree.js Outdated Show resolved Hide resolved
app/views/alchemy/admin/nodes/index.html.erb Show resolved Hide resolved
app/views/alchemy/admin/nodes/index.html.erb Show resolved Hide resolved
app/assets/stylesheets/alchemy/nodes.scss Show resolved Hide resolved
app/assets/javascripts/alchemy/alchemy.node_tree.js Outdated Show resolved Hide resolved
app/assets/javascripts/alchemy/alchemy.node_tree.js Outdated Show resolved Hide resolved
app/assets/javascripts/alchemy/alchemy.node_tree.js Outdated Show resolved Hide resolved
app/assets/javascripts/alchemy/alchemy.node_tree.js Outdated Show resolved Hide resolved
app/assets/javascripts/alchemy/alchemy.node_tree.js Outdated Show resolved Hide resolved
Copy link
Member

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

app/assets/javascripts/alchemy/alchemy.node_tree.js Outdated Show resolved Hide resolved
app/assets/javascripts/alchemy/alchemy.node_tree.js Outdated Show resolved Hide resolved
app/controllers/alchemy/api/nodes_controller.rb Outdated Show resolved Hide resolved
spec/requests/alchemy/api/nodes_controller_spec.rb Outdated Show resolved Hide resolved
mamhoff added 5 commits March 25, 2020 09:48
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.
mamhoff added 2 commits March 25, 2020 11:04
Now it also returns a bit of JSON about the toggled node.
Copy link
Member

@tvdeyen tvdeyen left a 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 🍰

@tvdeyen tvdeyen merged commit b7fce5c into AlchemyCMS:master Mar 25, 2020
tvdeyen added a commit that referenced this pull request Mar 30, 2020
tvdeyen added a commit that referenced this pull request Mar 30, 2020
- 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))
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