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

fix: regression introduced by PR #640 and commit f12f4cc #644

Merged
merged 2 commits into from
Sep 28, 2021

Conversation

ghiscoding
Copy link
Collaborator

@ghiscoding ghiscoding commented Sep 28, 2021

  • this PR fixes 2 issues identified in Frozen Grids and in Slickgrid-Universal lib

the grid shown below has both issues and this PR fixes both of them
1- the top portion of the grid below is broken and throws error shown below and is caused by a regression introduced via commit f12f4cc which is the new Post Async cleanup
2- the bottom portion, which shows object everywhere is caused by a regression introduced from previous PR #640
both issues can be seen in this live Example, the error thrown is

Uncaught TypeError: Cannot read properties of null (reading 'removeChild')

which seems to be caused by this.parentElement being null in some cases

TODOs

image

- this PR fixes 2 issues identified in Frozen Grids and in Slickgrid-Universal lib
@6pac
Copy link
Owner

6pac commented Sep 28, 2021

Merge this and do new release ASAP?

@ghiscoding
Copy link
Collaborator Author

@6pac yes please since I can't use the latest as it is.

@6pac 6pac merged commit aa21e33 into master Sep 28, 2021
@ghiscoding
Copy link
Collaborator Author

ghiscoding commented Sep 28, 2021

@6pac thanks I tested again with latest version in our Salesforce and it's all good now so thanks for that. We can see surely see that adding more Cypress tests (137 tests now) does add value on the long run to make sure Examples aren't broken. I wish others could contribute Cypress tests as well, not just me though haha. Anyway, thanks again for the quick turnaround, really appreciate. 👍

@6pac
Copy link
Owner

6pac commented Sep 28, 2021

No worries. Yeah, the Cypress tests are still a mystery to me, but one day I'll sit down and take a look. Getting less busy (slowly).

@6pac
Copy link
Owner

6pac commented Sep 28, 2021

I am keen to push ahead with TypeScript as well now. Perhaps if we got it partially done, it would motivate the Raid Guild to come back and finish it off. I'll hve to review what you've already done and get my head around your pipeline for the other grid repos.

@ghiscoding
Copy link
Collaborator Author

Converting is seriously a major task, it would require to rewrite the entire project, it can't be just part of it (all or nothing). I think there are better place to put our time into, for example adding Cypress tests or even better other core change could be to get rid of jQueryUI which would be nice (I came across this lib sortable.js which could be used to replace draggable/sortable code (like draggable grouping & column reordering), jQueryUI is a big fat lib and I would rather put focus on that first, then of course there's the jQuery lib itself and that is a much bigger task. There's plenty to do

@6pac
Copy link
Owner

6pac commented Sep 29, 2021

Sure, I get your points. It's been the drag and drop that's been the main issue, but it looks like sortable.js may support that?

I haven't done enough TS yet, but I know it's all rendered to js, so we could say convert slick.core.js to TS, and continue to use the js output file with the other native js files. Is this not how it works?

@ghiscoding
Copy link
Collaborator Author

Sure, I get your points. It's been the drag and drop that's been the main issue, but it looks like sortable.js may support that?

it does seem to support drag & drop, sort and other things too in a small package with lots of users, so yeah very interesting and it also support touch as well.

I haven't done enough TS yet, but I know it's all rendered to js, so we could say convert slick.core.js to TS, and continue to use the js output file with the other native js files. Is this not how it works?

not quite, because all the SlickGrid structure was written with jQuery namespace, I'm not totally sure how that would translate into TypeScript class/module though I've never really look that deep and I'm in the process of rewriting all controls/plugins as vanilla JS (without jQuery, except Draggable Grouping which requires jQueryUI draggable) into my own lib, I got about 60% done. I could eventually push that back into this repo but I would rather finish them on my side first. My next step was to eventually look at that Sortable.js, so it was on my plan to look into it eventually. After all, that is the first thing that slickgrid-es6 repo removed from his fork (he used another lib though)

@6pac
Copy link
Owner

6pac commented Sep 29, 2021

Thanks for the run-down. Great for me to know all of this.
I'll try to have a tinker, I also have some command-line RegEx based tools I've custom-built that may be able to assist with removing jQuery from the codebase, though clearly jQueryUI is the priority. It has felt like it needs to go for a long time!

@ghiscoding ghiscoding deleted the bugfix/froken-frozen-grid branch October 21, 2021 01:22
@6pac
Copy link
Owner

6pac commented Oct 31, 2021

hey @ghiscoding, I've looked around at libraries and I think draggable js is probably the best bet - comprehensive for drag/drop and sorting, lightweight, modern and popular.
The main competition is interact js.
You mentioned sortable js which looks good but doesn't offer generalised drag and drop from what I can see.

The only negative for draggable is that a lot of its original contributors have left the project - experience tells me that means it's not unlikely that they are building something new from scratch. Or perhaps it's just because this is part of shopify and they have left after it got started, to focus on the shopify core (or to retire and spend their IPO money!!!).

Do you have any objections or opinions?

@ghiscoding
Copy link
Collaborator Author

Well I don't know what is to develop year over year on a drag & drop library, the concept is not very large like SlickGrid is, both of these lib work with touch as well so what else is left to develop in that kind of lib? Either or doesn't really matter to me, I would go with size and functionality of the lib, typically it should be fairly small since drag & drop is now part of ES6 (though it's a very basic implementation).

@6pac
Copy link
Owner

6pac commented Oct 31, 2021

OK, I'll keep looking. I notice that draggable is techically still in beta. The production download is 2kb unminified.
Last time I looked, drag and drop was natively supported by most browsers, but the support was patchy and all over the place as far as cross-browser compat went. I think using a lib is a good thing now, but maybe in 5 years it won't be needed any more.

@ghiscoding
Copy link
Collaborator Author

BTW, you looked at a different lib than I was looking, which is Sortable.JS and is highly maintained with 23k stars

@6pac
Copy link
Owner

6pac commented Nov 1, 2021

I did mention that above, I think (unless there are two sortables). It doesn't appear to support generalised drag and drop. If we want to remove jQueryUI that means getting rid of the drag and drop files too, so I figure one dependency for sorting and drag/drop is better.

@ghiscoding
Copy link
Collaborator Author

there's 3 different libs that were mentioned here and Sortable.JS might be a confusing name but it's all about drag & drop, there's also this Wiki Article - Sorting with the help of HTML5 Drag'n'Drop API that explains some of these concepts. So yes I think it can replace jQueryUI and this article explains some of that

@6pac
Copy link
Owner

6pac commented Nov 1, 2021

Sure, I am aware that it uses drag and drop in service of sorting, but it didn't look as if it supported a lot of the more advanced non-sorting related scenarios that the other libs do. I will take a closer look though. At around a kilobyte, the other libs aren't exactly heavy though! I just remember that last time I tried to use the HTML drag and drop API, I fled in terror.

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