search

cdnjs / new-website

🖥 cdnjs.com website
https://cdnjs.com
ISC License
451 stars 212 forks source link

Lint & refactor public/js/main.js #298

Closed MattIPv4 closed 5 years ago

MattIPv4 commented 5 years ago

This resolves #297 by updating the test script to ensure ESLint checks public/js/main.js This also refactors the whole file to give us a cleaner base to work from going forward.

MattIPv4 commented 5 years ago

Sorry for the awful diff all. I'm super confident everything is working as intended on the front-end still after lots of testing, but I am happy to be proven wrong and then fix it.

MattIPv4 commented 5 years ago

@PeterDaveHello I'm happy to move the eslint change to its own commit for traceability but would like to keep in it this PR as it was a style rule that was reflected in this code originally and is still in place in the refactored code, I don't see a reason to move it to a separate PR.

As for splitting the changes by type, I could go in and attempt to do this now, but when I refactored this, I pretty much rewrote the file from scratch, so there isn't really a good way to split it up. Splitting by "section" would be almost impossible as so much of the old code was interlinked.

I realise it's an absolute nightmare to review it as one commit, but I think, in this case, that's going to be the easiest thing to do. Sorry! :(

PeterDaveHello commented 5 years ago

The linter config change is not reflected by the PR topic, and I believe that config should and would also be appied to the other js files, not only public/js/main.js, that's why it should be separated.

The other changes should be separated from the beginning, refactoring is really nice, but the process is as important as the refactoring, can't be done by a such huge commit which is not tracable and reviewable.

To be honest, effort of reviewing this commit will be times of splitting it, and also the future tracing effort, we can reduce the effort of future, so please let's do it the right way, with the already done right things here ;)

PeterDaveHello commented 5 years ago

btw, I also see some naming change / renaming changes, that's something should be separated, thank you!

MattIPv4 commented 5 years ago

@PeterDaveHello I don't understand how I would split this into like comment, indent, wrap, etc - I didn't do this when I refactored the file, I rewrote it from scratch?

PeterDaveHello commented 5 years ago

@MattIPv4 From my past experiences, yes, for this huge change, rewrite it from scratch would be easier, as they are too big to be just committed separately, the different thing is - this time, you have refactorred result as reference, only need to focus on how to split them, and the result would be really helpful.

MattIPv4 commented 5 years ago

Right, but as I've rewritten it from scratch, many things don't track over and I can't just "split it up". So many things were interlinked in the old version and some still are in the new version, there is no logical way to split this up.

if you see a way to split this up as you have requested, I invite you to do so. I do not see a way to do this and can only split this per "section", which is what I had done prior to squashing as you normally would request everything is squashed.