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

feat: Drop jQuery requirement #734

Merged
merged 28 commits into from
Apr 29, 2023
Merged

feat: Drop jQuery requirement #734

merged 28 commits into from
Apr 29, 2023

Conversation

MarkoBL
Copy link
Contributor

@MarkoBL MarkoBL commented Mar 8, 2023

Sorry, I messed it up to change the target (I accidentally renamed the branch). This closed the original pull request: #719

MarkoBL added 22 commits March 6, 2023 17:10
If jQuery is present, slideUp/slideDown is used, oterhwise the container will hide/show.

Fixes test: example-grid-menu.spec.js
…example-plugin-custom-tooltip.spec.js

Changed example-plugin-custom-tooltip.spec.js to use consistent mouse events (mouseenter/mouseleave and mouseover/mouseout)
Copy link
Collaborator

@ghiscoding ghiscoding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great start, I didn't test with my libs yet but that should be soon

@@ -178,7 +178,7 @@ <h2>View Source:</h2>


var containers = $.map(columns, function (c) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$.map is a jQuery function, I know it's just an Example but if we're expecting to remove jQuery everywhere then...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's not prohibited to use SlickGrid and jQuery together :) There are 77 examples included, and I don't see the use to modify every example. Maybe some important ones, but not all. And the plugins still make use of jQuery.

My goal was to remove jQuery from the core library (all the js files in the root directory) and make the vanilla version usable without jQuery.

@@ -72,7 +72,7 @@ <h2>View Source:</h2>

// create a detached container element
var myGrid = $("<div id='myGrid' style='width:600px;height:500px;position:relative;'></div>");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we keeping jQuery in some Examples? $("div...") is what I'm referring to

examples/example1-simple.html Show resolved Hide resolved
slick.editors.js Outdated Show resolved Hide resolved
slick.editors.js Outdated Show resolved Hide resolved
slick.grid.js Show resolved Hide resolved
slick.grid.js Outdated Show resolved Hide resolved
slick.grid.js Show resolved Hide resolved
slick.grid.js Outdated Show resolved Hide resolved
slick.grid.js Outdated Show resolved Hide resolved
@6pac
Copy link
Owner

6pac commented Mar 13, 2023

BTW @MarkoBL and @ghiscoding I just patched for #727 - you might want to integrate it.
It's a one-liner but was affecting performance of some frozen row/col unit tests quite severely.

Also, I see you're running across some issues with the grid returning jQuery objects from function calls, eg. for page elements. Things like this are not only breaking changes but essentially a paradigm shift.
We really need a good migration HOWTO summarising all the changes the developers (consumers of the grid) will have to make, probably with examples.

@MarkoBL
Copy link
Contributor Author

MarkoBL commented Mar 13, 2023

BTW @MarkoBL and @ghiscoding I just patched for #727 - you might want to integrate it. It's a one-liner but was affecting performance of some frozen row/col unit tests quite severely.

Merged

Also, I see you're running across some issues with the grid returning jQuery objects from function calls, eg. for page elements. Things like this are not only breaking changes but essentially a paradigm shift. We really need a good migration HOWTO summarising all the changes the developers (consumers of the grid) will have to make, probably with examples.

Yes, and when I converted everything, I actally didn't know, how to procedd with these. Like this

    function getFooterRow() {
      return hasFrozenColumns() ? _footerRow : _footerRow[0];
    }

In one case, it returns a HTML element and in the other case, it returns an array of HTML Elements. To make this consistent, the grid should probaby always return an array, with 1 or 2 elements.

    function getFooterRow() {
      return hasFrozenColumns() ? _footerRow : _footerRow.slice(0, 1);
    }

What do you think?

@6pac
Copy link
Owner

6pac commented Mar 13, 2023

@MarkoBL re return values, ouch, those were already pretty inconsistent. I think we're best off with a more consistent return type, either a single element or an array depending on the circumstances.

Since it's going to be breaking anyway, I don't think it matters what we end up with as long as it's (1) useful, (2) as consistent as possible and (3) well documented.

@ghiscoding
Copy link
Collaborator

ghiscoding commented Mar 13, 2023

we'll of course need a migration Wiki like I've done with Migration 3.0.

I would probably keep the same return, a single element or an array depending on the circumstance, that seems fine by me too. @MarkoBL What I'm more concerned about is the new getReturnValues() that you added to all the notify, I posted a comment about that, I'm not sure why it has to change but if it does have to change I would rather have a returned object that includes both the event and the returned value { event, value } instead of having to call an extra getReturnValues(). I could be missing the point, so perhaps you could provide us more info on the subject

@MarkoBL
Copy link
Contributor Author

MarkoBL commented Mar 15, 2023

I would probably keep the same return, a single element or an array depending on the circumstance, that seems fine by me too.

Okay, in this case, I don't have to change anything.

@MarkoBL What I'm more concerned about is the new getReturnValues() that you added to all the notify, I posted a comment about that, I'm not sure why it has to change but if it does have to change I would rather have a returned object that includes both the event and the returned value { event, value } instead of having to call an extra getReturnValues(). I could be missing the point, so perhaps you could provide us more info on the subject

Well, I don't know anymore, why I made it a function and it doesn't make any sense. I can change it to a property called returnValue.

The reason why I return the EventData: The notify stuff was basically broken. Let's say you have two event handlers attached to an event: The first would return true and the next undefined, notifiy would always return undefined, because it always overwrites the older return values and takes the last. And as the grid uses event handlers internally, this could lead to strange bugs, if external code subscribes to an important internal event that requires a return value.

What I'm doing now: I capture all the the return values and store them in an array. And the first non-undefined value is used as the default return value, returned by getReturnValue() and if you want to access all returned values from the event handlers, you call getReturnedValues(). But as I said, I could change these to properties, instead of functions.

@ghiscoding
Copy link
Collaborator

ghiscoding commented Mar 20, 2023

The biggest problem that I have with this PR is all around the return value/event from the events, for example the onBeforeEditCell is used by every users of the grid and is expecting that if we're passing false to the event, then we assume that the Editor will not show up. Will that behavior change? Are we going to have to pass an object instead of a boolean as argument to onBeforeEditCell? I haven't tried the code yet and I'm not exactly clear if this behavior will change or not, if it did then we'll have a lot backlash from the community (me included, I still want to pass a boolean). I don't see any examples using the onBeforeEditCell event but still, we need to be sure of its usage with this PR before proceeding. Thanks

https://github.com/6pac/SlickGrid/blob/5367e73a30878513c126a5b6dbf35fe462f7b197/slick.grid.js#LL5341C7-L5344C8

@6pac
Copy link
Owner

6pac commented Mar 20, 2023

Will try to have a look at these issues on the weekend. The notification comments are troubling: I didn't realise this was broken. Fixing it should be an option rather than working around the whole concept.

From my point of view, there should be no changes to method signatures (ie. input parameters) or return values (outputs), other than that if an parameter or return value (or element of an object as a return value) is a jQuery object or array of objects, it should be changed to the nearest equivalent non-jQuery object or array or objects.

headerColumnWidthDiff = headerColumnHeightDiff = 0;
if (el.css("box-sizing") != "border-box" && el.css("-moz-box-sizing") != "border-box" && el.css("-webkit-box-sizing") != "border-box") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the if condition is missing and is causing some styling issues on my side, I believe it should be

if (style["box-sizing"] != "border-box" && style["-moz-box-sizing"] != "border-box" && style["-webkit-box-sizing"] != "border-box") {
  //...

var r = $("<div class='slick-row' />").appendTo($canvas);
el = $("<div class='slick-cell' id='' style='visibility:hidden'>-</div>").appendTo(r);
cellWidthDiff = cellHeightDiff = 0;
if (el.css("box-sizing") != "border-box" && el.css("-moz-box-sizing") != "border-box" && el.css("-webkit-box-sizing") != "border-box") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as previous comment, the if condition is also missing

@ghiscoding
Copy link
Collaborator

ghiscoding commented Apr 22, 2023

@MarkoBL I'm starting to look at this, I'm starting to leave some comments on what I see missing and also so far I see that the Column Picker is not showing up at the correct x/y positions. It's caused by the event being triggered is missing e.pageX and e.pageY and the source of the problem is because when creating a new EventData() it doesn't include the element native event and that is a problem, the EventData should be a merged object that includes both the EventData and the element event (when exist)

I also checked the onBeforeEditCell and that is working correctly, so no issues there

image

image


function offset(el) {

box = el.getBoundingClientRect();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

box is missing the variable declaration let or const and because it is missing, it can throw this error that I received

referenceError: box is not defined

function offset(el) {

box = el.getBoundingClientRect();
docElem = document.documentElement;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same thing for docElem, it is missing the variable declaration and throws an error

.on("click", handleClick)
.on("dblclick", handleDblClick)
.on("contextmenu", handleContextMenu)
.on("mouseenter", ".slick-cell", handleMouseEnter)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was this changed from mouseenter to mouseover? It's not the same event, so why the change?


for (let i = 0; i < children.length; i++) {
const colElm = children[i];

if (i >= columns.length) { return; }
if (i < firstResizable || (options.forceFitColumns && i >= lastResizable)) {
return;
Copy link
Collaborator

@ghiscoding ghiscoding Apr 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this return was previously used within a jQuery .each() method, this return will not work inside a querySelectorAll loop because it will stop the loop too early. Basically we need to change the return to a continue for this to work the same as a jQuery each loop.

if (i < firstResizable || (options.forceFitColumns && i >= lastResizable)) {
-  return;
+  continue; 
}

How was this identified? Not all examples were having an issue but the issue was mostly showing up when using Slick.Editors, in the example-draggable-grouping.html, the resizable handle wasn't showing up, if we then change the return to a continue, we then get it working like it should

image

target = arguments[ 0 ],
i = 1,
length = arguments.length,
deep = true;
Copy link
Collaborator

@ghiscoding ghiscoding Apr 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the deep default property is wrong, it should be set to false since that would follow more closely jQuery.extend() and not cause surprises (and possibly breaking change) that I found myself. I found this to be a problem when in my own Slickgrid-Universal lib in which column filters were not showing up when they should be. Basically, the following code in SlickGrid should not be deep copied (the 1st argument is {} and in jQuery simply mean merge the 2 next arguments or assign to a new object {} when undefined)

var m = columns[i] = utils.extend({}, columnDefaults, columns[i]);

is supposed to be a shallow copy (work as a pointer), however because of this deep = true, it actually became a deep copy and is causing issues in my lib because my lib assume these column copies are shallow copy and not deep copy. After changing default to deep = false, that would also follow more closely the NodeJS implementation in external 3rd party lib node.extend

https://github.com/dreamerslab/node.extend/blob/62adee3327630c83f3d39395f7e275bfba281ecc/lib/extend.js#L47-L55

module.exports = function extend() {
  var target = arguments[0] || {};
  var i = 1;
  var length = arguments.length;
  var deep = false;
  var options, name, src, copy, copyIsArray, clone;
  // ....

@@ -1642,60 +1685,71 @@ if (typeof Slick === "undefined") {
return impactedColumns;
}

function handleResizeableHandleDoubleClick(evt) {
const triggeredByColumn = evt.target.parentElement.id.replace(uid, "");
trigger(self.onColumnsResizeDblClick, { triggeredByColumn: triggeredByColumn.getReturnValue() });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line throws an error because const triggeredByColumn is a string and does not have a .getReturnValue() function and it work as expected when triggering without it like so { triggeredByColumn: triggeredByColumn }

@ghiscoding
Copy link
Collaborator

ghiscoding commented Apr 25, 2023

another issue that I found, there are 2 references of $.grep (at line 1334 and 1345) that should be changed to Slick.Utils.grep. Also a bigger problem, the Slick.Resizer plugin still uses jQuery and must be converted to vanilla, that shouldn't be too hard since I have nearly the same resizer service in my own lib and I did convert it to vanilla...

... so I believe that will all of these code change request, it covers it all. I did manage to remove jQuery entirely in my lib and all my E2E tests are now passing after fixing all of the code change requested above. This is getting even closer to be a reality :)

@MarkoBL, if we're not getting news from your side in the coming week, we'll probably merge the PR as it is and create a 2nd PR to fix all the code change I requested. Obviously I would prefer if you were able to fix these changes, yourself :)

EDIT

oops I just found out that none of the Plugins are actually converted to vanilla, that might be a bit more work

@ghiscoding ghiscoding changed the title WIP - feat: Drop jQuery requirement feat: Drop jQuery requirement Apr 29, 2023
@ghiscoding ghiscoding merged commit a02fad9 into 6pac:next Apr 29, 2023
@ghiscoding
Copy link
Collaborator

@6pac @MarkoBL
Alright guys, I just finished my entire testing in my Slickgrid-Universal lib, I got all my unit tests and E2E tests working and while fixing them, I found a few issues in this PR and I highlighted these problems as comments in here but since @MarkoBL doesn't seem to be available, I'll go ahead and merge this PR into the next branch and create a 2nd PR to fix all these issues comments that I highlighted. I'm proceeding this way so that we can keep all of @MarkoBL great contributions.

@6pac
Please note that this PR only migrated the files in the root (slick.grid, slick.core, slick.dataview, ...), however it does not include any Controls & Plugins. However, I don't think we can ship a new major 4.0 version without rewriting and migrating all Controls & Plugins to vanilla JS... sooo it will still need more work.

On the other hand, I was able to completely remove jQuery in Slickgrid-Universal, which is why I found a few issues in here, because I already rewrote all of these Controls & Plugins to vanilla JS (actually in TypeScript for my lib), so I should be able to port the code back into here and hopefully that shouldn't be too too long. I would really like to be ready for a release at the start of the summer (North summer that is 😉)

@6pac also note that your other PR should be rebased with the next branch since your code will be shipped at the same time under this next major branch

ghiscoding added a commit that referenced this pull request Apr 29, 2023
- fixes some errors that came up while testing the whole thing in Slickgrid-Universal
ghiscoding added a commit that referenced this pull request Apr 29, 2023
)

- fixes some errors that came up while testing the whole thing in Slickgrid-Universal
@6pac
Copy link
Owner

6pac commented May 2, 2023

great, thanks for all your work. i'm still extremely busy (probably till July) so won't have a lot of time to dedicate to this. Will definitely rebase that PR though, and can have a look at small issues.

6pac added a commit that referenced this pull request May 18, 2023
* feat!: Drop jQuery requirement  (#734)

* feat!: Drop jQuery requirement

* fix: addresses all issues found in jQuery removal previous PR #734 (#742)

- fixes some errors that came up while testing the whole thing in Slickgrid-Universal

* feat(plugins): convert slick.draggablegrouping to vanillaJS (#744)

- fix ESLint for Cypress
- also remove jQuery from package.json list of dependencies

* show tooltip if the cell content is taller than the cell height - fixes #740 (#741)

* show tooltip if the cell content is taller than the cell height

The current code does not show a tooltip when word wrap is turned on and the text is taller than the cell height.

* combined height check with width check

* fix: enable AutoScroll with SortableJS for column reordering, fixes #735 (#736)

* fix: enable AutoScroll with SortableJS for column reordering, fixes #735

* chore: add auto-scroll comment for clarity

* chore(ci): run Cypress on the `next` branch as well as `main`

* feat(plugin): convert slick.autotooltips to vanillaJS (#745)

- remove jQuery from plugin and also in the autotooltip example as well

* chore: fix some UI issues in draggable grouping plugin

* feat(plugins): convert copy manager plugins to vanillaJS (#746)

* feat(plugins): remove jQuery from slick.customtooltip plugin (#747)

* feat(plugins): remove jQuery from header buttons/menus plugins (#748)

* chore: apply better code styling to few core files (#749)

* chore: apply better code styling to few core files

* feat(plugins): remove jQuery from ColumnPicker & GridMenu controls (#752)

* feat(plugins): remove jQuery from ColumnPicker & GridMenu controls

* tests: use input checked property instead of attr checked
- the previous code with `attr('checked')` was jQuery oriented and we are going away from jQuery

* feat(plugins): remove jQuery from CellMenu & ContextMenu plugins (#753)

* feat(plugins): remove jQuery from range decorator selection model (#754)

* feat(plugins): remove jQuery from range decorator selection model

* feat(plugins): remove jQuery from CheckboxSelectColumn plugins (#755)

* feat(plugins): remove jQuery from RowMove plugins (#756)

* feat(plugins): remove jQuery from Grid State plugin (#757)

* feat(plugins): remove jQuery from Grid Resizer plugin (#758)

* chore: remove Map polyfill since we will target ES6 (#759)

- Slick.Map polyfill is no longer required since Map is included in ES6 browsers

* feat(plugins): remove jQuery from Row Detail plugin (#760)

* Correct some instances of migration from $.each() to iteration (return needs to become continue)

* chore: remove eval & grep utils and replace with simple ES6 filter

* fix: filter header row should follow grid scroll

* feat(controls): remove jQuery from Slick Pager control (#762)

* fix: scrolling for all containers should work for regular & frozen grids

* fix: add missing aria accessibility (#764)

- closes #586, #587, #588 and #678

* chore: filling the window should be used with slick.resizer, closes #515

* chore: migrate more examples to vanilla JS with DOMContentLoaded

* chore: convert html template to pure DOM create element with JS (#766)

* chore: remove jQuery from all possible examples (#767)

* chore: fix html code showing up in column picker & grid menu picker (#768)

* fix(core): set wheel/touch listeners to passive for better perf (#769)

- this fixes warnings shown in Chrome and other browser console mentioning that we should consider using `passive` event listeners
- also uses a polyfill in case the `passive` option is not supported (for example IE)

* chore: better use of DOM element creation and innerHTML (#770)

- also remove `passive` mode to certain events that use preventDefault since that is not compatible with `passive` mode

* chore: remove jQuery from lib folder, replace with CDN (#771)

* Bugfix/example issues fixes (#772)

* fix: found a few small issues while testing examples with jQuery CDN

* fix: throw error when freezing columns are wider than canvas (#773)

- closes #667
- freezing columns cannot be wider than the actual grid canvas because when that happens, the viewport scroll becomes hidden behind the canvas... so let's throw an error advising the user to make adjustments

* fix: toggling frozen rows should recalc scroll height, closes #737 (#774)

- when changing frozen rows via `setOptions`, it should recalculate each viewports (top/bottom)
- the previous code skipped scroll height recalculation and that caused the issue identified in #737

* feat: Enable hidden property for column. Adds example-column-hidden, method… (#765)

* Enable hidden property for column. Adds example-column-hidden, method getVisibleColumns() and alternate method updateColumns() calling event onBeforeUpdateColumns() for when a hidden property has changed but the column list itself has not changed.

* remove jQuery from example and add frozen rows/hidden cols example

* final changes: add frozen columns example, fix issue with hidden columns on frozen grid boundary, fix gridmenu control to work with .hidden flag on columns)

* changes as suggested in #765

* feat: remove legacy TreeColumns code - now unused (#775)

* remove legacy treecolumns code - now unused

* fix typo and add back apparently unnecessary call to setcolumns() which does in fact do crucial refreshing of grid structure

* chore: fix a small editor problem with percent editor

* chore(release): publish version 4.0.0-beta.0

* chore: add migration guide to v4.0 link in changelog

* chore: remove jQuery from Example 4

---------

Co-authored-by: Marko B. Ludolph <der.ludi@web.de>
Co-authored-by: tr4npt <tranp@fastmail.us>
Co-authored-by: Ben McIntyre <email.ben.mcintyre@gmail.com>
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.

3 participants