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

checkmark not shown when selecting a row progammatically #478

Closed
gastonceron opened this issue Mar 26, 2020 · 17 comments
Closed

checkmark not shown when selecting a row progammatically #478

gastonceron opened this issue Mar 26, 2020 · 17 comments

Comments

@gastonceron
Copy link

gastonceron commented Mar 26, 2020

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
<button id="unselectRow5" style="position:relative;height:30px">
  unselect row 5</button>

<button id="unselectAllRows" style="position:relative;height:30px">
  unselect all rows</button>

. . . etc.

<script>
var grid;
 
$("#selectRow5").click(function() {
    let rowIndex = 5; // use row 5 for this example
    let selectedRowsIndexes = grid.getSelectedRows();
    selectedRowsIndexes.push(rowIndex);
    grid.setSelectedRows(selectedRowsIndexes);
});

$("#unselectRow5").click(function() {
    let rowIndex = 5; // use row 5 for this example
    let newSelectedRowsIndexes = [];
    let oldSelectedRowsIndexes = grid.getSelectedRows();
    while (oldSelectedRowsIndexes.length != 0) {
        let indexFound = oldSelectedRowsIndexes.pop();
        if (indexFound != rowIndex) {
            newSelectedRowsIndexes.push(indexFound)
        }
    }
    grid.setSelectedRows(newSelectedRowsIndexes);
});

$("#unselectAllRows").click(function() {
    grid.setSelectedRows([]);
});
</script>

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);
// }

@6pac
Copy link
Owner

6pac commented Mar 26, 2020

Thanks for raising this, will check it out on the weekend

@ghiscoding
Copy link
Collaborator

That seems very very risky to remove a trigger, you might fix something by breaking something else (or someone else code)

@6pac
Copy link
Owner

6pac commented Mar 27, 2020

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 ...

@ghiscoding
Copy link
Collaborator

not sure if it's also related to this issue #345, seems different but who knows.

Also worth noting that this issue came from this Stack Overflow question

@ghiscoding
Copy link
Collaborator

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

@gastonceron
Copy link
Author

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.

@gastonceron
Copy link
Author

My fault, I did not mean to close this. My apologies.

@ghiscoding
Copy link
Collaborator

ghiscoding commented Mar 27, 2020

@6pac
I have good and bad news for you, I saw you made a commit to try to fix this issue however it seems that the few Cypress E2E tests that I had added some time ago now fails. I tried re-running the GitHub Actions to retest but it still fails and the Good News is that these E2E tests now have a greater reason to exist 😉

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

@6pac
Copy link
Owner

6pac commented Mar 27, 2020

and am I correct in that the only problem with the previous release was the wrong version number in the package-lock.json file?

@6pac
Copy link
Owner

6pac commented Mar 27, 2020

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!

@6pac
Copy link
Owner

6pac commented Mar 27, 2020

@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 6pac closed this as completed Mar 27, 2020
@ghiscoding
Copy link
Collaborator

@6pac I will look at that test over the weekend then. I don't think the package.json has any effect on the content of the npm bundle though.

@6pac
Copy link
Owner

6pac commented Mar 27, 2020

from what I could see, the package.json contained the correct version, it was the package-lock.json that didn't because it's new and I hadn't updated my release script to sync the version number to it.

@ghiscoding
Copy link
Collaborator

ghiscoding commented Mar 27, 2020

@6pac
I looked at the Cypress test failing and got latest code, it really is a new bug that you introduced. The test is for making sure that when using the icon override (only show the checkbox on every 2nd row), it should return only the possible rows (1, 3, 5, ...) but after your change it now returns even rows that don't have the checkbox (0, 1, 2, 3, 4, 5, ...).

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.

image

@ghiscoding ghiscoding reopened this Mar 27, 2020
@6pac
Copy link
Owner

6pac commented Mar 28, 2020

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

@ghiscoding
Copy link
Collaborator

ghiscoding commented Mar 28, 2020

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.

@gastonceron
Copy link
Author

gastonceron commented Mar 29, 2020 via email

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

No branches or pull requests

3 participants