Skip to content
This repository was archived by the owner on Jul 29, 2019. It is now read-only.

fix(network) selected image: setting image doesn't affect current image #2894

Closed
wants to merge 2 commits into from
Closed

fix(network) selected image: setting image doesn't affect current image #2894

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dd030dc
fix(network) selected image: setting image doesn't affect current image
mehl321 Mar 23, 2017
d2424b4
Merge branch 'develop' into develop
wimrijnders Apr 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions lib/network/modules/components/nodes/util/CircleImageBase.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,22 @@ class CircleImageBase extends NodeBase {
}

setImages(imageObj, imageObjAlt) {
if (imageObj) {
this.imageObj = imageObj;
if (!this.selected) {
if (imageObj) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: imageObj must always be defined on the call of this method. For the intent of your code changes, you can assume this is so.

this.imageObj = imageObj;

if (imageObjAlt) {
this.imageObjAlt = imageObjAlt;
if (imageObjAlt) {
Copy link
Contributor

Choose a reason for hiding this comment

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

imageObjAlt needs to be set always, not just if it is defined. It should be possible to remove the alt image in an update.

This also applies to the uses of imageObjAlt below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I remove those checks and create an image node only containing an unselected image or even no image (which is stupid but possible to do) then we get a JS error.
You may want to update the code yourself.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, yes, the code needs to be rearranged somewhat.

You may want to update the code yourself.

If you mean that I create another PR to fix this, that's perfectly fine with me.
Shall I close this PR and create a new one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes please. I don't feel like refactoring the codebase myself. I'm too used to TypeScript now :)

Copy link
Contributor

Choose a reason for hiding this comment

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

OK fine.

I'm too used to TypeScript now :)

Oh, you lucky bastard. I'm itching to go there. There is no hope for TypeScript in vis.js, but I've been rabidly refactoring my own current project to make it possible.

Thank you sincerely for your effort here. The bug is genuine and it needs to be tackled. All the best!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no hope for TypeScript in vis.js

I understand that it would be an important update but maybe would it bring vis.js (or .ts??) to the next step! An intermediary step could be to switch to strict ES6 using eslint (with no vars etc.)

Copy link
Contributor

Choose a reason for hiding this comment

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

An intermediary step could be to switch to strict ES6 using eslint

Oh yes, definitely. That is actually an active discussion between the admins here 😄. Let's hope something good will come out of that!

this.imageObjAlt = imageObjAlt;
}
}

// if node currently selected keep current image
} else if (imageObjAlt) {
this.imageObj = imageObjAlt;
this.imageObjAlt = imageObj;

} else {
this.imageObj = imageObj;
}
}

Expand Down