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

BugFix constructor overload in DefaultPortModel #1013

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Alletkla
Copy link

@Alletkla Alletkla commented Aug 3, 2023

Checklist

  • The tests pass --> I didn't found one for default implementation and didn't get them to run (see below)
  • I have referenced the issue(s) or other PR(s) this fixes/relates-to --> didn't found any
  • I have run pnpm changeset and followed the instructions
  • I have explained in this PR, what I did and why
  • I replaced the image below
  • Had a beer/coffee/tea because I did something cool today

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)

Changed overloaded option creation check
@changeset-bot
Copy link

changeset-bot bot commented Aug 3, 2023

🦋 Changeset detected

Latest commit: 6ed03bb

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
Copy link
Author

Alletkla commented Aug 3, 2023

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

@Alletkla
Copy link
Author

Alletkla commented Aug 3, 2023

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.

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.

1 participant