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 issue #68 editor with activeRow selection #241

Merged
merged 3 commits into from
May 12, 2018

Conversation

ghiscoding
Copy link
Collaborator

@ghiscoding ghiscoding commented May 11, 2018

This fixes the error reported in issue #68 with a Slick.RowSelectionModel({selectActiveRow: true}) was not triggering properly the Editor.

What was happening is that the Editor was getting triggered but canceled out by the row selection. The fix is to set the setActiveCellInternal function 4th argument to True (that arg is suppressActiveCellChangedEvent). This makes the Editor to work correctly and doesn't trigger the cell active change. Also we want to do this only when our Grid is Editable.

@6pac
Totally sorry about the last commit made on Master, I was supposed to be in my own bugfix branch but forgot to create the branch before pushing the commit. So I reverted the change... Again sorry about that.

Tested Scenarios

  1. Double-click to Edit and selectActiveRow to False
    • autoEdit: false
    • grid.setSelectionModel(new Slick.RowSelectionModel({selectActiveRow: false}));
  2. Double-click to Edit and selectActiveRow to True
    • autoEdit: false
    • grid.setSelectionModel(new Slick.RowSelectionModel({selectActiveRow: true}));
  3. Single-click to Edit and selectActiveRow to False
    • autoEdit: true
    • grid.setSelectionModel(new Slick.RowSelectionModel({selectActiveRow: false}));
  4. Single-click to Edit and selectActiveRow to True
    • autoEdit: true
    • grid.setSelectionModel(new Slick.RowSelectionModel({selectActiveRow: true}));

Extra Commits

After testing it intensively, I found out that the argument suppressActiveCellChangedEvent should only be set to True when there is actually an Editor associated to the cell that we just clicked. This way it cancels out the Active Cell when Editing (which is what we want) but doesn't when there isn't any Editor attached.

See below to describe what I wrote in previous paragraph.
The example below is set with autoEdit: true and selectActiveRow: true, the grid has Column B editable but Column C is not. When we click to edit any cell of Column B, it won't activate the row, but as soon as we click on any row of Column C it will activate the row since there is not Editor associated to that cell. I believe this is the correct behavior.

2018-05-11_21-05-39

@6pac 6pac merged commit d778e25 into master May 12, 2018
@6pac
Copy link
Owner

6pac commented May 12, 2018

Great, #68 was a bug I looked at, and the more I looked the further it went. Sometimes these things are due to several different use cases inadvertently having conflicting requirements, and they can only be resolved by laying out the requirements for each explicitly and working out a way to accommodate them all.

Glad this patch was able to resolve the issue :-)

@ghiscoding
Copy link
Collaborator Author

ghiscoding commented May 12, 2018

Glad you merged it since it was an issue in my current App development 👍

Will you consider that issue #68 closed then, or is that still a different bug?
I mean, I'm not sure if you want to leave it open or not

@6pac
Copy link
Owner

6pac commented May 14, 2018

Will have to go back and repro the issue in #68. Some other time.

@ghiscoding
Copy link
Collaborator Author

ghiscoding commented May 14, 2018

I did use the example that was given in the issue to make the code change, which is why I had multiple commits. I couldn't reproduce (tested scenarios) after the code change that is now merged, which is why I was asking if we should just close the issue.

@6pac
Copy link
Owner

6pac commented May 14, 2018

Sure, if it's been tested and the original bug is resolved, absolutely close it. You should have the rights to do that? Happy for you to do so.

@ghiscoding
Copy link
Collaborator Author

Yeah I do, but I like to check with you anyway, team work ;)

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