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

6pac · 2023-05-09T13:11:51Z

… getVisibleColumns() and alternate method updateColumns() calling event onBeforeUpdateColumns() for when a hidden property has changed but the column list itself has not changed.

slick.grid.js

ghiscoding · 2023-05-09T14:03:56Z

I see you modified plugins/slick.cellexternalcopymanager.js but there's also plugins/slick.cellcopymanager.js which I would guess needs to be modified too?

Have you also tried Column Picker and Grid Menu? You can try both in the Grid Menu example, it has both. We need to make sure that any plugin calling getColumns(). Do we need to replace getColumns() with getVisibleColumns() anywhere it's called? I don't fully understand that part and almost every plugins and controls are using this method

ghiscoding · 2023-05-09T14:06:11Z

examples/example-column-hidden.html

+
+<script src="../lib/firebugx.js"></script>
+
+<script src="../lib/jquery-3.1.0.js"></script>


we'll eventually have to migrate this example also to be without jQuery, I started converting a few but will probably do the rest after work today

6pac · 2023-05-10T03:32:02Z

@ghiscoding

LogColWidths was a debug logging function, I tend to leave these in so they are there to temporarily give me good logging when trying to find a bug. Not a disaster if it goes though.
have removed jQuery dependencies from the example page
have added a frozen grid/hidden column example for testing. it does have an issue, so will work on that. haven't removed jQuery from it yet.
plugins/slick.cellcopymanager.js doesn't need changes, it gets a range from the grid. The externalcopymanager is a lot more complex.
plugins/slick.columnpicker.js has changes applied

plugins/slick.gridmenu.js is the one I queried in the original version of this PR. Because it can show/hide columns it could be considered to be directly affected by this change.

Now we are getting back to our original discussion. I don't see how the addition of a hidden flag could be breaking to existing code when no-one's code uses it. We can still use the old/existing way of hiding columns by removing them from the columns list. But IF a developer starts using the flag and has external libraries, they may need to modify their libraries to work with the flag.
So to keep compatilbility and make it non-breaking, we should mod thie gridmenu plugin to work with the hidden flag if it is used externally, but not to use it by default to hide columns, since this would inject the hidden flag into the columns list and could break external dependencies.

We could also make two versions of gridmenu, one with the current behaviour where it resets the column list to hide/show (but taking note of the 'hidden' flag), and a second new one (with a different name) where it uses the 'hidden' flag as the primary show/hide device. The second one would be a lot simpler, and could be adopted in situations where full external support of the flag was guaranteed.

When I speak about this being breaking or not, I am also just referring to this repo. If you have a similar situation with additional controls or plugins in your repos (which I am ashamed to admit I am still not really familar with), then you would also need to take the same approach of not actively using the flag but allowing for it.
I'm not sure if this is the case or not. If your controls were not actively showing or hiding columns, then using getVisibleColumns would be an easy way of getting this behaviour. If they are showing and hiding columns, then there is no way around writing code to allow for the hidden flag.

Hope this makes sense.

ghiscoding · 2023-05-10T03:55:23Z

ah ok I didn't realize we need the hidden flag to enable the new behavior, it sounds good then. You can change the Grid Menu code directly, I have my own sets of controls/plugins that I rewritten in vanilla JS a while ago (which I used to migrate the code here too). So whatever code you change in the Grid Menu and/or Column Picker, won't affect my libs. Basically the way I've done it in my libs was to keep a variable reference of "all columns" and another variable for "only visible columns", it would be easier with your code but I can still use my current code in the meantime.

I'm not intending to re-discuss the entire subject, just trying to foresee possible breaking changes before they happen. However I think in most cases running all the Cypress tests should be sufficient to catch any of these errors

6pac · 2023-05-10T04:04:55Z

Sure. Will have a look at the Grid Menu this evening.
We are on the same page re breaking changes - I have no desire to go there again!

ghiscoding · 2023-05-10T04:08:51Z

oops sorry it looks like my latest PR #766 to convert the html template to pure JS brought conflicts, at least it's just 1 line, the new code is with utils.createDomElement (which is in pure JS)

… getVisibleColumns() and alternate method updateColumns() calling event onBeforeUpdateColumns() for when a hidden property has changed but the column list itself has not changed.

…mns on frozen grid boundary, fix gridmenu control to work with .hidden flag on columns)

6pac · 2023-05-15T05:14:55Z

@ghiscoding this is ready to go. Please Review.

ghiscoding · 2023-05-15T13:41:58Z

controls/slick.gridmenu.js

@@ -679,6 +679,7 @@
        let visibleColumns = [];
        columnCheckboxes.forEach((columnCheckbox, idx) => {
          if (columnCheckbox.checked) {
+            if (columns[idx].hidden) columns[idx].hidden = false;


I'm not a fan of 1 liner without curly braces especially for assignment. I accept 1 liner for return but I would prefer this one wrapped in curly braces because it's assignment

ghiscoding · 2023-05-15T13:43:48Z

examples/example-column-hidden.html

+  el.addEventListener("mouseleave", function(e) { e.target.classList.remove('ui-state-hover'); });
+});
+
+//$(".grid-header .ui-icon")


I've seen this code in a few example that didn't need it, if you don't have the search icon in the top blue header on top, then you can delete this code. It's only for a visual effect while hovering the search icon in that header, which isn't that useful, feel free to delete

ghiscoding · 2023-05-15T13:44:49Z

examples/example-column-hidden.html

+  grid.getTopPanel().append(el);
+  el.style.display = 'block';
+
+  //$("#inlineFilterPanel")


you can probably delete this piece of code too, it's meant to add a filter in the preheader section of SlickGrid

ghiscoding · 2023-05-15T13:45:22Z

examples/example-column-hidden.html

+    updateFilter();
+ });
+
+//  $("#txtSearch,#txtSearch2").keyup(function (e) {


again some more code you can probably delete, it goes with the previous one to add filter in the preheader

ghiscoding · 2023-05-15T13:48:07Z

@6pac I left some comments about dead code, feel free to merge after fixing them, the PR looks ok by me

* 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]>

ghiscoding · 2023-05-18T22:26:41Z

@6pac I was looking at the new example you provided and I think I found some problems with the behavior, you might need to look into this. It doesn't seem to play nice with Column Picker (probably Grid Menu as well). I think the indexes are getting out of sync after a column got the hidden prop

Also I was expecting the hidden column to still show in the column picker, is that a bad assumption and did you intend to not show a column in the picker after it being hidden?

there's also a problem if we hide a column and then change another column position, when unchecking the hide column, the column doesn't seem to come back as expected

6pac · 2023-05-19T02:39:07Z

Sorry, these are thing I never use or look at. Will check them out, I don't think it will be hard to fix.

6pac · 2023-05-21T12:39:06Z

Should be fixed in master now. Gotta say the logic is very convoluted and does my head in, with multiple sets of columns everywhere. An excellent justification for adding the hidden column flag.

I'll probably create a new (breaking) version of the plugin (with a different name) using the hidden column flag which wil be waaaaay simpler.

ghiscoding · 2023-05-21T18:23:28Z

I'll probably create a new (breaking) version of the plugin (with a different name) using the hidden column flag which wil be waaaaay simpler.

Are you talking about Column Picker and/or Grid Menu Controls? Sure they can be simplified, as long as it doesn't break current ones, so it's great that you fixed it. Thanks

6pac · 2023-05-21T22:43:41Z

I am proposing making the hidden flag the default way of showing/hiding, because it's so much simpler (only one copy of columns, ever), but I'd leave the originals as-is and give the new controls or plugins new names so they were intrisically non-breaking.

ghiscoding · 2023-05-22T19:27:54Z

@6pac hey sorry buddy, I don't like blowing it again but it still looks like a problem trying it in a different way. When I click on the "hide Duration", it looks like calling getColumns() no longer include "Duration" inside the array and when I uncheck the checkbox it throws an error. Isn't the getColumns supposed to include everything and getVisibleColumns() the rest?

Also note, I opened up 2 other PRs for errors that I received while finishing up code change and getting all Cypress tests for all my SlickGrid libs

6pac · 2023-05-24T00:40:26Z

Thanks, will check it out. I haven't encountered a show-stopper yet with this, hopefully this isn't it!

6pac changed the title ~~Enable hidden property for column. Adds example-column-hidden, method…~~ feat: Enable hidden property for column. Adds example-column-hidden, method… May 9, 2023

6pac mentioned this pull request May 9, 2023

feat: Column 'hidden' property #738

Closed

ghiscoding reviewed May 9, 2023

View reviewed changes

slick.grid.js Show resolved Hide resolved

ghiscoding reviewed May 9, 2023

View reviewed changes

6pac added 2 commits May 15, 2023 10:04

Enable hidden property for column. Adds example-column-hidden, method…

c04106c

… 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

1ec806b

6pac force-pushed the column-visible-next branch from a0040b8 to 1ec806b Compare May 15, 2023 00:37

final changes: add frozen columns example, fix issue with hidden colu…

b1a59d4

…mns on frozen grid boundary, fix gridmenu control to work with .hidden flag on columns)

ghiscoding reviewed May 15, 2023

View reviewed changes

changes as suggested in #765

d70b7e9

6pac merged commit 78540ef into next May 16, 2023

ghiscoding mentioned this pull request May 17, 2023

BUG: hiding a column by which the table is sorted breaks multi column sorting #546

Closed

ghiscoding deleted the column-visible-next branch May 17, 2023 19:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

6pac commented May 9, 2023

ghiscoding commented May 9, 2023

ghiscoding May 9, 2023

6pac commented May 10, 2023 •

edited

Loading

ghiscoding commented May 10, 2023

6pac commented May 10, 2023

ghiscoding commented May 10, 2023 •

edited

Loading

6pac commented May 15, 2023

ghiscoding May 15, 2023

ghiscoding May 15, 2023

ghiscoding May 15, 2023

ghiscoding May 15, 2023

ghiscoding commented May 15, 2023

ghiscoding commented May 18, 2023 •

edited

Loading

6pac commented May 19, 2023

6pac commented May 21, 2023 •

edited

Loading

ghiscoding commented May 21, 2023

6pac commented May 21, 2023

ghiscoding commented May 22, 2023 •

edited

Loading

6pac commented May 24, 2023


		<script src="../lib/firebugx.js"></script>

		<script src="../lib/jquery-3.1.0.js"></script>

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

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

Conversation

6pac commented May 9, 2023

ghiscoding commented May 9, 2023

ghiscoding May 9, 2023

Choose a reason for hiding this comment

6pac commented May 10, 2023 • edited Loading

ghiscoding commented May 10, 2023

6pac commented May 10, 2023

ghiscoding commented May 10, 2023 • edited Loading

6pac commented May 15, 2023

ghiscoding May 15, 2023

Choose a reason for hiding this comment

ghiscoding May 15, 2023

Choose a reason for hiding this comment

ghiscoding May 15, 2023

Choose a reason for hiding this comment

ghiscoding May 15, 2023

Choose a reason for hiding this comment

ghiscoding commented May 15, 2023

ghiscoding commented May 18, 2023 • edited Loading

6pac commented May 19, 2023

6pac commented May 21, 2023 • edited Loading

ghiscoding commented May 21, 2023

6pac commented May 21, 2023

ghiscoding commented May 22, 2023 • edited Loading

6pac commented May 24, 2023

6pac commented May 10, 2023 •

edited

Loading

ghiscoding commented May 10, 2023 •

edited

Loading

ghiscoding commented May 18, 2023 •

edited

Loading

6pac commented May 21, 2023 •

edited

Loading

ghiscoding commented May 22, 2023 •

edited

Loading