Convert EuiTableOfRecords to simpler table component in scripted fields table. #16901
Conversation
cjcenizal
changed the title
cjcenizal
force-pushed
the
remove-table-of-records
branch
from
eb1537c to
3a1dca5
Compare
💚 Build Succeeded |
There was a problem hiding this comment.
Awesome! This looks great! Seeing how easy this is, I'm going to go ahead and update #16649 to use the new implementation!
I found one bug but other than that, looks great!
| this.setState(this.computeTableState(this.state.criteria, nextProps)); | ||
| if (fieldFilter) { | ||
| const normalizedFieldFilter = this.props.fieldFilter.toLowerCase(); | ||
| return fields.filter(field => field.name.toLowerCase().includes(normalizedFieldFilter)); |
There was a problem hiding this comment.
So because we are doing a return here, we aren't applying both filters consistently so we end up in a state like:
Where we are only looking at a certain scripted field language but it's showing others because on the initial filter match
There was a problem hiding this comment.
After talking this through with @cjcenizal, I think I was promoting a poor pattern of storing computed data which makes it hard to manage the original data set as well as the computed set. It doesn't seem like a good idea to store them both so I think we should move to a pattern where we only store the original data set and calculate the computed data during the render() method. To help on performance of potentially recalculating computed data unnecessarily, let's use reselect which is a tiny library designed to only call a function when the parameters have changed.
See 56e76d2
cjcenizal
force-pushed
the
remove-table-of-records
branch
from
3a1dca5 to
f4cb8fe
Compare
|
@chrisronline I addressed your feedback, but I didn't use your reselect suggestion. Can you take another look? Are you OK if we implement reselect in another PR, just so I can get some time to familiarize myself with it? |
💔 Build Failed |
💚 Build Succeeded |
There was a problem hiding this comment.
LGTM!!
The reselect optimization isn't always necessary so it's totally fine to exclude for now. If the data set will always be small, it's an unnecessary optimization but the implementation is fairly trivial and straight forward so we can revisit later and quickly add if desired.
cjcenizal
force-pushed
the
remove-table-of-records
branch
from
0d6a1d3 to
bc28cb3
Compare
💚 Build Succeeded |
…ds table. (elastic#16901) * Convert EuiTableOfRecords to EuiBasicTable in scripted fields table. * Convert from EuiBasicTable to EuiInMemoryTable. * Fix bug with filtering and show 'no items' message when the filter doens't match any scripted fields.
Switching to
EuiBasicTableresults in a 12% reduction in code... not bad.Aaaaand switching to
EuiInMemoryTablereduced the code by 48%.