-
Notifications
You must be signed in to change notification settings - Fork 424
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
Conversation
- fixed coding style
Missed last jQuerx use
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)
…l-selection-and-move.spec.js
This reverts commit 7e57d87.
There was a problem hiding this 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) { |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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>"); |
There was a problem hiding this comment.
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
BTW @MarkoBL and @ghiscoding I just patched for #727 - you might want to integrate it. 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. |
Merged
Yes, and when I converted everything, I actally didn't know, how to procedd with these. Like this
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.
What do you think? |
@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. |
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 |
Okay, in this case, I don't have to change anything.
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 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 What I'm doing now: I capture all the the return values and store them in an array. And the first non- |
The biggest problem that I have with this PR is all around the return value/event from the events, for example the |
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") { |
There was a problem hiding this comment.
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") { |
There was a problem hiding this comment.
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
@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 I also checked the |
|
||
function offset(el) { | ||
|
||
box = el.getBoundingClientRect(); |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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
target = arguments[ 0 ], | ||
i = 1, | ||
length = arguments.length, | ||
deep = true; |
There was a problem hiding this comment.
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
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() }); |
There was a problem hiding this comment.
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 }
another issue that I found, there are 2 references of ... 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 |
@6pac @MarkoBL @6pac 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 |
- fixes some errors that came up while testing the whole thing in Slickgrid-Universal
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. |
* 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 <[email protected]> Co-authored-by: tr4npt <[email protected]> Co-authored-by: Ben McIntyre <[email protected]>
Sorry, I messed it up to change the target (I accidentally renamed the branch). This closed the original pull request: #719