zgrossbart / jdd

A semantic JSON compare tool
http://www.jsondiff.com
Apache License 2.0
1.03k stars 180 forks source link

Removes jQuery #57

Closed gangadharjannu closed 1 year ago

gangadharjannu commented 1 year ago

PR is ready for functionality testing

zgrossbart commented 1 year ago

I found a bug. If you press the Compare button with nothing in either field you get this error in the browser console:

Uncaught TypeError: Cannot set properties of null (setting 'disabled')
    at Object.compare (jdd.js:1067:58)
    at HTMLButtonElement.<anonymous> (jdd.js:1147:13)

Then if you add some content and press the compare button again nothing happens.

zgrossbart commented 1 year ago

There are 21 jshint errors in jdd.js:

$ jshint jdd.js
jdd.js: line 679, col 103, Missing semicolon.
jdd.js: line 761, col 9, 'const' is available in ES6 (use 'esversion: 6') or Mozilla JS extensions (use moz).
jdd.js: line 761, col 191, Missing semicolon.
jdd.js: line 867, col 69, 'arrow function syntax (=>)' is only available in ES6 (use 'esversion: 6').
jdd.js: line 947, col 33, Missing semicolon.
jdd.js: line 954, col 89, 'arrow function syntax (=>)' is only available in ES6 (use 'esversion: 6').
jdd.js: line 956, col 60, Missing semicolon.
jdd.js: line 959, col 93, 'arrow function syntax (=>)' is only available in ES6 (use 'esversion: 6').
jdd.js: line 961, col 64, Missing semicolon.
jdd.js: line 971, col 86, 'arrow function syntax (=>)' is only available in ES6 (use 'esversion: 6').
jdd.js: line 973, col 57, Missing semicolon.
jdd.js: line 976, col 90, 'arrow function syntax (=>)' is only available in ES6 (use 'esversion: 6').
jdd.js: line 978, col 61, Missing semicolon.
jdd.js: line 988, col 84, 'arrow function syntax (=>)' is only available in ES6 (use 'esversion: 6').
jdd.js: line 990, col 55, Missing semicolon.
jdd.js: line 993, col 88, 'arrow function syntax (=>)' is only available in ES6 (use 'esversion: 6').
jdd.js: line 995, col 59, Missing semicolon.
jdd.js: line 1024, col 74, Extra comma. (it breaks older versions of IE)
jdd.js: line 1028, col 23, Extra comma. (it breaks older versions of IE)
jdd.js: line 1030, col 35, 'arrow function syntax (=>)' is only available in ES6 (use 'esversion: 6').
jdd.js: line 1145, col 45, Mixed double and single quotes.

21 errors

...and four in jdd_test.js

$ jshint jdd_test.js
jdd_test.js: line 270, col 27, Missing semicolon.
jdd_test.js: line 286, col 27, Missing semicolon.
jdd_test.js: line 304, col 50, ['string'] is better written in dot notation.
jdd_test.js: line 310, col 27, Missing semicolon.

4 errors

I'm happy to make a PR to your branch to fix these or let you fix them as you prefer.

zgrossbart commented 1 year ago

Everything else looks good for the functionality. I've tested this on the latest versions of Chrome, Firefox, Safari, and Edge.

gangadharjannu commented 1 year ago

Also in description I mentioned about What about jsl.interactions.js which is not used anywhere. Do we need that ?

If yes, I also need to convert that in native JavaScript

gangadharjannu commented 1 year ago

I added fixes for all the code review comments and pushed the changes

zgrossbart commented 1 year ago

Let's leave the jsl.interactions.js file in the project for now. I see that the reference has been removed from the index.html file so it isn't hurting anything.

gangadharjannu commented 1 year ago

This is all looking good. Thank you so much for the work. I think this is a really positive change. Please let me know when you're set and I'll merge it.

I also didn't remove jQuery in jdd_test.js. Do we need to do it ?

If not, you can merge the changes otherwise let me know

zgrossbart commented 1 year ago

I'm fine leaving jQuery in the test page for now. That file isn't used at runtime. Let me know when you're done making changes and I'll merge it.

gangadharjannu commented 1 year ago

I'm fine leaving jQuery in the test page for now. That file isn't used at runtime. Let me know when you're done making changes and I'll merge it.

I'm done changing :)

zgrossbart commented 1 year ago

You're all merged. Thank you so much for all the work. I'm going to do a little more testing and then I'll get it deployed.