projectstorm / react-diagrams

a super simple, no-nonsense diagramming library written in react that just works
https://projectstorm.cloud/react-diagrams
MIT License
8.45k stars 1.16k forks source link

BugFix constructor overload in DefaultPortModel #1013

Open Alletkla opened 11 months ago

Alletkla commented 11 months ago

Checklist

What, why and how?

What: I changed the behaviour of setting "default" options when using the overloaded constructor of the DefaultPortModel. Why: When creating a DefaultPortModel like: new DefaultPortModel(true) the in option wasn't set and label was undefined How: Wanted to change it, first made name mandatory (would be another option) but then adopted style like in DefaultNodeModel to typechecking options.

Feel good image:

I Like

PS: It's my first contribution sorry for any mistakes on the process. PSS: wanted to Test but got: "The command '..' is incorrect or cannot be located in: reat-diagrams-routing test ../../node_modules/.bin/jest but the file is there. Maybe you can help thanks.

997 (not exactly but points at the problem)

changeset-bot[bot] commented 11 months ago

🦋 Changeset detected

Latest commit: 6ed03bbbc39f876db986c944d8010ff2f985bce6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages | Name | Type | | ------------------------------------- | ----- | | @projectstorm/react-diagrams-defaults | Patch | | @projectstorm/react-diagrams-routing | Patch | | @projectstorm/react-diagrams | Patch | | @projectstorm/react-diagrams-gallery | Patch | | @projectstorm/react-diagrams-demo | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Alletkla commented 11 months ago

Sorry found out that there are more chageset options besides "major" changed it.

Alletkla commented 11 months ago

Okay proceeded with my tests and there is another problem with this:

options.name is undefined before and after my change when constructing with new DefaultPortModel(true)

So what to set as a "default name" for a port? 'Untitled' like in the DefaultNodeModel seems inappropriate since the name is used as an index in NodeModel

addPort(port) {
        port.setParent(this);
        this.ports[port.getName()] = port;
        return port;
    }

Seems more and more to me that it would be better to make name mandatory as an easy fix.

But referencing #997 a change in NodeModels this.ports[port.getName()] = port; would be more appropriate.

At least i don't know why its name indexed here and not like in PortModel:

    addLink(link) {
        this.links[link.getID()] = link;
    }

id indexed.

Maybe I'm missing something. If not: When somebody explains to me how I get the tests running I will do the change.