-
Notifications
You must be signed in to change notification settings - Fork 429
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
checkmark not shown when selecting a row progammatically #478
Comments
Thanks for raising this, will check it out on the weekend |
That seems very very risky to remove a trigger, you might fix something by breaking something else (or someone else code) |
I seem to recall there were some parts of the selection/current row logic that were very murky, the more I looked into it the more complex it got, and finally did nothing pending a complete rewite. Hopefully this is not that part of the code ... |
That is when adding more and more Cypress E2E tests does help and becomes more important. Every new PR that I do, I'm trying to add new tests as much as possible (now at 78 tests yay), though I wish someday that someone else will contribute in adding these tests too lol |
In response to the kind observation from ghiscoding, I am not suggesting to remove a trigger. On the contrary, the line "if (simpleArrayEquals(pr...etc" that I am commenting out prevents the trigger to be fired on some circumstances. On the case of my examples, they fail because the trigger is not fired due to the line I commented out. Removing the line (and its closing bracket a couple lines below) allows the trigger to fire on all occasions. I think that the purpose of that line is to prevent the trigger on some cases for speed optimization reasons, but I am not sure of that for I did not take the time to fully understand the code. |
My fault, I did not mean to close this. My apologies. |
@6pac Here's the test that fails 1) Example - Checkbox Header Row should click on Select All and display previous and new selected rows:
CypressError: Timed out retrying: Expected to find content: '1,3,5,7,9,11,13,15,17,19,21,23,25,27,29,31,33,35,37,39,41,43,45,47,49,51,53,55,57,59,61,63,65,67,69,71,73,75,77,79,81,83,85,87,89,91,93,95,97,99' within the element: <div#selectedRows> but never did. That is when the "Select All" button is clicked, it seems to be broken after your last commit. Perhaps you can run the Cypress E2E tests on your local, you can follow the instruction I wrote in README |
and am I correct in that the only problem with the previous release was the wrong version number in the package-lock.json file? |
I can't find any reason for that test not to work. It works locally for me, and I didn't modify that example or any code that depends on it. It's now 12:30pm HERE and I'm turning in! |
@gastonceron thanks a lot for that bug report! That was a really bad bug - the getSelectedRows() method was returning the actual internal selectedRows variable and the button code was modifying it, hence when calling the 'selections changed' code, the change was already made and no change was detected. I've modified slick.grid.js so that getSelectedRows() returns a copy of the array instead, and now it's all back to normal. |
@6pac I will look at that test over the weekend then. I don't think the |
from what I could see, the |
@6pac I'll take a look this weekend and try to fix this, but that is a new bug. I'm reopening the issue to remind myself to look into this. |
It's OK, I've fixed this now too. An additional bug in the plugin. It was using |
oh ok cool, so are all the Cypress tests passing now? That'd be great, 1 less thing to look into :) ...yup I see the GitHub Action is all green now and also tried locally, also all green. Sweet, I guess we could close this issue since you've fixed it all. Thanks Still glad I started adding all these E2E tests, it's starting to help. |
Thanks for your help.
My program is running fine now with the new version 2.4.21
Gaston
From: Ben McIntyre [mailto:[email protected]]
Sent: Friday, March 27, 2020 11:25 PM
To: 6pac/SlickGrid <[email protected]>
Cc: Gaston Ceron <[email protected]>; Mention <[email protected]>
Subject: Re: [6pac/SlickGrid] checkmark not shown when selecting a row progammatically (#478)
It's OK, I've fixed this now too. An additional bug in the plugin. It was using if (rowItem.selectableRow !== false) { rather than checkSelectableOverride(row, dataContext, _grid) in one last spot
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#478 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AC22Z2GOLSRXUQUXJATXIATRJVU2LANCNFSM4LUKAVFQ> .
|
When selecting a row programmatically (for instance from a click to a button) the associated check box does not get checked. I modified the program "example-checkbox-row-select.html" from the examples folder for my testing. See below:
select row 5. . . etc.
1 - Start the program, Click the first button (select row 5). Row 5 gets selected but the checkbox is not checked. The box shows checked when manually selecting any other row by clicking on their check box.
2 - Start the program, Click the first button twice. The checkbox is cheked on the second click. Click the second button (unselect row 5). The row is unselected but the check mark is not removed. Click any other row. The check mark now is removed.
3 – Start the program. Click the first button twice. The check mark is checked. Click on the second button (unselect row 5). The check mark is not un-checked. Click on the third button (unselect all rows). The check mark is not unchecked. Click on the checkbox of any other row. The check mark on row 5 is unchecked.
SOLUTION:
Modify function handleSelectedRangesChanged(e, ranges) (around line 2683 of slick.grid.js)
comment lines 2703 and 2705. See below:
// if (simpleArrayEquals(previousSelectedRows, selectedRows)) {
trigger(self.onSelectedRowsChanged, {rows: getSelectedRows()}, e);
// }
The text was updated successfully, but these errors were encountered: