search

jrowen / rhandsontable

A htmlwidgets implementation of Handsontable.js
http://jrowen.github.io/rhandsontable/
Other
383 stars 148 forks source link

Fix typo in hot_context_menu. #360

Closed DillonHammill closed 3 years ago

DillonHammill commented 4 years ago

Just a minor fix so that hot_context_menu() throws a warning when useTypes = TRUE and allowColEdit = TRUE.

DillonHammill commented 3 years ago

@jrowen, I have also added a commit to fix an issue that I am currently facing with my DataEditR package. There are instances where the genRowHeaders code is being invoked when the event is afterChange - this caused problems downstream as ct is not present for this change but is required within the genRowHeaders code. The change I made simply bypasses this code if it is not a afterCreateRow or afterRemoveRow event.

DillonHammill commented 3 years ago

@jrowen, I also added a commit to handle logical column classes within colClasses(). This used to cause issues with new columns - which are assigned logical class initially due to NA values. However, when data is entered these edits are not retained under the current implementation. To fix this I have used utils:type_convert() to ensure that columns with logical class are appropriately converted to numeric, character or logical classes (I turned off factor conversion by default for these columns). Happy to amend how you see fit, particularly if you want to avoid importing utils. I personally think this is the best way to do this as it is widely adopted by other packages and will automatically return suitable classes for new columns (including those that should be of class logical).

DillonHammill commented 3 years ago

@jrowen, I added a new commit to also handle cases where an empty column is created and then a new row is added. At the moment, when a new row is created columns with a logical class have all their NAs converted to FALSE - I suspect that this was added for checkbox columns. The problem is that new columns are initiated with class logical too, so all their values get converted to FALSE when a new row is added. To fix this, I have just bypassed the NA to FALSE conversion if the column contains only NA values to cover new columns. This does mean that if no checkboxes are ticked they will remain as NAs but I think this is a highly unlikely situation - it defeats the whole purpose of adding the checkbox column in the first place (i.e. there should always be at least one box checked which will trigger NA to FALSE conversion).

jrowen commented 3 years ago

Sounds like a great fix.

On Fri, Dec 11, 2020, 6:17 PM Dillon Hammill @.***> wrote:

@jrowen https://github.com/jrowen, I added a new commit to also handle cases where an empty column is created and then a new row is added. At the moment, when a new row is created columns with a logical class have all their NAs converted to FALSE - I suspect that this was added for checkbox columns. The problem is that new columns are initiated with class logical too, so all their values get converted to FALSE when a new row is added. To fix this, I have just bypassed the NA to FALSE conversion if the column contains only NA values to cover new columns. This does mean that if no checkboxes are ticked they will remain as NAs but I think this is a highly unlikely situation - it defeats the whole purpose of adding the checkbox column in the first place (i.e. there should always be at least one box checked which will trigger NA to FALSE conversion).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jrowen/rhandsontable/pull/360#issuecomment-743493836, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALVRLQ5RYNBQ627YCGQ3NTSUKZBTANCNFSM4OUFAJBQ .

DillonHammill commented 3 years ago

These changes are now incorporated into rhandsontable 0.3.8.