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: ensure DOM alterations during initialization are always cleaned up when there are hidden parents and forceFitColumns=true #1085

Merged

Conversation

jonathanjytang
Copy link
Contributor

Fixes Issue #1084

@jonathanjytang
Copy link
Contributor Author

Since I wasn't sure if anyone else is using the existing protected fields _hiddenParents and oldProps, this change was made in a way that avoids changing the signatures of those fields.

@ghiscoding
Copy link
Collaborator

ghiscoding commented Nov 21, 2024

The changes seems ok to me, but like I said I never really worked on these functions, so maybe a review by @6pac might be good too. Let's see if all the Cypress E2E tests are passing or not, that would be a good indication too (edit: all tests are passing)

@jonathanjytang jonathanjytang force-pushed the fix/nested_css_changes_on_hidden_parents branch from ed99db7 to a9b52db Compare November 23, 2024 00:30
…up when there are hidden parents and forceFitColumns=true
@jonathanjytang jonathanjytang force-pushed the fix/nested_css_changes_on_hidden_parents branch from 2001488 to e7708d7 Compare November 23, 2024 00:33
@jonathanjytang
Copy link
Contributor Author

jonathanjytang commented Nov 23, 2024

I think it should be easy enough to add a flag so that repeat calls to cacheCssForHiddenInit don't have any effect - it should cache the CSS data only once. That's also a good point about a flag to disable this altogether so that the external code can handle it.

@6pac I've changed the PR as suggested so the CSS data is only cached once when the methods are called in a "nested" manner, which fixes the correctness issue in #1084. The change effectively ensures that when there's an nested call to the cacheCssForHiddenInit and restoreCssFromHiddenInit methods in SlickGrid code (only possible in autosizeColumns), the "inner" methods calls aren't run -- it seemed like the most straightforward way to ensure an "inner" nested restoreCssFromHiddenInit call is a no-op (for correctness), ie when forceFitColumns=true the sequence of 1) cacheCss -> 2) cacheCss -> 3) restoreCss -> 4) restoreCss having 2) and 3) be no-ops.

With this change, it seems to me users who want to modify hidden parents externally won't be affected by the cacheCssForHiddenInit / restoreCssFromHiddenInit calls within autosizeColumns, since the modified CSS does not show up outside of SlickGrid methods (unlike the initial request for options.suppressCssChangesOnHiddenInit where explicitInitialization=true was used and the issue was CSS changes observed in-between the two stages of initialization). Though I suppose future contributors can add a flag if they so desire.

@6pac
Copy link
Owner

6pac commented Nov 24, 2024

Thanks, this works.

@ghiscoding
Copy link
Collaborator

@6pac feel free to merge if you think this PR is good, it seems fine from my point of view. Cheers

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