msyavuz commented on code in PR #37112:
URL: https://github.com/apache/superset/pull/37112#discussion_r2704018136
##########
superset-frontend/src/dashboard/components/SliceHeader/SliceHeader.test.tsx:
##########
@@ -677,8 +671,187 @@ test('Should show RowCountLabel in embedded when
uiConfig.showRowLimitWarning is
initialState: rowCountState,
});
- expect(screen.getByTestId('warning')).toBeInTheDocument();
+ expect(screen.queryByTestId('warning')).not.toBeInTheDocument();
+ expect(screen.queryByTestId('row-count-label')).not.toBeInTheDocument();
mockIsEmbedded.mockRestore();
mockUseUiConfig.mockRestore();
});
+
+test('Should show row count badge for table chart without server pagination',
() => {
+ const mockUseUiConfig = useUiConfig as jest.MockedFunction<
+ typeof useUiConfig
+ >;
+ mockUseUiConfig.mockReturnValue({
+ hideTitle: false,
+ hideTab: false,
+ hideNav: false,
+ hideChartControls: false,
+ emitDataMasks: false,
+ showRowLimitWarning: true,
+ });
+
+ const props = createProps({
+ formData: {
+ ...createProps().formData,
+ viz_type: VizType.Table,
+ row_limit: 10,
+ },
+ slice: {
+ ...createProps().slice,
+ form_data: {
+ ...createProps().slice.form_data,
+ viz_type: VizType.Table,
+ row_limit: 10,
+ },
+ viz_type: VizType.Table,
+ },
Review Comment:
Also why are we singling out the row limit for table charts?
##########
superset-frontend/src/dashboard/components/SliceHeader/index.tsx:
##########
@@ -304,18 +316,20 @@ const SliceHeader = forwardRef<HTMLDivElement,
SliceHeaderProps>(
<FiltersBadge chartId={slice.slice_id} />
)}
- {shouldShowRowLimitWarning && sqlRowCount === rowLimit && (
+ {showRowCountLabel && (
<RowCountLabel
rowcount={sqlRowCount}
limit={rowLimit}
label={
- <Icons.WarningOutlined
- iconSize="l"
- iconColor={theme.colorWarning}
- css={theme => css`
- padding: ${theme.sizeUnit}px;
- `}
- />
+ showRowLimitWarning ? (
+ <Icons.WarningOutlined
+ iconSize="l"
+ iconColor={theme.colorWarning}
+ css={theme => css`
+ padding: ${theme.sizeUnit}px;
+ `}
+ />
+ ) : undefined
Review Comment:
I think this is one level higher than where it should be. RowCountLabel
already has the row limit warning check implemented and this icon is displayed
according to that. Can we check if we need to modify the logic inside
`RowCountLabel` instead?
##########
superset-frontend/src/dashboard/components/SliceHeader/index.tsx:
##########
@@ -186,14 +186,24 @@ const SliceHeader = forwardRef<HTMLDivElement,
SliceHeaderProps>(
({ dashboardInfo }) => dashboardInfo.crossFiltersEnabled,
);
- const firstQueryResponse = useSelector<RootState, QueryData | undefined>(
- state => state.charts[slice.slice_id].queriesResponse?.[0],
+ const queriesResponse = useSelector<RootState, QueryData[] | null |
undefined>(
+ state => state.charts[slice.slice_id].queriesResponse,
);
const theme = useTheme();
- const rowLimit = Number(formData.row_limit || -1);
- const sqlRowCount = Number(firstQueryResponse?.sql_rowcount || 0);
+ const rowLimit = Number(formData.row_limit ?? 0);
Review Comment:
I implemented this in [this
pr](https://github.com/apache/superset/pull/33781). Can we check if the
functionality there haven't regressed? This -1 was for that.
##########
superset-frontend/src/dashboard/components/SliceHeader/SliceHeader.test.tsx:
##########
@@ -565,7 +565,7 @@ test('Add extension to SliceHeader', () => {
expect(screen.getByText('This is an extension')).toBeInTheDocument();
});
-test('Should render RowCountLabel when row limit is hit, and hide it
otherwise', () => {
+test('Should NOT render warning for non-table chart when row limit is hit', ()
=> {
Review Comment:
It seems a bit wrong to change this. I don't think the requirements have
changed for it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
