search

6pac / SlickGrid

A lightning fast JavaScript grid/spreadsheet
https://github.com/6pac/SlickGrid/wiki
MIT License
1.83k stars 423 forks source link

Should we cleanup Unused Code/Functions #514

Closed ghiscoding closed 4 years ago

ghiscoding commented 4 years ago

I wonder if we should cleanup all the unused code, now that es-lint is part of the project, I see a lot of these errors by the linter and after doing some search across the entire project, I found these

That is the full list but only for the slick.grid.js file, there's probably more in other files. @6pac should we cleanup the code or just disregard them?

ghiscoding commented 4 years ago

also as a side note, I found another bug with using SlickGrid in SalesForce that I'll investigate next week. So you can wait until I get that resolved with a PR before pushing a new release.

6pac commented 4 years ago

No probs re the release.

I have a bit of a love-hate relationship with linters, they do find outright errors, which is great, but at the same time they usually flag a whole bunch of code that I don't think is wrong, for example the !== vs != paradigm. I know settings can be used to turn the categories of warning on and off, but they are not terribly discriminating. Also, a construct might be a mistake in one place but valid in another. Ideally, they would scan the code and you'd have the option to turn off warnings on specific lines, and then they'd never raise that warning on that line again.
So generally, I manually run them once a year or so, and fix any obvious errors they identify.

To answer your actual question :-) , happy to remove anything that's outright trash, but the above I think are worth keeping around. They don't hurt anything and may be useful. I think they are best removed in a big refactor, where we are working to a big picture and are in a better position to evaluate whether they are useful or not.

ghiscoding commented 4 years ago

so shall I close this issue then?

ghiscoding commented 4 years ago

@6pac Hey Ben, I finished troubleshooting the issue I had with the grid in SalesForce, which happens when I want to get the Editor position with the function getActiveCellPosition which itself calls the absBox it returns me incorrect values and I believe it's because SalesForce uses Shadown DOM (with slot) and it can't find the position of the real parent container because it's outside of it's shadow DOM....

So long story short, I managed to fix it with local code and I won't have any new PRs coming, so you can release a new version whenever you're ready. Thanks

6pac commented 4 years ago

Cool. Let me know if a release becomes urgent, left to my own devices I'll probably wait another week or two until I have a chance to trawl through the backlog. I'm slowly getting back on top of it.

ghiscoding commented 4 years ago

It can wait a week, no problem, the more fix the better.

ghiscoding commented 4 years ago

@6pac Have you had a chance to look at your backlog? I'd be fine by me to have a new version by next weekend (or early next week) if possible, so I could release new versions on my side as well.

Let me know if I can be of any help with some of these issues you wanted to look at. Cheers

6pac commented 4 years ago

OK, so another emergency has emerged, probably not going to look at much. I'd like to fix the PostRender clear issue. But I wrote that feature originally, so I'm happy to tackle it. Will look at it on the weekend or I'll just do a release anyway!

ghiscoding commented 4 years ago

Ok sure, here's a reference to previous version (before Frozen feat), this could probably help you out, I wrote this comment in the issue that was opened for the PostRender

Looking at previous version tag 2.3.23, it seems to be called at 2 different places here and here

There's also unpkg - SlickGrid@2.3.23 which you can see any version code.