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

Teach #move_to_child_with_index to accept :root as a target #481

Merged

Conversation

botandrose
Copy link
Contributor

Overview:

This PR enhances the #move_to_child_with_index method to accept :root as a valid parent:

categories(:child_1).move_to_child_with_index(:root, 1)

Motivation:

The motivation was pairing ANS with a JavaScript drag-and-drop tree reordering library, and needing to persist the moves made to the database. The #move_to_child_with_index API was perfect for this, except it couldn't handle moves made to the top-level. This has been noted before in #446

Questions:

Does this look like a reasonable addition to ANS? Is this the right API design? Would passing nil instead of :root be more consistent with the rest of the API? Or perhaps it should be a new method entirely, like #447 ?

Future work:

If this is the right API design, I'd like to refactor out the duplication in #move_to_child_with_index. Also, I'd like to add a few more tests.

What do you think?

@botandrose
Copy link
Contributor Author

botandrose commented Feb 26, 2024

Looks like CI is broken on master, cuz rails-main depends on Ruby 3.x now. Fixed in #482

@botandrose
Copy link
Contributor Author

@parndt ok, green after rebasing on master

@parndt
Copy link
Collaborator

parndt commented Feb 28, 2024

@botandrose given we already have this code move_to self, :root which special cases the term :root I'm happy with this API design too, so feel free to refactor this please in this PR if you're willing to put in the time 😄

@botandrose
Copy link
Contributor Author

@parndt I started the refactoring today, and its becoming clear to me that in order to remove the duplication, I'll need to introduce two more API additions:

  1. Add ANS::Model::Relatable#roots, for getting a relation of the root nodes of the tree of the current node. This seems like an obvious hole in the API, and there is a class-method equivalent, so I don't expect this to be particularly controversial.
  2. Teach ANS::Model::Movable#move_to_child_of(node) to also accept :root as an argument. Given this PR's validity, this seems like it should also work too, and if it does, #move_to_child_with_index can use it internally to remove a lot of duplication.

My question for you is whether or not I should open separate PRs for these two items to evaluate and discuss one-by-one, or if you'd prefer this whole endeavour to live in this one PR as a coherent package?

@parndt
Copy link
Collaborator

parndt commented Feb 29, 2024

@botandrose happy for it to be one change if you are 😄 then it provides the whole context for anyone looking later. IF you'd prefer to do it separately, I'm fine with that too.

@botandrose
Copy link
Contributor Author

@parndt Okay, I'm pretty satisfied with this, now. Do you think it needs anything more before its good to merge?

Copy link
Collaborator

@parndt parndt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work on this!

@parndt parndt merged commit e2f88c1 into collectiveidea:master Mar 8, 2024
59 checks passed
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.

3 participants