feat: Uses new table component in Drill to Detail #22173
Conversation
michael-s-molina
force-pushed
the
change-d2d-table
branch
from
7b041c6
to
9d24766
Compare
michael-s-molina
force-pushed
the
change-d2d-table
branch
from
9d24766
to
c146e73
Compare
|
@eric-briscoe I noticed an error in the browser console when paginating. It seems an internal table error. Can you check? |
| expect(screen.getByText('2022-01-01')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| test('renders with number', async () => { |
There was a problem hiding this comment.
@michael-s-molina can we add tests that pass a mixture of null | undefined| invalid time formats as we as mix of invalid DATE values to ensure the cell renderer does not throw runtime error and break app in case a single row has bad / missing data? I think we want to make cell renderers fault tolerant especially when doing error prone translations like dates. antd does not give us an error protection on cell rendering so we have to make sure our cell renderers do not throw error that will break the render loop for entire table component / app
There was a problem hiding this comment.
Good idea. Added ✅
| const timeFormatter = getTimeFormatter(format); | ||
| return <span>{timeFormatter.format(value)}</span>; |
There was a problem hiding this comment.
@michael-s-molina pairing this with comment on superset-frontend/src/components/Table/cell-renderers/TimeCell/TimeCell.test.tsx for testing with bad data formats / formats value mismatch to make sure we are handling any potential errors due to bad dat values for a row
Codecov Report
@@ Coverage Diff @@
## master #22173 +/- ##
==========================================
+ Coverage 66.86% 66.89% +0.03%
==========================================
Files 1840 1845 +5
Lines 70160 70281 +121
Branches 7657 7683 +26
==========================================
+ Hits 46909 47012 +103
+ Misses 21281 21277 -4
- Partials 1970 1992 +22
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 |
|
/testenv up |
|
@geido Ephemeral environment spinning up at http://35.92.48.144:8080. Credentials are |
|
/testenv up FEATURE_DRILL_TO_DETAIL=true |
|
@geido Ephemeral environment spinning up at http://52.41.165.144:8080. Credentials are |
| { label: t('Formatted value'), value: 'formatted' }, | ||
| ]} | ||
| value={ | ||
| timeFormatting[column] === 'original' ? 'original' : 'formatted' |
There was a problem hiding this comment.
What do you think about using an Enum for these two values?
There was a problem hiding this comment.
This looks great! One issue I'm seeing is that the fixed-height table doesn't play well with the adjustable-height modal:
Is there any way to make the table adjustable height, so it fills the modal vertically when expanded and leaves the pagination controls visible when minimizing? If not, should we maybe make the modal height fixed to match the fixed height of the table?
|
Ephemeral environment shutdown and build artifacts deleted. |
mistercrunch
added
🏷️ bot
SUMMARY
Uses the new table component in Drill to Detail to benefit from virtualization and deal with tables with many columns.
It also extends the new Table component with new cell and header renderers.
@eric-briscoe
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Screen.Recording.2022-11-28.at.10.03.11.AM.mov
Screen.Recording.2022-11-28.at.9.58.38.AM.mov
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION