[SIP-5] Refactor table #5707
[SIP-5] Refactor table #5707
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5707 +/- ##
==========================================
+ Coverage 63.75% 63.78% +0.03%
==========================================
Files 364 364
Lines 23104 23110 +6
Branches 2579 2587 +8
==========================================
+ Hits 14729 14740 +11
+ Misses 8360 8355 -5
Partials 15 15
Continue to review full report at Codecov.
|
| filters, | ||
| includeSearch, | ||
| metrics: rawMetrics, | ||
| onAddFilter = NOOP, |
There was a problem hiding this comment.
Why do we need a No Op on these prop functions?
There was a problem hiding this comment.
Set NOOP as default value to avoid checking if onAddFilter exists every time before calling it.
| import tableVis from '../../../src/visualizations/table'; | ||
|
|
||
| // Fix `Option is not defined` | ||
| // https://stackoverflow.com/questions/39501589/jsdom-option-is-not-defined-when-running-my-mocha-test | ||
| global.Option = window.Option; |
There was a problem hiding this comment.
I'm not sure if it'd be useful to put this into the spec/helpers/browser.js file, that's where other things like this currently are. but once we move to packages, this type of config should be specific to each package.
| // Each object is { field1: value1, field2: value2 } | ||
| data: PropTypes.arrayOf(PropTypes.object), | ||
| height: PropTypes.number, | ||
| alignPn: PropTypes.bool, |
There was a problem hiding this comment.
"Pn" means positive/negative? could we update that variable name to be more descriptive to indicate that?
| @@ -1,5 +1,5 @@ | |||
| .slice-grid .widget.table .slice_container { | |||
There was a problem hiding this comment.
possibly good news: anything with .widget I think can be removed. this is what grid cells on the old dashboard used to be called.
| } | ||
| datatable.draw(); | ||
| container.parents('.widget').find('.tooltip').remove(); | ||
| $container.parents('.widget').find('.tooltip').remove(); |
There was a problem hiding this comment.
I would guess that this tried to remove tooltips from other dashboard charts 🤔
| pageLength: pageLength && parseInt(pageLength, 10), | ||
| percentMetrics, | ||
| // Aug 22, 2018 | ||
| // Perhaps this `tableFilter` field can be removed as there is |
There was a problem hiding this comment.
that's interesting. I think this table used to be able to function as a filter on dashboards but perhaps that broke at some point. @mistercrunch any thoughts?
|
Addressed comments, except the |
* update indent * extract formData and data. * take filter * remove dependencies * remove removeFilter * add comment * remove columnFormats and verboseMap from props. clarify a few more props * fix linting issue * minor syntax * syntax fix * Move check to adaptor * update unit test * remove code related to .widget * rename variables for clarity * move Option fix to browser.js (cherry picked from commit 8a4b1b7)
* update indent * extract formData and data. * take filter * remove dependencies * remove removeFilter * add comment * remove columnFormats and verboseMap from props. clarify a few more props * fix linting issue * minor syntax * syntax fix * Move check to adaptor * update unit test * remove code related to .widget * rename variables for clarity * move Option fix to browser.js
* update indent * extract formData and data. * take filter * remove dependencies * remove removeFilter * add comment * remove columnFormats and verboseMap from props. clarify a few more props * fix linting issue * minor syntax * syntax fix * Move check to adaptor * update unit test * remove code related to .widget * rename variables for clarity * move Option fix to browser.js
mistercrunch
added
🏷️ bot
sliceandformDataaccording to [SIP-5]@williaster @conglei @graceguo-supercat