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

feat: support custom nodeTree in connectors.create() #317

Merged
merged 7 commits into from
Jan 16, 2022

Conversation

linxianxi
Copy link
Contributor

@linxianxi linxianxi commented Sep 22, 2021

issue: #316

To save nodes as templates that can be dragged into the editor, userElement can be of type function that returns NodeTree.

Executed at the start of each drag to generate a new id for each node.

connectors.create(
    ref,
    // executed at the start of each drag to generate a new id for each node
    () => {
        const nodeTree = ...;
        const newNodeId = getRandomId();
        ......
        return nodeTree;
    }
  )

@netlify
Copy link

netlify bot commented Sep 22, 2021

✔️ Deploy Preview for craftjs ready!

🔨 Explore the source changes: e938d54

🔍 Inspect the deploy log: https://app.netlify.com/sites/craftjs/deploys/61df7a65bbee920007b36fa2

😎 Browse the preview: https://deploy-preview-317--craftjs.netlify.app

@@ -245,7 +245,7 @@ export class DefaultEventHandlers<O = {}> extends CoreEventHandlers<
},
create: (
el: HTMLElement,
userElement: React.ReactElement,
userElement: React.ReactElement | (() => NodeTree),
Copy link
Owner

Choose a reason for hiding this comment

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

Could we have userElement: React.ReactElement | NodeTree instead?

In that case, we will probably need a helper function (ie: isNodeTree) to check if the input is a valid NodeTree as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You cannot replace it with NodeTree.Because this function needs to be executed each time to generate a new ID for each node.If it is NodeTree, the id of each drag will be the same

Copy link
Owner

Choose a reason for hiding this comment

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

Makes sense. In that case, could we update this callback so it accepts a React.ReactElement as well? So:

userElement: React.ReactElement | (() => NodeTree | React.ReactElement)

@Spenc84
Copy link

Spenc84 commented Dec 17, 2021

What is the status of this PR? This change would be incredibly helpful for our use case so we would love to see it go through. Is there anything that still needs to happen before this can get merged in?

@prevwong prevwong changed the title fix: fix create userElement's type to support custom nodeTree feat: support custom nodeTree in connectors.create() Jan 10, 2022
@prevwong
Copy link
Owner

Apologies on the late response. Was out on holiday! PR looks good, would be great to have this included as well if possible!

@prevwong prevwong changed the base branch from next to develop January 10, 2022 10:12
@linxianxi
Copy link
Contributor Author

Apologies on the late response. Was out on holiday! PR looks good, would be great to have this included as well if possible!

It's ok now.

@Spenc84
Copy link

Spenc84 commented Jan 11, 2022

@linxianxi Would you be willing to slightly adjust your most recent commit so that userElement() is only getting called once? That should improve efficiency slightly and could also prevent a potential issue that might pop up in the unlikely scenario that userElement() returns a React Element on one call and a Node Tree on the other.

Something like this perhaps?

let tree, result;
if (typeof userElement === 'function') {
    result = userElement();
    if (isValidElement(result)) {
        tree = store.query
            .parseReactElement(result as React.ReactElement)
            .toNodeTree();
    } else {
        tree = result;
    }
} else {
    tree = store.query.parseReactElement(userElement).toNodeTree();
}

@linxianxi
Copy link
Contributor Author

@linxianxi Would you be willing to slightly adjust your most recent commit so that userElement() is only getting called once? That should improve efficiency slightly and could also prevent a potential issue that might pop up in the unlikely scenario that userElement() returns a React Element on one call and a Node Tree on the other.

yes,I wrote it too fast to notice

Copy link
Owner

@prevwong prevwong left a comment

Choose a reason for hiding this comment

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

Thanks! 💯 @linxianxi and @Spenc84

@prevwong prevwong merged commit d4c2164 into prevwong:develop Jan 16, 2022
@prevwong
Copy link
Owner

Released in 0.2.0-beta.2 🎉

@Nym02
Copy link

Nym02 commented Jan 19, 2024

@linxianxi @Spenc84 @prevwong

"{"ROOT":{"type":{"resolvedName":"CanvasContainer"},"isCanvas":true,"props":{"flexDirection":"column","alignItems":"flex-start","justifyContent":"flex-start","fillSpace":"no","padding":["0","20","0","20"],"margin":["0","0","0","0"],"background":"#FFFFFF","color":"#000000","shadow":0,"radius":0,"width":"600px","minHeight":"300px"},"displayName":"Frame Container","custom":{"displayName":"App"},"hidden":false,"nodes":[],"linkedNodes":{}}}"

this is a JSON i get when I save my template. Now I want to drag this to the editor? Is it possible? I can see that this PR supports NodeTree in connectors.create() . Is there anyway I can implement drag feature on Serialized JSON? Any code example will really help.

@xs-nayeem
Copy link

xs-nayeem commented Jan 31, 2024

@linxianxi Would you be willing to slightly adjust your most recent commit so that userElement() is only getting called once? That should improve efficiency slightly and could also prevent a potential issue that might pop up in the unlikely scenario that userElement() returns a React Element on one call and a Node Tree on the other.

Something like this perhaps?

let tree, result;
if (typeof userElement === 'function') {
    result = userElement();
    if (isValidElement(result)) {
        tree = store.query
            .parseReactElement(result as React.ReactElement)
            .toNodeTree();
    } else {
        tree = result;
    }
} else {
    tree = store.query.parseReactElement(userElement).toNodeTree();
}

@Spenc84 It looks like it only takes a single node. what to do in case of multiple node?

@Spenc84
Copy link

Spenc84 commented Feb 2, 2024

@xs-nayeem I apologize, it's been a couple of years since I worked on the section of our codebase that implements craft, so my memory of the implementation details is a bit hazy, but I thought any single node could specify any number of of child nodes underneath it? If so, it seems like you simply need to make sure that the single node used here is the parent of any of the other nodes that should also get added to the layout. Just make sure that your new node and every one of it's descendants each have their own unique id.

@xs-nayeem
Copy link

@xs-nayeem I apologize, it's been a couple of years since I worked on the section of our codebase that implements craft, so my memory of the implementation details is a bit hazy, but I thought any single node could specify any number of of child nodes underneath it? If so, it seems like you simply need to make sure that the single node used here is the parent of any of the other nodes that should also get added to the layout. Just make sure that your new node and every one of it's descendants each have their own unique id.

@Spenc84 thanks for your response. Well I am trying to achieve that but somehow I am missing something maybe.
https://pastebin.com/gzqGpyu5 this is the JSON i am trying to drag to the editor. If could make some time to give me some hint about how can i drag this to the editor.

@Spenc84
Copy link

Spenc84 commented Feb 5, 2024

Again I haven't used any of that code in awhile, so my memory is hazy, but if that JSON you linked is static and you're trying to replicate that after each drag then I imagine you'll run into some issues with duplicated ids. IIRC each node in your layout has to have it's own unique ID, so for instance you couldn't have two different nodes with the id "ROOT", or two different nodes with the id "b2Hu6-fsIl" within the same layout JSON. If you're not doing it already, my first suggestion would be to write some code that parses that JSON and replaces each id with a new unique id within the callback method that you're passing to .create(ref, callback).

Example:

connectors.create(
    ref,
    // executed at the start of each drag to generate a new id for each node
    () => {
        /* Here you might copy the template JSON you're trying to replicate while also
        *  replacing the id for every node with a new unique one. */
        const newNodeTree = generateUniqueNodeTree( EXISTING_STATIC_JSON );
        return newNodeTree ;
    }
  )

I also don't recall exactly how the code was implemented to integrate the JSON for a new nodeTree into the existing layout JSON via the .create method. I imagine it would take all of the nodes in the returned nodeTree JSON and add it to the existing layout JSON, and then take the id of the first node in returned JSON and add that id as a child of whichever node on the layout that you dragged the new nodeTree into.

However if it does not work that way then I suppose that it's also entirely possible that you can only return a single node from that callback, in which case you'd just let the create method handle adding the root node of your new nodeTree to the existing JSON and then you'd have to add all of the descendants of that node yourself programmatically either during or after the drag event which (IIRC) shouldn't be to difficult to do. You will still need to ensure that each newly added node has it's own unique id.

@xs-nayeem
Copy link

@Spenc84 thanks for your response. I have achieved what I was looking for. If anyone want to drag multiple node at once, all the nodes must have a parent node. Parent node id will be the root node id and all the children must be defined the parent node's node array.

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.

5 participants