Table expander checkbox #738
Conversation
Codecov Report
@@ Coverage Diff @@
## main #738 +/- ##
==========================================
- Coverage 85.20% 85.18% -0.02%
==========================================
Files 786 786
Lines 16097 16097
Branches 2053 2051 -2
==========================================
- Hits 13715 13713 -2
- Misses 2349 2350 +1
- Partials 33 34 +1
Continue to review full report at Codecov.
|
| tap(() => this.loadingStateSubject.next({ loading$: NEVER })), | ||
| /** | ||
| * Below debouce is needed to handle multiple emission from buildChangeObservable. | ||
| * Below debounce is needed to handle multiple emission from buildChangeObservable. |
There was a problem hiding this comment.
I assume this is the real reason for the PR and all this other stuff is just because you were working in the area.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
lgtm will let @jake-bassett take a look since he's more familiar
damn, he beat me |
| }) | ||
| export class TableCellStateParserCheckbox extends TableCellParserBase<TableRowState, TableRowState, undefined> { | ||
| public parseValue(_: TableRowState, rowData: StatefulTableRow): TableRowState { | ||
| return rowData.$$state; |
There was a problem hiding this comment.
I'm not sure how this is working. Shouldn't you need to map this to the specific property you are looking for? In this case, the boolean for selected?
There was a problem hiding this comment.
This approach looks right, so if its working, all good - approved. Just curious where the final mapping to the property is happening.
There was a problem hiding this comment.
The renderer actually depends on the entire state. I didn't change that part. Just using the row to find state object instead of depending on the cell value and deal with id conflicts
| @TableCellParser({ | ||
| type: CoreTableCellParserType.RowExpander | ||
| }) | ||
| export class TableCellStateParserExpander extends TableCellParserBase<TableRowState, TableRowState, undefined> { |
There was a problem hiding this comment.
Are these two parsers the same? Do we need to dupe it?
There was a problem hiding this comment.
They are same right now. I wanted to get feedback on whether I should make this return the whole object (more safer) or make it return the specific property. I can update this PR depending on this.
There was a problem hiding this comment.
I'm fine with either:
- one copy of this
- no copies (and just use the no-op parser and update the renderers to read state off the row)
I think splitting to two specific ones is overkill - it's not really safer because the renderer has the row anyway.
There was a problem hiding this comment.
Will move to one copy. Don't want to remove the parser.
This comment has been minimized.
This comment has been minimized.
Description
Please include a summary of the change, motivation and context.
Fix: Separating Table Expander and checkbox header renderers
Testing
Please describe the tests that you ran to verify your changes. Please summarize what did you test and what needs to be tested e.g. deployed and tested helm chart locally.
Checklist:
Documentation
Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.