Tabulator: fix default values of a list header filter #3826
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3826 +/- ##
==========================================
+ Coverage 83.00% 83.03% +0.02%
==========================================
Files 221 221
Lines 32312 32362 +50
==========================================
+ Hits 26821 26871 +50
Misses 5491 5491
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -216,7 +216,7 @@ | |||
| " 'int': None,\n", | |||
| " 'float': {'type': 'number', 'max': 10, 'step': 0.1},\n", | |||
| " 'bool': {'type': 'tickCross', 'tristate': True, 'indeterminateValue': None},\n", | |||
| " 'str': {'type': 'autocomplete', 'values': True},\n", | |||
| " 'str': {'type': 'list', 'valuesLookup': True},\n", | |||
There was a problem hiding this comment.
Is 'values' entirely ignored now or does it have another meaning? Trying to work out whether we can provide backward compatibility.
There was a problem hiding this comment.
It seems to be that when values is set to True it is just ignored by Tabulator JS. Otherwise values accept a list of values or dict of value/label.
We could look for values in the editor or header filter params, when its type is select or autocomplete, and replace it by valuesLookup?
There was a problem hiding this comment.
I'm adding backwards compatibility to select and autocomplete in 6ec3c1e, for the editor and header filter. The editor didn't set values to True by default, so I'm sticking to that.
I've also added a warning to encourage users to switch to list and lookupValues. Used self.param.warning, is that the preferred way over warnings.warn? I'm never sure.
There was a problem hiding this comment.
Honestly I have lost track about warnings too, I think at some point we just need to go through and make everything consistent.
|
Looks good, thanks! |
Before the Tabulator JS upgrade
values: Truewas alright but now we need to usevaluesLookup: True.