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

remove jquery dependency #47

Open
wants to merge 6 commits into
base: unstable
Choose a base branch
from

Conversation

marfyl
Copy link

@marfyl marfyl commented Jul 25, 2022

  • remove jquery dependency on javascript code
  • remove all references in the readme file and demo htmls
  • code formatter
    • tabWidth: 4
    • singleQuote: true

Each file was formatted in a different way and cytoscape-node-editing.js file use several criterias at the same time, so I use the criteria shown above, since it were the most used inside the file. (If you want another criteria I can change it)

I think that in the future it would be good to add a .editorconfig file or .prettierrc.js file to maintain consistent coding styles for multiple developers working on the same project across various editors and IDEs. (I could add one if you want)

I hope this PR will help, anything let me know.

@hasanbalci
Copy link
Contributor

@marfyl Thanks for the PR. I checked some other extensions we developed and it seems that we generally prefer tabWidth: 2. Can you also change it for this PR as tabWidth: 2?

@marfyl
Copy link
Author

marfyl commented Aug 13, 2022

done! I just pushed the changes, thanks for the review.

@hasanbalci
Copy link
Contributor

hasanbalci commented Aug 17, 2022

@marfyl Thanks for the changes. I reviewed the PR and it seems generally ok, but I noticed a bug in your version that doesn't exist in our version. You can see that in the below video I generated using undoable demo. Can you please check the cause of the problem?

node-editing-bug.mp4

@marfyl
Copy link
Author

marfyl commented Aug 27, 2022

fixed, please refresh the cache to avoid problems

@marfyl
Copy link
Author

marfyl commented Aug 31, 2022

Do you need anything else from my side to bring out a new product release? Anything else you need just let me know

@hasanbalci
Copy link
Contributor

@marfyl Sorry for the late response. Your last commit fixed the error that I mentioned, but I now realized another bug during undo-redo operation. It is as follows:
Correct behavior in master branch (when I undo, nodes return to their original position):

master_branch.mp4

Behaviour in PR (nodes don't return to their original positions):

PR_branch.mp4

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.

2 participants