From 6a0b46e54dac7591497a096520271c5ded5d4f9b Mon Sep 17 00:00:00 2001 From: Uladzimir Havenchyk <125459446+uladzimirdev@users.noreply.github.com> Date: Fri, 18 Oct 2024 20:48:10 +0300 Subject: [PATCH] rework ChartSettings to typescript (#48877) * rework ChartSettings to typescript * lint --- .../query_builder/components/view/View.jsx | 2 +- ...gsSidebar.jsx => ChartSettingsSidebar.tsx} | 36 ++-- .../ChartSettingsSidebar.unit.spec.js | 2 +- .../{ChartSettings.jsx => ChartSettings.tsx} | 158 +++++++++++++----- .../components/ChartSettings.unit.spec.tsx | 29 ++-- ...tNestedSettingsSeriesMultiple.unit.spec.js | 2 +- .../ChartSettingFieldPicker.unit.spec.js | 2 +- .../settings/ChartSettingStacked.unit.spec.js | 2 +- .../visualizations/BarChart.unit.spec.tsx | 2 +- .../PivotTable/PivotTable.unit.spec.js | 2 +- .../visualizations/Table.unit.spec.js | 2 +- 11 files changed, 161 insertions(+), 78 deletions(-) rename frontend/src/metabase/query_builder/components/view/sidebars/{ChartSettingsSidebar.jsx => ChartSettingsSidebar.tsx} (61%) rename frontend/src/metabase/visualizations/components/{ChartSettings.jsx => ChartSettings.tsx} (75%) diff --git a/frontend/src/metabase/query_builder/components/view/View.jsx b/frontend/src/metabase/query_builder/components/view/View.jsx index 1383b0fe6ee..53efb1c8214 100644 --- a/frontend/src/metabase/query_builder/components/view/View.jsx +++ b/frontend/src/metabase/query_builder/components/view/View.jsx @@ -47,7 +47,7 @@ import { } from "./View.styled"; import { ViewFooter } from "./ViewFooter"; import ViewSidebar from "./ViewSidebar"; -import ChartSettingsSidebar from "./sidebars/ChartSettingsSidebar"; +import { ChartSettingsSidebar } from "./sidebars/ChartSettingsSidebar"; import { ChartTypeSidebar } from "./sidebars/ChartTypeSidebar"; import { QuestionInfoSidebar } from "./sidebars/QuestionInfoSidebar"; import { QuestionSettingsSidebar } from "./sidebars/QuestionSettingsSidebar"; diff --git a/frontend/src/metabase/query_builder/components/view/sidebars/ChartSettingsSidebar.jsx b/frontend/src/metabase/query_builder/components/view/sidebars/ChartSettingsSidebar.tsx similarity index 61% rename from frontend/src/metabase/query_builder/components/view/sidebars/ChartSettingsSidebar.jsx rename to frontend/src/metabase/query_builder/components/view/sidebars/ChartSettingsSidebar.tsx index 6b622974017..827b999166f 100644 --- a/frontend/src/metabase/query_builder/components/view/sidebars/ChartSettingsSidebar.jsx +++ b/frontend/src/metabase/query_builder/components/view/sidebars/ChartSettingsSidebar.tsx @@ -1,22 +1,30 @@ -/* eslint-disable react/prop-types */ -import { memo, useCallback, useMemo, useState } from "react"; +import { memo, useCallback, useMemo } from "react"; import { t } from "ttag"; import ErrorBoundary from "metabase/ErrorBoundary"; import CS from "metabase/css/core/index.css"; import SidebarContent from "metabase/query_builder/components/SidebarContent"; import visualizations from "metabase/visualizations"; -import ChartSettings from "metabase/visualizations/components/ChartSettings"; +import { + ChartSettings, + type Widget, +} from "metabase/visualizations/components/ChartSettings"; +import type Question from "metabase-lib/v1/Question"; +import type { Dataset, VisualizationSettings } from "metabase-types/api"; -function ChartSettingsSidebar(props) { - const [sidebarPropsOverride, setSidebarPropsOverride] = useState(null); - const handleSidebarPropsOverride = useCallback( - sidebarPropsOverride => { - setSidebarPropsOverride(sidebarPropsOverride); - }, - [setSidebarPropsOverride], - ); +interface ChartSettingsSidebarProps { + question: Question; + result: Dataset; + addField: () => void; + initialChartSetting: { section: string; widget?: Widget }; + onReplaceAllVisualizationSettings: (settings: VisualizationSettings) => void; + onClose: () => void; + onOpenChartType: () => void; + visualizationSettings: VisualizationSettings; + showSidebarTitle?: boolean; +} +function ChartSettingsSidebarInner(props: ChartSettingsSidebarProps) { const { question, result, @@ -30,7 +38,7 @@ function ChartSettingsSidebar(props) { } = props; const sidebarContentProps = showSidebarTitle ? { - title: t`${visualizations.get(question.display()).uiName} options`, + title: t`${visualizations.get(question.display())?.uiName} options`, onBack: () => onOpenChartType(), } : {}; @@ -55,7 +63,6 @@ function ChartSettingsSidebar(props) { className={CS.fullHeight} onDone={handleClose} {...sidebarContentProps} - {...sidebarPropsOverride} > <ErrorBoundary> <ChartSettings @@ -66,7 +73,6 @@ function ChartSettingsSidebar(props) { onClose={handleClose} noPreview initial={initialChartSetting} - setSidebarPropsOverride={handleSidebarPropsOverride} computedSettings={visualizationSettings} /> </ErrorBoundary> @@ -75,4 +81,4 @@ function ChartSettingsSidebar(props) { ); } -export default memo(ChartSettingsSidebar); +export const ChartSettingsSidebar = memo(ChartSettingsSidebarInner); diff --git a/frontend/src/metabase/query_builder/components/view/sidebars/ChartSettingsSidebar.unit.spec.js b/frontend/src/metabase/query_builder/components/view/sidebars/ChartSettingsSidebar.unit.spec.js index c8c050330c2..e37c184138b 100644 --- a/frontend/src/metabase/query_builder/components/view/sidebars/ChartSettingsSidebar.unit.spec.js +++ b/frontend/src/metabase/query_builder/components/view/sidebars/ChartSettingsSidebar.unit.spec.js @@ -1,7 +1,7 @@ import { fireEvent, render, screen } from "@testing-library/react"; import { createMockMetadata } from "__support__/metadata"; -import ChartSettingsSidebar from "metabase/query_builder/components/view/sidebars/ChartSettingsSidebar"; +import { ChartSettingsSidebar } from "metabase/query_builder/components/view/sidebars/ChartSettingsSidebar"; import registerVisualizations from "metabase/visualizations/register"; import { SAMPLE_DB_ID, diff --git a/frontend/src/metabase/visualizations/components/ChartSettings.jsx b/frontend/src/metabase/visualizations/components/ChartSettings.tsx similarity index 75% rename from frontend/src/metabase/visualizations/components/ChartSettings.jsx rename to frontend/src/metabase/visualizations/components/ChartSettings.tsx index db01e735f6d..8560873b7a1 100644 --- a/frontend/src/metabase/visualizations/components/ChartSettings.jsx +++ b/frontend/src/metabase/visualizations/components/ChartSettings.tsx @@ -1,4 +1,3 @@ -/* eslint-disable react/prop-types */ import { assocIn } from "icepick"; import { Component } from "react"; import * as React from "react"; @@ -23,7 +22,18 @@ import { import { getSettingDefinitionsForColumn } from "metabase/visualizations/lib/settings/column"; import { keyForSingleSeries } from "metabase/visualizations/lib/settings/series"; import { getSettingsWidgetsForSeries } from "metabase/visualizations/lib/settings/visualization"; +import type Question from "metabase-lib/v1/Question"; import { getColumnKey } from "metabase-lib/v1/queries/utils/column-key"; +import type { + Dashboard, + DashboardCard, + DatasetColumn, + RawSeries, + Series, + VisualizationSettings, +} from "metabase-types/api"; + +import type { ComputedVisualizationSettings } from "../types"; import { ChartSettingsFooterRoot, @@ -44,20 +54,25 @@ const DEFAULT_TAB_PRIORITY = [t`Data`]; /** * @deprecated HOCs are deprecated */ -const withTransientSettingState = ComposedComponent => - class extends React.Component { +function withTransientSettingState( + ComposedComponent: React.ComponentType<ChartSettingsProps>, +) { + return class extends React.Component< + ChartSettingsProps, + { settings?: VisualizationSettings } + > { static displayName = `withTransientSettingState[${ ComposedComponent.displayName || ComposedComponent.name }]`; - constructor(props) { + constructor(props: ChartSettingsProps) { super(props); this.state = { settings: props.settings, }; } - UNSAFE_componentWillReceiveProps(nextProps) { + UNSAFE_componentWillReceiveProps(nextProps: ChartSettingsProps) { if (this.props.settings !== nextProps.settings) { this.setState({ settings: nextProps.settings }); } @@ -68,17 +83,61 @@ const withTransientSettingState = ComposedComponent => <ComposedComponent {...this.props} settings={this.state.settings} - onChange={settings => this.setState({ settings })} - onDone={settings => - this.props.onChange(settings || this.state.settings) + onChange={(settings: VisualizationSettings) => + this.setState({ settings }) + } + onDone={(settings: VisualizationSettings) => + this.props.onChange?.(settings || this.state.settings) } /> ); } }; +} + +// this type is not full, we need to extend it later +export interface Widget { + id: string; + section: string; + hidden?: boolean; + props: Record<string, unknown>; + title?: string; + widget: (() => JSX.Element | null) | undefined; +} -class ChartSettings extends Component { - constructor(props) { +export interface ChartSettingsProps { + className?: string; + dashboard?: Dashboard; + dashcard?: DashboardCard; + initial?: { section: string; widget?: Widget }; + onCancel?: () => void; + onDone?: (settings: VisualizationSettings) => void; + onReset?: () => void; + onChange?: ( + settings: ComputedVisualizationSettings, + question?: Question, + ) => void; + onClose?: () => void; + rawSeries?: RawSeries[]; + settings?: VisualizationSettings; + widgets?: Widget[]; + series: Series; + computedSettings?: ComputedVisualizationSettings; + isDashboard?: boolean; + question?: Question; + addField?: () => void; + noPreview?: boolean; +} + +interface ChartSettingsState { + currentSection: string | null; + currentWidget: Widget | null; + popoverRef?: HTMLElement | null; + warnings?: string[]; +} + +class ChartSettings extends Component<ChartSettingsProps, ChartSettingsState> { + constructor(props: ChartSettingsProps) { super(props); this.state = { currentSection: (props.initial && props.initial.section) || null, @@ -86,7 +145,7 @@ class ChartSettings extends Component { }; } - componentDidUpdate(prevProps) { + componentDidUpdate(prevProps: ChartSettingsProps) { const { initial } = this.props; if (!_.isEqual(initial, prevProps.initial)) { this.setState({ @@ -96,7 +155,7 @@ class ChartSettings extends Component { } } - handleShowSection = section => { + handleShowSection = (section: string) => { this.setState({ currentSection: section, currentWidget: null, @@ -104,7 +163,7 @@ class ChartSettings extends Component { }; // allows a widget to temporarily replace itself with a different widget - handleShowWidget = (widget, ref) => { + handleShowWidget = (widget: Widget, ref: HTMLElement | null) => { this.setState({ popoverRef: ref, currentWidget: widget }); }; @@ -115,32 +174,38 @@ class ChartSettings extends Component { handleResetSettings = () => { const originalCardSettings = - this.props.dashcard.card.visualization_settings; + this.props.dashcard?.card.visualization_settings; const clickBehaviorSettings = getClickBehaviorSettings(this._getSettings()); - this.props.onChange({ ...originalCardSettings, ...clickBehaviorSettings }); + this.props.onChange?.({ + ...originalCardSettings, + ...clickBehaviorSettings, + }); }; - handleChangeSettings = (changedSettings, question) => { - this.props.onChange( + handleChangeSettings = ( + changedSettings: VisualizationSettings, + question: Question, + ) => { + this.props.onChange?.( updateSettings(this._getSettings(), changedSettings), question, ); }; - handleChangeSeriesColor = (seriesKey, color) => { - this.props.onChange( + handleChangeSeriesColor = (seriesKey: string, color: string) => { + this.props.onChange?.( updateSeriesColor(this._getSettings(), seriesKey, color), ); }; handleDone = () => { - this.props.onDone(this._getSettings()); - this.props.onClose(); + this.props.onDone?.(this._getSettings()); + this.props.onClose?.(); }; handleCancel = () => { - this.props.onClose(); + this.props.onClose?.(); }; _getSettings() { @@ -153,7 +218,7 @@ class ChartSettings extends Component { return this.props.computedSettings || {}; } - _getWidgets() { + _getWidgets(): Widget[] { if (this.props.widgets) { return this.props.widgets; } else { @@ -188,7 +253,7 @@ class ChartSettings extends Component { return transformedSeries; } - columnHasSettings(col) { + columnHasSettings(col: DatasetColumn) { const { series } = this.props; const settings = this._getSettings() || {}; const settingsDefs = getSettingDefinitionsForColumn(series, col); @@ -206,7 +271,7 @@ class ChartSettings extends Component { ).some(widget => !widget.hidden); } - getStyleWidget = widgets => { + getStyleWidget = (widgets: Widget[]): Widget | null => { const series = this._getTransformedSeries(); const settings = this._getComputedSettings(); const { currentWidget } = this.state; @@ -235,7 +300,8 @@ class ChartSettings extends Component { }, }; } else if (currentWidget.props?.initialKey) { - const hasBreakouts = settings["graph.dimensions"]?.length > 1; + const hasBreakouts = + settings["graph.dimensions"] && settings["graph.dimensions"].length > 1; if (hasBreakouts) { return null; @@ -244,7 +310,9 @@ class ChartSettings extends Component { const singleSeriesForColumn = series.find(single => { const metricColumn = single.data.cols[1]; if (metricColumn) { - return getColumnKey(metricColumn) === currentWidget.props.initialKey; + return ( + getColumnKey(metricColumn) === currentWidget?.props?.initialKey + ); } }); @@ -262,7 +330,7 @@ class ChartSettings extends Component { return null; }; - getFormattingWidget = widgets => { + getFormattingWidget = (widgets: Widget[]): Widget | null => { const { currentWidget } = this.state; const widget = currentWidget && widgets.find(widget => widget.id === currentWidget.id); @@ -279,10 +347,10 @@ class ChartSettings extends Component { className, question, addField, - noPreview, + noPreview = false, dashboard, dashcard, - isDashboard, + isDashboard = false, } = this.props; const { popoverRef } = this.state; @@ -290,8 +358,8 @@ class ChartSettings extends Component { const widgets = this._getWidgets(); const rawSeries = this._getRawSeries(); - const widgetsById = {}; - const sections = {}; + const widgetsById: Record<string, Widget> = {}; + const sections: Record<string, Widget[]> = {}; for (const widget of widgets) { widgetsById[widget.id] = widget; @@ -338,7 +406,7 @@ class ChartSettings extends Component { // overriding the sidebar title. const currentSectionHasColumnSettings = ( sections[currentSection] || [] - ).some(widget => widget.id === "column_settings"); + ).some((widget: Widget) => widget.id === "column_settings"); const extraWidgetProps = { // NOTE: special props to support adding additional fields @@ -347,8 +415,8 @@ class ChartSettings extends Component { onShowWidget: this.handleShowWidget, onEndShowWidget: this.handleEndShowWidget, currentSectionHasColumnSettings, - columnHasSettings: col => this.columnHasSettings(col), - onChangeSeriesColor: (seriesKey, color) => + columnHasSettings: (col: DatasetColumn) => this.columnHasSettings(col), + onChangeSeriesColor: (seriesKey: string, color: string) => this.handleChangeSeriesColor(seriesKey, color), }; @@ -409,7 +477,9 @@ class ChartSettings extends Component { isSettings showWarnings onUpdateVisualizationSettings={this.handleChangeSettings} - onUpdateWarnings={warnings => this.setState({ warnings })} + onUpdateWarnings={(warnings: string[]) => + this.setState({ warnings }) + } /> </ChartSettingsVisualizationContainer> <ChartSettingsFooter @@ -420,11 +490,11 @@ class ChartSettings extends Component { </ChartSettingsPreview> )} <ChartSettingsWidgetPopover - anchor={popoverRef} + anchor={popoverRef as HTMLElement} widgets={[ this.getStyleWidget(widgets), this.getFormattingWidget(widgets), - ].filter(widget => !!widget)} + ].filter((widget): widget is Widget => !!widget)} handleEndShowWidget={this.handleEndShowWidget} /> </ChartSettingsRoot> @@ -432,7 +502,15 @@ class ChartSettings extends Component { } } -const ChartSettingsFooter = ({ onDone, onCancel, onReset }) => ( +const ChartSettingsFooter = ({ + onDone, + onCancel, + onReset, +}: { + onDone: () => void; + onCancel: () => void; + onReset: (() => void) | null; +}) => ( <ChartSettingsFooterRoot> {onReset && ( <Button @@ -446,6 +524,6 @@ const ChartSettingsFooter = ({ onDone, onCancel, onReset }) => ( </ChartSettingsFooterRoot> ); -export default ChartSettings; +export { ChartSettings }; export const ChartSettingsWithState = withTransientSettingState(ChartSettings); diff --git a/frontend/src/metabase/visualizations/components/ChartSettings.unit.spec.tsx b/frontend/src/metabase/visualizations/components/ChartSettings.unit.spec.tsx index a40b64e68ad..9703c7d48ef 100644 --- a/frontend/src/metabase/visualizations/components/ChartSettings.unit.spec.tsx +++ b/frontend/src/metabase/visualizations/components/ChartSettings.unit.spec.tsx @@ -1,44 +1,43 @@ import userEvent from "@testing-library/user-event"; import { fireEvent, renderWithProviders, screen } from "__support__/ui"; -import ChartSettings from "metabase/visualizations/components/ChartSettings"; +import { + ChartSettings, + type ChartSettingsProps, + type Widget, +} from "metabase/visualizations/components/ChartSettings"; import registerVisualizations from "metabase/visualizations/register"; -import type { DashboardCard, Series } from "metabase-types/api"; import { createMockCard, createMockDashboardCard, + createMockDataset, createMockVisualizationSettings, } from "metabase-types/api/mocks"; -import type { ComputedVisualizationSettings } from "../types"; - registerVisualizations(); const DEFAULT_PROPS = { series: [ - { card: { visualization_settings: {} }, data: { rows: [], cols: [] } }, + { + card: createMockCard({ visualization_settings: {} }), + ...createMockDataset({ data: { rows: [], cols: [] } }), + }, ], settings: {}, }; -function widget(widget = {}) { +function widget(widget: Partial<Widget> = {}): Widget { return { id: "id-" + Math.random(), title: "title-" + Math.random(), widget: () => null, + section: "section-" + Math.random(), + props: {}, ...widget, }; } -// TODO Use ChartSettingsProps after ChartSettings is converted to TypeScript -type SetupOpts = { - series?: Series; - settings?: ComputedVisualizationSettings; - dashcard?: DashboardCard; - widgets?: any[]; - initial?: { section: string }; - onChange?: (settings: ComputedVisualizationSettings) => void; -}; +type SetupOpts = Partial<ChartSettingsProps>; const setup = (props: SetupOpts) => { return renderWithProviders(<ChartSettings {...DEFAULT_PROPS} {...props} />); diff --git a/frontend/src/metabase/visualizations/components/settings/ChartNestedSettingsSeriesMultiple.unit.spec.js b/frontend/src/metabase/visualizations/components/settings/ChartNestedSettingsSeriesMultiple.unit.spec.js index 7ac0e2e1524..b45bbc68640 100644 --- a/frontend/src/metabase/visualizations/components/settings/ChartNestedSettingsSeriesMultiple.unit.spec.js +++ b/frontend/src/metabase/visualizations/components/settings/ChartNestedSettingsSeriesMultiple.unit.spec.js @@ -2,7 +2,7 @@ import userEvent from "@testing-library/user-event"; import { renderWithProviders, screen, within } from "__support__/ui"; -import ChartSettings from "metabase/visualizations/components/ChartSettings"; +import { ChartSettings } from "metabase/visualizations/components/ChartSettings"; import registerVisualizations from "metabase/visualizations/register"; registerVisualizations(); diff --git a/frontend/src/metabase/visualizations/components/settings/ChartSettingFieldPicker.unit.spec.js b/frontend/src/metabase/visualizations/components/settings/ChartSettingFieldPicker.unit.spec.js index 78d2a846d83..7616531c2db 100644 --- a/frontend/src/metabase/visualizations/components/settings/ChartSettingFieldPicker.unit.spec.js +++ b/frontend/src/metabase/visualizations/components/settings/ChartSettingFieldPicker.unit.spec.js @@ -2,7 +2,7 @@ import { within } from "@testing-library/react"; import { renderWithProviders, screen } from "__support__/ui"; -import ChartSettings from "metabase/visualizations/components/ChartSettings"; +import { ChartSettings } from "metabase/visualizations/components/ChartSettings"; import registerVisualizations from "metabase/visualizations/register"; registerVisualizations(); diff --git a/frontend/src/metabase/visualizations/components/settings/ChartSettingStacked.unit.spec.js b/frontend/src/metabase/visualizations/components/settings/ChartSettingStacked.unit.spec.js index 2b1c246a9b4..479cda507ed 100644 --- a/frontend/src/metabase/visualizations/components/settings/ChartSettingStacked.unit.spec.js +++ b/frontend/src/metabase/visualizations/components/settings/ChartSettingStacked.unit.spec.js @@ -1,6 +1,6 @@ // these tests use ChartSettings directly, but logic we're testing lives in ChartNestedSettingSeries import { renderWithProviders, screen } from "__support__/ui"; -import ChartSettings from "metabase/visualizations/components/ChartSettings"; +import { ChartSettings } from "metabase/visualizations/components/ChartSettings"; import registerVisualizations from "metabase/visualizations/register"; registerVisualizations(); diff --git a/frontend/src/metabase/visualizations/visualizations/BarChart.unit.spec.tsx b/frontend/src/metabase/visualizations/visualizations/BarChart.unit.spec.tsx index 1bcb0b19fc2..a628c711646 100644 --- a/frontend/src/metabase/visualizations/visualizations/BarChart.unit.spec.tsx +++ b/frontend/src/metabase/visualizations/visualizations/BarChart.unit.spec.tsx @@ -1,6 +1,6 @@ import { createMockMetadata } from "__support__/metadata"; import { renderWithProviders, screen } from "__support__/ui"; -import ChartSettings from "metabase/visualizations/components/ChartSettings"; +import { ChartSettings } from "metabase/visualizations/components/ChartSettings"; import registerVisualizations from "metabase/visualizations/register"; import Question from "metabase-lib/v1/Question"; import type { Series } from "metabase-types/api"; diff --git a/frontend/src/metabase/visualizations/visualizations/PivotTable/PivotTable.unit.spec.js b/frontend/src/metabase/visualizations/visualizations/PivotTable/PivotTable.unit.spec.js index 5b62ddcdda2..ba199b1e152 100644 --- a/frontend/src/metabase/visualizations/visualizations/PivotTable/PivotTable.unit.spec.js +++ b/frontend/src/metabase/visualizations/visualizations/PivotTable/PivotTable.unit.spec.js @@ -4,7 +4,7 @@ import { useState } from "react"; import { createMockMetadata } from "__support__/metadata"; import { render, screen } from "__support__/ui"; -import ChartSettings from "metabase/visualizations/components/ChartSettings"; +import { ChartSettings } from "metabase/visualizations/components/ChartSettings"; import registerVisualizations from "metabase/visualizations/register"; import Question from "metabase-lib/v1/Question"; import { createMockColumn } from "metabase-types/api/mocks"; diff --git a/frontend/src/metabase/visualizations/visualizations/Table.unit.spec.js b/frontend/src/metabase/visualizations/visualizations/Table.unit.spec.js index 3fd48decc6d..88a395ff9a0 100644 --- a/frontend/src/metabase/visualizations/visualizations/Table.unit.spec.js +++ b/frontend/src/metabase/visualizations/visualizations/Table.unit.spec.js @@ -4,7 +4,7 @@ import { useState } from "react"; import { createMockMetadata } from "__support__/metadata"; import { renderWithProviders, screen, within } from "__support__/ui"; -import ChartSettings from "metabase/visualizations/components/ChartSettings"; +import { ChartSettings } from "metabase/visualizations/components/ChartSettings"; import registerVisualizations from "metabase/visualizations/register"; import Question from "metabase-lib/v1/Question"; import { createMockVisualizationSettings } from "metabase-types/api/mocks"; -- GitLab