Skip to content

Conversation

@junsikshim
Copy link
Collaborator

@junsikshim junsikshim commented Jan 22, 2022

Summary
Fixed the 'Anchors' example.

  • point.gif is now loaded using import. Other images should follow, but I haven't modified them in this PR.
  • In the example, the connection constraints were per-shape basis, but during refactoring they were changed to per-geometry basis. So, the custom getAllConnectionConstraints() is now updated accordingly.
  • Applied prettier on the changed files, so this commit is larger than it actually is. (I think we should run prettier on pre-commit hook or something)

Description for the changelog
Fixed the 'Anchors' example.

Other info

covers #73

@junsikshim junsikshim requested review from mcyph and tbouffard January 22, 2022 04:18
*/
// pointImage: mxImage;
pointImage = new Image(`${Client.imageBasePath}/point.gif`, 5, 5);
pointImage = new Image(pointImg, 5, 5);
Copy link
Member

@tbouffard tbouffard Jan 25, 2022

Choose a reason for hiding this comment

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

⚠️ this prevents to use an alternate image
Historically, the images were not packaged withing the mxGraph bundle. When using the editor, you could provide your own image set and mxClient.imageBasePath was used to configure the path to the custom images. The same remark applies to the change in ConnectionHandler.
My comment is mainly about informing all of us that we drop this support for sure. I don't know if it was broken in maxGraph before this change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it'd better to provide a way to overwrite those images by adding some parameters to Graph.
Maybe we could change the signature of Graph constructor a bit like (to an object),

constructor({
  container: ...,
  model: ...,
  plugins: ...,
  stylesheet: ...,
  resources: {
    ConnectionPointImageUrl: ...
  }
})

Then, the users can provide their own images while the default images are also available.

Let me know what you think!

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this will be a better way. This can apply to other images referenced within the code.
I suggest we manage it in a dedicated Discussion/Pull Request as it goes beyond the scope of the fix we want to do here.

Copy link
Collaborator

@mcyph mcyph Jan 27, 2022

Choose a reason for hiding this comment

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

I think if we wanted to be (reasonably) back-compatible, we could set Client.imageBasePath to be null by default and use ${Client.imageBasePath}/point.gif if specified by the user or pointImg if not i.e.

pointImage = new Image(Client.imageBasePath != null ? `${Client.imageBasePath}/point.gif` : pointImg, 5, 5);

I feel more generally, it would be nice to allow setting specific user subclasses of handlers to a graph so that you could have different settings for different graphs on the same page. While subclasses might be a verbose solution, it at least is fairly consistent and haven't yet been able to think of a better solution.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it is best to stay away from global configuration(the previous way) since it is separated from the actual Graph instantiation. When we want to create multiple different graphs on the same page, there has to be separate <script> tags containing Client.imageBasePath = ... which also could result in race conditions and so on.

I agree with @tbouffard that this should be an another topic. I'll merge this PR first, and let's discuss on this somewhere else.

deletable?: boolean;
direction?: DirectionValue;
edge?: string;
edgeStyle?: string;
Copy link
Member

Choose a reason for hiding this comment

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

❤️ This restores the original mxGraph style value. This will help people migrating from mxGraph.
Original mxGraph code: https://github.com/jgraph/mxgraph/blob/v4.2.2/javascript/src/js/util/mxConstants.js#L1796

@tbouffard
Copy link
Member

The story is now working, good job! 👍🏿

anchors_are_now_working.mp4

@junsikshim junsikshim merged commit aa22e83 into development Jan 27, 2022
@junsikshim junsikshim deleted the anchors branch January 27, 2022 10:01
@tbouffard tbouffard mentioned this pull request Oct 9, 2022
@tbouffard tbouffard added the bug Something isn't working label Nov 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants