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: remove legacy TreeColumns code - now unused #775

Merged
merged 2 commits into from
May 17, 2023
Merged

Conversation

6pac
Copy link
Owner

@6pac 6pac commented May 15, 2023

Updated version: Remove legacy TreeColumns code

@6pac
Copy link
Owner Author

6pac commented May 15, 2023

hmmm ... impressive crash and burn for something that should be zero impact. trying to work out what's wrong here - all appears to be well on a browser.

@ghiscoding
Copy link
Collaborator

I'll mention again that this code came from the X-SlickGrid merge, so it's something to do with Frozen grids, I don't understand how it works and what it does exactly but it has to be related to Frozen feature

@ghiscoding ghiscoding changed the title remove legacy treecolumns code - now unused feat: remove legacy TreeColumns code - now unused May 15, 2023
@6pac
Copy link
Owner Author

6pac commented May 15, 2023

@ghiscoding yes, I'm aware that it came from the other repo, but as far as I can tell, it does nothing useful and it's unused anywhere in the project. And it's quite modular - there is literally zero impact from removing it. As far as I can tell, it's a completely separate feature to frozen columns - it was just another (very overcomplicated) way of grouping columns.

A lot of tests are failing, but there's no problems I can see with the examples that are failing, when I open them in a browser. I'm currently trying to get Cypress to work on my dev environment so I can try to repo the failure locally. Got past a startup failure, now stuck with localhost not responding.

@ghiscoding
Copy link
Collaborator

you need to run the test under localhost and for that you need to run npm run serve, the full instructions are shown on homepage here

@6pac
Copy link
Owner Author

6pac commented May 16, 2023

Oh, thanks! A bit embarrassing that I didn't look at that first!

…ch does in fact do crucial refreshing of grid structure
@6pac
Copy link
Owner Author

6pac commented May 17, 2023

@ghiscoding OK, a typo fixed and a line of code I thought could be omitted couldn't be. Looks all good now!

Pretty much all this code would never be called unless treeColumns.HasDepth was true, which it never was unless you specifically set up parent column references. So, zero impact unless you were using this feature which was undocumented and untested.

Will merge once you've had a look. Bonus is I learned how to use Cypress. It's quite impressive!

@ghiscoding
Copy link
Collaborator

awesome this is great, I'm currently in the middle of trying something else which I'm not sure if we should ship or not under v4.0. Could you tell me what you think in the new Discussion #776 that I just opened for this.

Give me couple of hours and I should be able to give it a try in my own libs and check all my Cypress tests as well. As you found out, having proper E2E tests with Cypress is a really great feature to have, it avoids breaking our lib 😉

@ghiscoding
Copy link
Collaborator

ghiscoding commented May 17, 2023

@6pac ok I tried it in my lib and all Cypress tests (500x of them) still pass, also tried a bit in the UI and it seems to work correctly. So I'm ok with you merging it :)

It's also great that even if slick.core got bigger (because we needed to add back some functions that jQuery previously provided). You managed to lower the file size a bit more 🚀

@6pac 6pac merged commit af82a57 into next May 17, 2023
@ghiscoding ghiscoding deleted the remove-treecolumn-next branch May 17, 2023 19:10
@ghiscoding
Copy link
Collaborator

@6pac could you go in the project setting and in the "General" config, click the following checkbox so that I won't have to go manually delete branches by hand every time

image

@6pac
Copy link
Owner Author

6pac commented May 17, 2023

Done

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>
ghiscoding added a commit that referenced this pull request Dec 23, 2023
- the legacy TreeColumns depth was removed in PR #775 but some functions were missed and should be removed, they are `getHeadersWidthL()` and `getHeadersWidthR()`
ghiscoding added a commit that referenced this pull request Dec 23, 2023
- the legacy TreeColumns depth was removed in PR #775 but some functions were missed and should be removed, they are `getHeadersWidthL()` and `getHeadersWidthR()`
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