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 bug - do not remove last row from cache #25

Merged
merged 1 commit into from
Feb 3, 2016
Merged

Conversation

royling
Copy link
Contributor

@royling royling commented Jan 5, 2016

It seems an obvious boundary check mistake, comparing the 2 condition check in https://github.com/6pac/SlickGrid/blob/2.1/slick.grid.js#L1601-L1610.

It seems an obvious boundary check mistake, comparing the 2 condition check in https://github.com/6pac/SlickGrid/blob/2.1/slick.grid.js#L1601-L1610.
@royling
Copy link
Contributor Author

royling commented Jan 5, 2016

Hi,

Recently I submitted this PR (mleibman/SlickGrid#1102) to the original repo for fixing a bug in updateRowCount function, but because the repo is not active any more, no response after that.
From other PRs, I get to this alternative repo (thanks for this to keep SlickGrid going on). So I submitted the PR here again, would you please review it?

Let me know if you think this change may have any impact or any reason why it is as is.

Thanks!

@6pac
Copy link
Owner

6pac commented Jan 5, 2016

thanks royling. I am aware of this patch but I have been insanely busy for a few months. i will assess and apply it soon. It hasn't been high priority because it's been around for a while and doesn't seem to have been flagged before.
however, patches are always appreciated!

@royling
Copy link
Contributor Author

royling commented Jan 6, 2016

This issue may not cause a critical problem, so not flagged. Let me elaborate the problem I encountered:

When editing a cell in the last row in the grid, and do something else to trigger grid resize, then the cell cannot be activated any more.

I debugged the whole process and found that it's removing the cache of the last row, then the row will be redrawn, but leaving active cell as is. Then the click event will not be handled.

Hope this clarifies the issue.

@6pac
Copy link
Owner

6pac commented Jan 8, 2016

thanks, i wish all pull requests were this well put together! The two conditions definitely should match, but after a quick look I would have thought both should be >= rather than both being >
do you have an opinion on that?

@royling
Copy link
Contributor Author

royling commented Jan 9, 2016

No, > is right if you see this line - https://github.com/6pac/SlickGrid/blob/2.1/slick.grid.js#L1601
the variable l = length - 1

6pac added a commit that referenced this pull request Feb 3, 2016
fix bug - do not remove last row from cache
@6pac 6pac merged commit 05b027f into 6pac:master Feb 3, 2016
@6pac
Copy link
Owner

6pac commented Feb 3, 2016

(long pause due to holiday)
royling, you are of course correct. I've pulled the patch.

thanks again

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.

None yet

2 participants