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

Add namespace for Tree related routes #2037

Merged
merged 4 commits into from
Mar 2, 2021

Conversation

dhiraj14
Copy link
Contributor

@dhiraj14 dhiraj14 commented Feb 23, 2021

What is this pull request for?

NodeTree is being used for rendering the Menu in tree
structure. This Tree could be made generic so that it could be
used at other places too. All that will be required is the fields
similar to what we are using for Node, and the API endpoints.
The API endpoints needs to be similar to what we have for Node.
To differentiate and use the existing NodeTree.js, a namespace could
be added so that it could be extended and new routes could be added
it someone needs to make a similar Tree structure anywhere else too.

This PR adds a namespace for existing Alchemy.routes for node type
objects.
Similarly, in future if it is required to use the currently implemented
NodeTree, all that will be required is to extend and add the routes
to the API with namespace, update the model similar to the Node model
attributes and use Alchemy.NodeTree().

Notable changes

The type is added as "node" at NodeTree.js, node_folder.hbs and
_node.html.erb since these are the places from where the type data
could be sent out and the current NodeTree works as expected.

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

NodeTree is being used for rendering the Menu in tree
structure. This Tree could be made generic so that it could be
used at other places too. All that will be required is the fields
similar to what we are using for Node, and the API endpoints.
The API endpoints needs to be similar to what we have for Node.
To differentiate and use the existing NodeTree.js, a namespace could
be added so that it could be extended and new routes could be added
it someone needs to make a similar Tree structure anywhere else too.

This commit adds a namespace for existing Alchemy.routes for node type
objects. The type is added as "node"  at NodeTree.js, node_folder.hbs and
_node.html.erb since these are the places from where the type data
could be sent out and the current NodeTree works as expected.
Similary, in future if it is required to use the currently implemented
NodeTree, all that will be required is to extend and add the routes
to the API with namespace, update the model similar to the Node model
attribbutes and use Alchemy.NodeTree().
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, this is a very nice abstraction. I have some questions, though

In the previous commits, NodeTree was supported with type
feature such that it could be used generically. This caused
an issue where existing Menu was not able to toggle.

This commit updates node_folder to include data-record-id
rather than data-node-id to fix the issue and align with the
recent 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.

After moving a node I am getting 404 errors from target_parent_id missing.

I think in this line we need to change node_id into record_id
https://github.com/AlchemyCMS/alchemy_cms/pull/2037/files?diff=unified&w=1#diff-bcd606e5cbbdeba1415b27e585e6fde84ad19c9bf1be5edbc662c8616e9759deL70

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. This is a nice abstraction

@tvdeyen tvdeyen added this to the 6.0 milestone Mar 2, 2021
@tvdeyen tvdeyen merged commit d05a757 into AlchemyCMS:main Mar 2, 2021
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