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: draggable grouping shouldn't throw error when dynamically changing columns #922

Merged
merged 1 commit into from
Mar 3, 2023

Conversation

dmitov92
Copy link
Contributor

@dmitov92 dmitov92 commented Mar 1, 2023

  • get latest reorderedColumns on each end event handler

- get latest reorderedColumns on each end event handler
@dmitov92
Copy link
Contributor Author

dmitov92 commented Mar 1, 2023

@ghiscoding The columns are getting updated from outside of this function and it often results in "cannot use 'id' of undefined" error as the reorderedColumns instance is getting outdated. If we hide/show columns or we reassign the columns input with a new instance, we are getting this error. Preserving the columns instance doesn't solve all errors.

@ghiscoding
Copy link
Owner

@dmitov92 how do you update the columns externally? Which functions are you using to do that?
The grid.getColumns() will always return the visible columns only, it's probably what we need here, I can't remember

@codecov
Copy link

codecov bot commented Mar 1, 2023

Codecov Report

Merging #922 (f79d073) into master (da782ed) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #922   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          244       244           
  Lines        16605     16604    -1     
  Branches      5906      5906           
=========================================
- Hits         16605     16604    -1     
Impacted Files Coverage Δ
...es/common/src/extensions/slickDraggableGrouping.ts 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@dmitov92
Copy link
Contributor Author

dmitov92 commented Mar 2, 2023

@ghiscoding We are changing the columns via the angular-slickgrid component (the input), but I don't think it matters how as long as the private columns in the component are updated. You are saving the columns in a let variable, but then we are still able to call setColumns from other functions (inside of Angular-Slickgrid) and when this happens - the let variable doesn't know about the new columns. We should always get all current columns in the beginning of the END event handler just like in this commit or else the let variable will become outdated. In our case, we are changing the columns after the initialization, the columns property of the Angular-Slickgrid gets updated, but the let variable in that function is still holding the old columns.

@ghiscoding
Copy link
Owner

ok great thanks for the explanation, I was busy with other things but I should be able to test this in the coming days and publish a new release after that. I'm expecting that all done by the end of the week.

@ghiscoding ghiscoding changed the title Update slickDraggableGrouping.ts fix(extensions): draggable grouping should keep in sync when dynamically adding columns Mar 3, 2023
@ghiscoding ghiscoding changed the title fix(extensions): draggable grouping should keep in sync when dynamically adding columns fix: draggable grouping should keep in sync when dynamically adding columns Mar 3, 2023
@ghiscoding ghiscoding changed the title fix: draggable grouping should keep in sync when dynamically adding columns fix: draggable grouping shouldn't throw error when dynamically changing columns Mar 3, 2023
@ghiscoding ghiscoding merged commit 07a39dc into ghiscoding:master Mar 3, 2023
@ghiscoding
Copy link
Owner

ghiscoding commented Mar 3, 2023

@dmitov92 this is now released under Slickgrid-Universal v2.6.2 and Angular-Slickgrid v5.6.1, thanks for the contribution 😃

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