-
Notifications
You must be signed in to change notification settings - Fork 199
Fixes the 'Anchors' example. #77
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
Conversation
| */ | ||
| // pointImage: mxImage; | ||
| pointImage = new Image(`${Client.imageBasePath}/point.gif`, 5, 5); | ||
| pointImage = new Image(pointImg, 5, 5); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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
|
The story is now working, good job! 👍🏿 anchors_are_now_working.mp4 |
Summary
Fixed the 'Anchors' example.
import. Other images should follow, but I haven't modified them in this PR.getAllConnectionConstraints()is now updated accordingly.Description for the changelog
Fixed the 'Anchors' example.
Other info
covers #73