EnxDev commented on code in PR #37625:
URL: https://github.com/apache/superset/pull/37625#discussion_r2767693849
##########
superset-frontend/src/dashboard/components/dnd/DragDroppable.test.tsx:
##########
@@ -102,23 +105,23 @@ describe('DragDroppable', () => {
});
test('should call its child function with "dropIndicatorProps" dependent on
editMode and isDraggingOver', () => {
- const renderChild = jest.fn(provided => (
+ const renderChild = jest.fn((provided: Record<string, unknown>) => (
<div data-test="child-content" {...provided}>
Test Content
</div>
));
// Create a mock component with the dropIndicator state already set
class MockDragDroppable extends DragDroppable {
- constructor(props) {
- super(props);
- this.state = { dropIndicator: true };
+ constructor(mockProps: ConstructorParameters<typeof DragDroppable>[0]) {
+ super(mockProps);
+ this.state = { dropIndicator: 'DROP_TOP' as any };
}
}
render(
<MockDragDroppable
- {...props}
+ {...(props as any)}
Review Comment:
Do you think it would be helpful to tighten the props typing here?
Otherwise, the test could pass invalid prop shapes without TypeScript flagging
it
##########
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx:
##########
@@ -168,55 +184,74 @@ const Chart = props => {
[dispatch],
);
- const chart = useSelector(state => state.charts[props.id] || EMPTY_OBJECT);
- const { queriesResponse, chartUpdateEndTime, chartStatus, annotationQuery } =
- chart;
+ const chart = useSelector((state: RootState) => state.charts[props.id]);
+ const queriesResponse = chart?.queriesResponse;
+ const chartUpdateEndTime = chart?.chartUpdateEndTime;
+ const chartStatus = chart?.chartStatus;
+ const annotationQuery = chart?.annotationQuery;
- const slice = useSelector(
- state => state.sliceEntities.slices[props.id] || EMPTY_OBJECT,
+ const slice: SliceEntity | Record<string, never> = useSelector(
+ (state: RootState) =>
+ (state.sliceEntities as { slices: Record<number, SliceEntity> }).slices[
+ props.id
+ ] || EMPTY_OBJECT,
+ );
+ const sliceVizType = (slice as SliceEntity)?.viz_type;
+ const sliceSliceId = (slice as SliceEntity)?.slice_id;
+ const sliceSliceName = (slice as SliceEntity)?.slice_name;
+ const editMode = useSelector(
+ (state: RootState) => state.dashboardState.editMode,
Review Comment:
Could we type sliceEntities properly in RootState instead of casting here?
The `Record<string, never>` fallback also makes downstream property access
unsafe
##########
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx:
##########
@@ -700,37 +737,39 @@ const Chart = props => {
<ChartContainer
width={width}
height={getChartHeight()}
- addFilter={addFilter}
+ addFilter={addFilter as unknown as (type: string) => void}
Review Comment:
It looks like the double cast may indicate a mismatch between addFilter’s
signature and what ChartContainer expects. Would it be possible to align the
types? If not, a wrapper function could be a safer alternative to as unknown as
##########
superset-frontend/src/dashboard/components/gridComponents/Column/Column.tsx:
##########
@@ -109,7 +115,7 @@ const ColumnStyles = styled.div`
`}
`;
-const emptyColumnContentStyles = theme => css`
+const emptyColumnContentStyles = (theme: import('@emotion/react').Theme) =>
css`
Review Comment:
Would it make sense to use `SupersetTheme` from `@superset-ui/core` instead
of the inline Emotion Theme import? That’s the usual pattern in the codebase
and provides Superset-specific theme tokens
##########
superset-frontend/spec/fixtures/mockNativeFilters.ts:
##########
@@ -132,7 +132,7 @@ export const NATIVE_FILTER_ID = 'NATIVE_FILTER-p4LImrSgA';
export const singleNativeFiltersState = {
filters: {
[NATIVE_FILTER_ID]: {
- id: [NATIVE_FILTER_ID],
+ id: NATIVE_FILTER_ID,
Review Comment:
Is this change intended?
##########
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx:
##########
@@ -390,25 +440,29 @@ const Chart = props => {
const formData = useMemo(
() =>
getFormDataWithExtraFilters({
- chart: { id: chart.id, form_data: chart.form_data }, // avoid passing
the whole chart object
+ chart: { id: chart?.id ?? props.id, form_data: chart?.form_data }, //
avoid passing the whole chart object
chartConfiguration,
- chartCustomizationItems,
+ chartCustomizationItems:
+ chartCustomizationItems as
import('@superset-ui/core').ChartCustomization[],
Review Comment:
Could we import ChartCustomization at the top instead of using inline import
syntax? Also, why does `chartCustomizationItems` need this cast; is the source
type wrong?
##########
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx:
##########
@@ -660,11 +692,14 @@ const Chart = props => {
addDangerToast={boundActionCreators.addDangerToast}
handleToggleFullSize={props.handleToggleFullSize}
isFullSize={props.isFullSize}
- chartStatus={chartStatus}
- formData={formData}
+ chartStatus={chartStatus || ''}
+ formData={
+ formData as unknown as import('@superset-ui/core').QueryFormData
Review Comment:
here as well
##########
superset-frontend/src/dashboard/components/gridComponents/Markdown/Markdown.tsx:
##########
@@ -32,47 +32,63 @@ import HoverMenu from
'src/dashboard/components/menu/HoverMenu';
import ResizableContainer from
'src/dashboard/components/resizable/ResizableContainer';
import MarkdownModeDropdown from
'src/dashboard/components/menu/MarkdownModeDropdown';
import WithPopoverMenu from 'src/dashboard/components/menu/WithPopoverMenu';
-import { componentShape } from 'src/dashboard/util/propShapes';
+import type { LayoutItem } from 'src/dashboard/types';
+import type { DropResult } from
'src/dashboard/components/dnd/dragDroppableConfig';
import { ROW_TYPE, COLUMN_TYPE } from 'src/dashboard/util/componentTypes';
import {
GRID_MIN_COLUMN_COUNT,
GRID_MIN_ROW_UNITS,
GRID_BASE_UNIT,
} from 'src/dashboard/util/constants';
-const propTypes = {
- id: PropTypes.string.isRequired,
- parentId: PropTypes.string.isRequired,
- component: componentShape.isRequired,
- parentComponent: componentShape.isRequired,
- index: PropTypes.number.isRequired,
- depth: PropTypes.number.isRequired,
- editMode: PropTypes.bool.isRequired,
-
- // from redux
- logEvent: PropTypes.func.isRequired,
- addDangerToast: PropTypes.func.isRequired,
- undoLength: PropTypes.number.isRequired,
- redoLength: PropTypes.number.isRequired,
+interface EditorInstance {
+ resize?: (force: boolean) => void;
+ getSession?: () => { setUseWrapMode: (wrap: boolean) => void };
+ focus?: () => void;
+}
+
+interface MarkdownOwnProps {
+ id: string;
+ parentId: string;
+ component: LayoutItem;
+ parentComponent: LayoutItem;
+ index: number;
+ depth: number;
+ editMode: boolean;
// grid related
- availableColumnCount: PropTypes.number.isRequired,
- columnWidth: PropTypes.number.isRequired,
- onResizeStart: PropTypes.func.isRequired,
- onResize: PropTypes.func.isRequired,
- onResizeStop: PropTypes.func.isRequired,
+ availableColumnCount: number;
+ columnWidth: number;
+ onResizeStart: import('re-resizable').ResizeStartCallback;
+ onResize: import('re-resizable').ResizeCallback;
Review Comment:
same here
##########
superset-frontend/src/dashboard/actions/hydrate.ts:
##########
@@ -146,16 +205,19 @@ export const hydrateDashboard =
{
chartId: slice.slice_id,
},
- (newSlicesContainer.parents || []).slice(),
+ (newSlicesContainer!.parents || []).slice(),
);
const count = (slicesFromExploreCount.get(slice.slice_id) ?? 0) + 1;
chartHolder.id = `${CHART_TYPE}-explore-${slice.slice_id}-${count}`;
slicesFromExploreCount.set(slice.slice_id, count);
layout[chartHolder.id] = chartHolder;
- newSlicesContainer.children.push(chartHolder.id);
- chartIdToLayoutId[chartHolder.meta.chartId] = chartHolder.id;
+ newSlicesContainer!.children.push(chartHolder.id);
+ const holderChartId = chartHolder.meta.chartId;
+ if (holderChartId !== undefined) {
+ chartIdToLayoutId[holderChartId] = chartHolder.id;
+ }
Review Comment:
Do we know if this code can run when `newSlicesContainer` isn’t assigned? If
it can’t, maybe we could keep these lines inside the conditional block to avoid
using `!`
--
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]
